-
Notifications
You must be signed in to change notification settings - Fork 534
monitoring: update apm-server metrics collection to avoid conflicts #17512
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
monitoring: update apm-server metrics collection to avoid conflicts #17512
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
This pull request is now in conflicts. Could you fix it @isaacaflores2? 🙏
|
Quickly tested this manually.
"sampling": {
"tail": {
"dynamic_service_groups": 1,
"events": {
"processed": 3,
"stored": 3
}
}
}
"sampling": {
"tail": {
"dynamic_service_groups": 1,
"events": {
"processed": 3,
"sampled": 3,
"stored": 3
},
"storage": {
"lsm_size": 5407,
"value_log_size": 0
}
}
}
apm-server:
host: "127.0.0.1:8200"
sampling.tail:
enabled: true
policies:
- sample_rate: 1.0
output.elasticsearch:
hosts: ["https://216c4d53f99e4e48a556e4cfe72cb680.us-central1.gcp.qa.cld.elstc.co:443"]
username: "elastic"
password: "<REDACTED>"
http:
enabled: true
host: localhost
port: 5066 |
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, thanks for adding the test
…17512) (#17527) * monitoring: update apm-server metrics collection to avoid conflicts * refactor beats monitoring since there are no more global registries * removed redundant temp slice from apm-server monitoring func (cherry picked from commit f1c279b) Co-authored-by: Isaac Flores <34590010+isaacaflores2@users.noreply.github.com>
…ion to avoid conflicts (#17526) * monitoring: update apm-server metrics collection to avoid conflicts (#17512) * monitoring: update apm-server metrics collection to avoid conflicts * refactor beats monitoring since there are no more global registries * removed redundant temp slice from apm-server monitoring func (cherry picked from commit f1c279b) * fix merge conflict issues caused by global registries --------- Co-authored-by: Isaac Flores <34590010+isaacaflores2@users.noreply.github.com> Co-authored-by: Isaac Flores <isaac.flores@elastic.co>
// register all metrics once | ||
// this prevents metrics with the same prefix in the name | ||
// from different scoped meters from overwriting each other | ||
reportOnKey(v, beatsMetrics) |
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.
Great finding!
…tion to avoid conflicts (#17525) * monitoring: update apm-server metrics collection to avoid conflicts (#17512) * monitoring: update apm-server metrics collection to avoid conflicts * refactor beats monitoring since there are no more global registries * removed redundant temp slice from apm-server monitoring func (cherry picked from commit f1c279b) # Conflicts: # internal/beatcmd/beat_test.go * fix merge conflict in beat_test.go --------- Co-authored-by: Isaac Flores <34590010+isaacaflores2@users.noreply.github.com> Co-authored-by: Isaac Flores <isaac.flores@elastic.co> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Motivation/summary
Currently
apm-server.sampling.tail.storage.lsm_size
andapm-server.sampling.tail.dynamic_service_groups
are never reported together.Testing locally this is caused because both sets of metrics use the same namespace (metric name prefix)
apm-server.sampling
but they are created using different instances of a Meter.The related monitoring func calls
addAPMServerMetrics
multiple times for each scoped metric. Metric names with the same prefix in different scopes are somehow overwriting each other. This approach opts to collect all "apm-server" metrics and add them to the snapshot once. Another approach would be to update theelastic-agent-lib
to prevent metrics from overwriting each other.Checklist
For functional changes, consider:
How to test these changes
manual test
./sendotlp -insecure -endpoint=http://localhost:8200 -secret-token=<token>
storage
should be visible along with other metrics underapm-server.sampling.tail
Related issues
Closes #17342
Alternate approach to #17427