-
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 metricset work for Stack Monitoring without xpack.enabled flag #21438
Make elasticsearch/index metricset work for Stack Monitoring without xpack.enabled flag #21438
Conversation
Pinging @elastic/stack-monitoring (Stack monitoring) |
Pinging @elastic/integrations-services (Team:Services) |
I'm seeing this error when running MB:
|
Okay! Fixed, sorry for that. I still don't understand very well why those errors do not appear to me because this was tested with ES to produce the |
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 I think this is ready for your final review, thanks! |
event.MetricSetFields, err = schema.Apply(index) | ||
// Convert struct to common.Mapstr by passing it to JSON first so we can store the data in the root of the | ||
// metricset level | ||
byt2, err := json.Marshal(idx) |
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.
Consider replacing byt2
with something more descriptive or at least less arbitrary?
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, good catch, I change its name to something more meaningful
event.MetricSetFields.Put("total.store.size.bytes", idx.Total.Store.SizeInBytes) | ||
event.MetricSetFields.Put("total.segments.memory.bytes", idx.Total.Segments.MemoryInBytes) |
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.
I'm curious why these two fields need special treatment here. Shouldn't they get marshalled & unmarshalled above along with all the other fields?
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.
Frankly, I don't remember either but I have double checked it and I think that it was an attempt to rename the fields to follow the naming conventions, that I later realized that I had way too many to change and then I regret and I forgot to remove those two.
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.
I finally removed them
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.
Ha! No, forget that. They were original fields https://github.com/elastic/beats/pull/21438/files#diff-397cb07191b7b507fd7ca773b66d961954da4c2bfc5694ab8cf997e1cf991037R44
Instead of creating an alias I just duplicated those fields. I'll add the alias.
# Conflicts: # metricbeat/docs/fields.asciidoc # metricbeat/module/elasticsearch/fields.go # metricbeat/module/elasticsearch/node_stats/_meta/fields.yml
f500817
to
c0ed397
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.
|
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 minor comment about a TODO
. Other than that, code changes LGTM.
However, tests are failing in CI and the failures look related to this PR. So please fix those before merging.
…without xpack.enabled flag (elastic#20615)
c0ed397
to
0c0ded2
Compare
58e2a7d
to
9de5959
Compare
…csearch/index_xpack_flag # Conflicts: # metricbeat/docs/fields.asciidoc # metricbeat/module/elasticsearch/fields.go # metricbeat/module/elasticsearch/node_stats/_meta/fields.yml
…csearch/index_xpack_flag # Conflicts: # metricbeat/docs/fields.asciidoc # metricbeat/module/elasticsearch/fields.go
…csearch/index_xpack_flag
Pinging @elastic/ingest-management (Team:Ingest Management) |
…csearch/index_xpack_flag # Conflicts: # metricbeat/module/elasticsearch/testing.go
…csearch/index_xpack_flag # Conflicts: # metricbeat/docs/fields.asciidoc # metricbeat/module/elasticsearch/_meta/fields.yml # metricbeat/module/elasticsearch/fields.go
/test metricbeat |
…csearch/index_xpack_flag # Conflicts: # metricbeat/docs/fields.asciidoc # metricbeat/module/elasticsearch/fields.go
…xpack.enabled flag (elastic#21438)
Ready to test with Kibana
Most of the fields will work with aliases but the fields that previously were on
index_stats.*
will now be in the same subpath but always underelasticsearch.index
root path (and withoutindex_stats
at the beginning)