-
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
Enable monitoring buffer for elastic-agent #30471
Enable monitoring buffer for elastic-agent #30471
Conversation
This pull request does not have a backport label. Could you fix it @michel-laterman? 🙏
NOTE: |
This pull request is now in conflicts. Could you fix it? 🙏
|
# buffer: | ||
# enabled: false | ||
# period: 10s | ||
# size: 60 | ||
# namespaces: ['stats'] |
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've added the full buffer config here, but I'm not sure if we should. Currently the enabled
flag is the only one that's passed (it's injected via command line args at the moment).
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.
So the other values would be noop, if this is the case we should not expose 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.
I agree if its not exposed then it should be hidden.
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
/package |
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, just some questions for context:
beat metrics are collected in the diag command over grpc by way of an http endpoint. does that sound right?
are there any docs i can read up on the metrics libbeat exports this way?
Co-authored-by: Bryan Clement <bclement01@gmail.com>
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.
@michel-laterman Can we add a few tests to ensure the behavior?
# buffer: | ||
# enabled: false | ||
# period: 10s | ||
# size: 60 | ||
# namespaces: ['stats'] |
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.
So the other values would be noop, if this is the case we should not expose 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.
Looks good to me, minus the one comment.
# buffer: | ||
# enabled: false | ||
# period: 10s | ||
# size: 60 | ||
# namespaces: ['stats'] |
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 agree if its not exposed then it should be hidden.
@ph, as we discussed I think testing for the diagnostics commands should be added to e2e tests, i've made an issue to track this here: elastic/e2e-testing#2204 |
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
@michel-laterman will you be able to merge it before your time off? |
yep, as soon as the e2e passes i'll merge |
/test |
I'm going to force this through, all checks succeeded in c8339ad and I have only changed default config to remove unused values. |
What does this PR do?
Enable the monitoring buffer for the elastic-agent and beats that it starts.
Add metrics collection (from buffers) into the
diagnostics collect
bundle if enabled.Why is it important?
Allows us to gather metrics data for debugging if agent->ES communications are failing.
Checklist
I have added tests that prove my fix is effective or that my feature worksCHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.How to test this PR locally
Start agent and run
elastic-agent diagnostics collect
. Metrics should not be included in the archive by default.Enable metrics buffers by adding
agent.monitoring.http.buffer.enabled: true
then rerunelastic-agent diagnostics collect
, metrics (json) files should be added to the archiveRelated issues