-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Monitoring] Once the buffer has any events, the collector is always ready #36995
[Monitoring] Once the buffer has any events, the collector is always ready #36995
Conversation
Pinging @elastic/stack-monitoring |
@chrisronline The setting in the test server is one we added, right? Just trying to recall... |
Yes, that's right. Introduced by #36153 |
💔 Build Failed |
…ready_for_stats_collector
Hmmmm, yeah, I can't think of a way at the moment. Perhaps this is a test that could go into https://github.com/elastic/elastic-stack-testing (not as part of monitoring parity tests but maybe there's another place in that repo)? |
I opened #37009 to track the need to add tests |
💚 Build Succeeded |
isReady: () => { | ||
if (!bufferHadEvents) { | ||
bufferHadEvents = buffer.hasEvents(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: you could replace this if
block with:
bufferHadEvents = bufferHadEvents || buffer.hasEvents();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a nitpicky comment which you can take or ignore. LGTM.
Fixes #36991
Copied from the issue:
To test, repeatably hit the
/api/stats?extended
endpoint and ensure that once it returns a 200, it always return a 200.My test bash script:
Note: I'm not sure we can add a test for this, as this configuration circumvents the potential 500 response