Skip to content

[Monitoring] Once the buffer has any events, the collector is always ready - #36995

Merged
chrisronline merged 2 commits into
elastic:masterfrom
chrisronline:monitoring/fix_is_ready_for_stats_collector
May 24, 2019
Merged

chrisronline merged 2 commits into
elastic:masterfrom
chrisronline:monitoring/fix_is_ready_for_stats_collector

Conversation

@chrisronline

@chrisronline chrisronline commented May 23, 2019 •

Copy link
Copy Markdown
Contributor

Fixes #36991

Copied from the issue:

The work done in #36153 introduced an issue with one collectors isReady implementation - the stats collector within monitoring. The event roller is designed to accept events, then flush them and it's possible it doesn't have any at any given time. This is causing an issue with the /api/stats?extended endpoint where we are waiting for all collectors to be ready before returning usage data and the experience, because of this bug, is if you repeatably ping /api/stats?extended it will occasionally return a 503 indicating stats are not ready yet.

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:

for ((i=1;i<=100;i++));
do
  curl -I --show-error --fail --header "Content-type: application/json" -- "elastic:changeme@localhost:5601/api/stats?extended";
  sleep 1s
done

Note: I'm not sure we can add a test for this, as this configuration circumvents the potential 500 response

@elasticmachine

Copy link
Copy Markdown
Contributor

Pinging @elastic/stack-monitoring

@ycombinator

Copy link
Copy Markdown
Contributor

@chrisronline The setting in the test server is one we added, right? Just trying to recall...

@chrisronline

chrisronline commented May 23, 2019 •

Copy link
Copy Markdown
Contributor Author

@ycombinator

@chrisronline The setting in the test server is one we added, right? Just trying to recall...

Yes, that's right. Introduced by #36153

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@ycombinator

Copy link
Copy Markdown
Contributor

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)?

@chrisronline

Copy link
Copy Markdown
Contributor Author

I opened #37009 to track the need to add tests

@elasticmachine

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

isReady: () => {
if (!bufferHadEvents) {
bufferHadEvents = buffer.hasEvents();
}

Copy link
Copy Markdown
Contributor

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();

@ycombinator ycombinator left a comment

Copy link
Copy Markdown
Contributor

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.

@chrisronline

chrisronline commented May 24, 2019 •

Copy link
Copy Markdown
Contributor Author

Backport:

6.8: 54c73e2
7.2: e874a8e
7.x: 97a78d3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Monitoring] Ops collector is inconsistent with its isReady implementation

3 participants