-
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
Adding beat module with stats metricset #12181
Conversation
With this the stats endpoint basically becomes a "documented" endpoint and the API and it's content should not change anymore. Perhaps a good time to revised the output we have to check for example if it follows ECS etc. At the same time we use this event already for the monitoring reporting, so we should not break too many things here? |
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.
Sorry for the early review. I'm stuck at the airport 😄
jenkins, test this |
Pinging @elastic/stack-monitoring |
Reviewers: there is a LOT of new code / files in this PR. However, they are all in service of a single logical end-goal: introducing the |
I would not change this API now, in a minor version. Perhaps we can do that in 8.0.0. However, what I would do now is make sure the non-xpack code path of the |
I tested this according to the steps outlined by @ycombinator and it worked as described! |
settings: ["ssl", "http"] | ||
short_config: false | ||
fields: | ||
- name: beats |
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.
Same as above, beat.*
instead?
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.
Done.
} | ||
} | ||
|
||
func TestData(t *testing.T) { |
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 not use the new testing mechanism here with the golden files?
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.
Sorry, I think I already forgot about this 😞. Could you point me to an example module that's using this?
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 help: https://github.com/elastic/beats/tree/master/metricbeat/mb/testing/data
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, I started to use this new testdata
framework in 627ecf2bb. However, as you will see the generated stats.800.json-expected.json
file is bad. It has a error.message
field with value as "HTTP error 404 in : 404 Not Found"
.
Can you help me figure out what I'm doing wrong? Better yet, can you help me figure out how to debug what's going on with this new framework 🎣?
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, I did some digging and I think I know why this is happening. The beats/stats
metricset makes 2 HTTP API calls: GET /
and GET /stats
.
I'm investigating now how to mock both these calls with the testdata
framework.
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.
AFAICT, there's no way currently for the testdata
framework to handle the scenario where a metricset needs to call multiple HTTP APIs. Can you confirm this?
If this is true, I'd rather not hold up this PR (and the one coming after it for the beat/state
metricset: #12615) for this enhancement to be implemented in the new test framework. Once the enhancement is there I can make a follow up PR to use it with these metricsets. WDYT?
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 can confirm unfortunately, see #11425 SGTM to move forward.
Co-Authored-By: Nicolas Ruflin <spam@ruflin.com>
Co-Authored-By: Nicolas Ruflin <spam@ruflin.com>
I've merged #12621 and rebased this PR on |
Skimmed through it and looks good to me. What I didn't check in detail is naming fields. It could be an opportunity to rethink how we report beats metrics (structure), at the same time this should not hold back this. |
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.
Changelog?
Re: API change (naming of fields), I mentioned it earlier in the PR (#12181 (comment)):
|
This PR adds a new
beat
module to Metricbeat. It also implements the first (of two) metricsets for this module,stats
.More specifically, this PR:
beat
module.stats
metricset for this module.modules.d/beat.yml
andmodules.d/beat-xpack.yml
module configuration files.Testing this PR
X-Pack code path (for Stack Monitoring UI)
Build metricbeat with this PR.
Using Kibana > Console, clear out existing Beats monitoring data.
Start Metricbeat with internal collection.
Wait 10 seconds or so for monitoring data to be collected.
Using Kibana > Console, retrieve the latest document from the
.monitoring-beats-*
index and save it.Stop Metricbeat.
Using Kibana > Console, clear out existing Beats monitoring data.
Start Metricbeat with HTTP API enabled for external collection.
In a new window enable the
beats-xpack
Metricbeat module for external collection and start this Metricbeat instance.Wait 10 seconds or so for monitoring data to be collected.
Using Kibana > Console, retrieve the latest document from the
.monitoring-beats-*
index and save it.Compare the JSON document saved from internal collection (step 5) with the JSON document saved from external collection (step 11). You might want to use a tool like jsondiff.com for this purpose.
The only structural difference between the two documents should be that the externally-collected JSON document should have 7 extra top-level fields:
@timestamp
,event
,ecs
,agent
,metricset
,service
,host
. These are expected extra fields that are always inserted by Metricbeat. Other than these expected structural differences, the rest of the structure should be identical; however, values of corresponding fields may be different.