Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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_summary metricset work for Stack Monitoring without xpack.enabled flag #20615
Make elasticsearch/index_summary metricset work for Stack Monitoring without xpack.enabled flag #20615
Changes from 2 commits
153b386
faa8ea1
209462a
b6e407c
b43e1f2
22c52f6
031d91c
78165c4
b57b32d
1993d99
05cc97d
fd2e40b
64635db
b67ca49
35a7209
c4934af
2abc9ae
ab25c29
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
If the field is not needed by the Stack Monitoring UI (@chrisronline may want to confirm) and it wasn't a field we were already collecting in
data.go
, then it's safe to not collect it at all. So I would remove this line and others like it.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 is correct. We need to not only collect the mappings, but also continue to collect and index any other fields we used to because we often read values from the
source
document that weren't considered when the finalized mappings were produced.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.
@chrisronline So just to clarify, you're saying we should collect this field,
is_throttled
, but we should not include it in thefields.yml
so it doesn't show up in the mappings, right?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.
Correct
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.
Understood 🙂
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 really even want to index fields that aren't used and we could take another pass on which fields we read from source directly, but we're also currently in talks about potentially indexing as much data as possible (even if the stack monitoring UI doesn't use it) to enable users to build custom dashboards. Nothing is decided but I'd rather leave the functionality as is in case we do go down that path
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.
Wait a sec 😄 as far as I understood, we should map this fields in elasticsearch
fields.yml
mapping. But should we still map it in the metricsetfields.yml
file? As a Metricbeat convention, it must be mapped, AFAIK 100% of Metricbeat fields are map infields.yml
of their respective metricsets. Metricbeat is not going to fail though, we however have a commonly used python test (like this one in MySQL module https://github.com/elastic/beats/blob/master/metricbeat/module/mysql/test_mysql.py#L47) to check if all fields are map infields.yml
but fortunately it's not included in the python test code for the elasticsearch module.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.
.monitoring-*
indices have dynamic mapping set tofalse
, but perhapsmetricbeat-*
handles this differently? In that case, maybe we should add every field we are indexing tofields.yml
but if the field isn't contained in the used mappings list, we set the mapping toenabled: false
?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.
@sayden I can try to help clarify about when and where to map fields for stack monitoring metricsets. 😅
Consider a field, say
elasticsearch.<foo>.bar.baz
that is newly (i.e. not already inmaster
) being collected by theelasticsearch/<foo>
metricset indata.go
. For this field:Go ahead and define the field in the
module/elasticsearch/<foo>/_meta/fields.yml
file.Now, check if the field has a corresponding field in the
.monitoring-es-*
index mappings..monitoring-es-*
issome_thing.some_thing_else.qux.boom
. Go ahead and define a field alias fromsome_thing.some_thing_else.qux.boom
=>elasticsearch.<foo>.bar.baz
in themodule/elasticsearch/_meta/fields.yml
file.module/elasticsearch/<foo>/_meta/fields.yml
and addenabled: false
to it. This will ensure that the field is mapped (so theassert_fields_are_documented
test assertion you are talking about in your comment will pass) but not be unnecessarily indexed.Hopefully this now clear as mud 😃.
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.
Good idea, @chrisronline re:
enabled: false
. I will edit my comment above to reflect this step.