-
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/cluster_stats metricset work for Stack Monitoring without xpack.enabled flag #21099
Make elasticsearch/cluster_stats metricset work for Stack Monitoring without xpack.enabled flag #21099
Conversation
Pinging @elastic/stack-monitoring (Stack monitoring) |
Pinging @elastic/integrations-services (Team:Services) |
I'm seeing this while running this PR:
|
I'm seeing this error now:
Are you able to run this successfully against ES? |
I'm seeing this error on startup still:
|
I can now run this PR and it indexes properly! Thanks for fixing that! I'm seeing an issue with the document though. In existing
which returns:
I'm not seeing that in the |
The The error is more a misconception: You'll never find a
Another solution is to create a mapping, which is included as an example in the PR now. But you'll have to "query" this info:
Note that we query the I also removed some data from the incoming data from Elasticsearch to save some space. I have included it again just in case. Sorry for the long explanation 😅 I just wanted to make sure that I explained it properly. EDIT: Forgot to mention: In the mappings https://gist.github.com/chrisronline/3b123c85d03b97ec3cbf63d9d72b3e2c there was no mention to |
I forgot to push the commit that brings back the data at |
I'm seeing an error running this now:
|
Chris, can you briefly describe a step by step way to reproduce this error? Somehow it's happening in all metricsets but I cannot manage to reproduce it (I'm usually just guessing what the mapping issue is). The way I do it is:
From there, I don't get any error and I usually copy-paste the generated event from the line |
My steps which go against the same ES cluster
Am I doing something wrong here? |
It must be me the one who is doing something wrong but I can't manage to understand why because, esentially, we are doing the same thing. You delete MB ES resources and I Anyways, the mapping error was fixed. |
9a43848
to
0a93e93
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.
LGTM!
e7a761b
to
0b57912
Compare
"stats": c.Ifc("policy_stats"), | ||
}, | ||
}), | ||
//"watcher": c.Ifc("watcher"), |
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.
Why is this field commented out?
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 forgot to add a comment for double check it's not required (I was hoping it wasn't). If Chris hasn't complain I guess it's not so I have removed 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.
Is this data contained in the stack_stats
field?
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.
It wasn't included when you first tested it, @chrisronline I guess it's safe to remove it
) | ||
|
||
func TestMapper(t *testing.T) { | ||
elasticsearch.TestMapperWithInfo(t, "./_meta/test/cluster_stats.*.json", eventMapping) | ||
// TODO This test must be skipped until we find a reasonable way to test the event mapping when it has to do extra | ||
// HTTP calls |
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.
Could you mock an HTTP server using http.NewServeMux
? IIRC we are doing this in a few places in the Beats codebase if you want to try and follow some examples.
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.
Ah yes, the problem I had with this test is the helper.HTTP
which is not easy to use in tests. Actually it's not used in any test file apart from some fake test in rabbitmq. The cleanest way to fix it is refactoring the helper as you can see in this branch #22573 with the solution applied as example.
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, the problem is that you need to pass in the Metricset and that's not easy to construct in tests.
I took a look at the refactoring PR (#22573) and it LGTM.
…csearch/cluster_stats_xpack_flag
Okay I have a final version now. I had to remove 2 fields that could be affected according to this mapping https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/core/src/main/resources/monitoring-es.json#L503
The reason is that an alias cannot point to an object type. According to docs "The target must be a concrete field, and not an object or another field alias." Take a final look and tell me if I can do something as a workaround |
We don't need the fields aliased right? We just read them from source for telemetry purposes.
Not yet, but it will be done for elastic/kibana#73864 |
/test metricbeat |
Gotcha. Yeah, if we just read them from source then we don't need the aliases. But then I imagine Telemetry code will need to be updated to also look at the new non-aliased field paths? Will this also be done as part of elastic/kibana#73864? |
…csearch/cluster_stats_xpack_flag # Conflicts: # metricbeat/module/elasticsearch/elasticsearch_integration_test.go # metricbeat/module/elasticsearch/fields.go
/test metricbeat |
…csearch/cluster_stats_xpack_flag
…csearch/cluster_stats_xpack_flag
…csearch/cluster_stats_xpack_flag # Conflicts: # metricbeat/module/elasticsearch/elasticsearch_integration_test.go # metricbeat/module/elasticsearch/fields.go
…csearch/cluster_stats_xpack_flag
…csearch/cluster_stats_xpack_flag
… without xpack.enabled flag (elastic#21099)
Ready to test in Kibana. data.json is included to help troubleshooting and it has been generating testing the Metricbeat binary directly with Elasticsearch