-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Make elasticsearch/index_recovery metricset work for Stack Monitoring without xpack.enabled flag #20810
Make elasticsearch/index_recovery metricset work for Stack Monitoring without xpack.enabled flag #20810
Conversation
Pinging @elastic/stack-monitoring (Stack monitoring) |
Pinging @elastic/integrations-services (Team:Services) |
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.
LGTM!
@ycombinator I think this is ready for your final review, thanks! |
}, | ||
"index": { | ||
"name": ".monitoring-es-6-2018.11.20", |
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.
We are removing this field, which would be a breaking change. We should add it back.
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.
Bump
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.
Never mind, I see this has been updated now.
"name": "es1_1" | ||
}, | ||
"type": "EMPTY_STORE" | ||
"shards": [ |
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.
Hmmm, this is a tricky change.
This appears to be a breaking change too. Not so much defining a new elasticsearch.index.recovery.shards
field because that's an additive change. But more that we're taking away the elasticsearch.index.recovery.id
, elasticsearch.index.recovery.primary
, etc. fields.
I think the reason you had to do this is because previously each document produced by the index_recovery
metricset represented a single shard, whereas what Stack Monitoring expects each document to contain an array of shards.
@chrisronline what do you need the data type of this elasticsearch.index.recovery.shards
field to be for queries to work correctly: object
or nested
? Depending on that we might need to figure out how we handle this breaking change.
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.
We don't perform any aggregations across this data, so object
works fine.
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.
Okay, that's good news.
I'm still concerned about the breaking change in document structure here, for any existing users of the elasticsearch/index_recovery
metricset (non-xpack).
@chrisronline How much work would it be to make the UI work on the current (before this PR) structure of elasticsearch/index_recovery
metricset (non-xpack) documents where one document == one shard? Keep in mind that this would be code we'd get to clean up in 8.0 as we could introduce a breaking change in document structure in this metricset at that point.
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.
This should be possible. We only access this data from a single code path and adding any kind of funky logic in there is fine, knowing it can go away in 8.0.
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.
Thanks so much @chrisronline ❤️ . Essentially you'll need logic to check for the new structure (1 document == 1 shard) that will be produced by this metricset when its run without the xpack.enabled
flag but fall back to the current logic of expecting multiple shard inner documents in a single type: index_recovery
document. Hope that makes sense.
In that case, @sayden, we should keep the 1 document == 1 shard structure that is currently (prior to this PR) being produced by this metricset. We can add more fields to that document, if needed for Stack Monitoring, of course.
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.
@ycombinator Yes, that's my understanding. Should be fine
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.
Yeah! Ok! Sounds good! I'll revert to the OSS original structure of 1 doc == 1 shard.
@sayden I don't see a |
That's because there's no |
cab7374
to
b169e66
Compare
Hey @sayden, could you update the PR description please? It's not clear if this PR is still WIP and there are things you are still working on or if it's ready for me to review. Also, there are failing tests that look related, so I'm assuming you need to do more on this PR before it's ready for me to review again. In general, for each of these PRs, can I suggest a bit of process please?
Otherwise right now I'm having to periodically look at each PR and try to guess what state it's in, and whether it's awaiting my review. Thanks! |
# Conflicts: # metricbeat/docs/fields.asciidoc # metricbeat/module/elasticsearch/fields.go # metricbeat/module/elasticsearch/node_stats/_meta/fields.yml
b169e66
to
02be3b3
Compare
🐛 Flaky test report❕ There are test failures but not known flaky tests. Expand to view the summary
Test stats 🧪
Genuine test errors💔 There are test failures but not known flaky tests, most likely a genuine test failure.
|
b822954
to
5f8560d
Compare
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.
Code changes LGTM but tests are failing in CI and test failures look relevant.
58e2a7d
to
9de5959
Compare
…csearch/index_recovery_xpack_flag # Conflicts: # metricbeat/module/elasticsearch/fields.go
…csearch/index_recovery_xpack_flag
…csearch/index_recovery_xpack_flag
…csearch/index_recovery_xpack_flag # Conflicts: # metricbeat/module/elasticsearch/fields.go
…csearch/index_recovery_xpack_flag
…csearch/index_recovery_xpack_flag # Conflicts: # metricbeat/module/elasticsearch/fields.go
… without xpack.enabled flag (elastic#20810)
WIP
Missing steps:
fields.yml
elasticsearch/fields.yml
aliases