-
Notifications
You must be signed in to change notification settings - Fork 168
[jmx-scraper] Create weaver model for jmx-scraper metrics #2072 #2362
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
base: main
Are you sure you want to change the base?
[jmx-scraper] Create weaver model for jmx-scraper metrics #2072 #2362
Conversation
…try#2072 - created metrics.yaml weaver model - created attriutes.yaml - tested with weaver registry generate
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 - please look at the CI?
|
I am not familiar with weaver, does it means that the jmx scraper yaml configuration would be now generated from registry+template ? Maybe we could modify the yaml format to make this easier or even avoid having to use a template. Do you have an high level overview of what this change helps to achieve? Also, the metrics descriptions are part of what is being tested in unit tests, so when changing them you'll have to update the test code as well. |
|
Thanks all. sorry for the delay! The tests are fixed now. Re: @SylvainJuge Perhaps @atoulme can chime in since he created the original request. As far as I understand, one of the main benefits coming out of this is standardization of metrics. Ultimately, we can have a centralized registry of metrics. That would become the single source of truth / semconv. I think that's what @atoulme has in mind "The idea is that eventually we can move some of the weaver models to semconv, as metric references." |
|
If I understand this correctly, this PR does the following:
With that, modifying the JMX metrics yaml would require expertise in jinja and in running weaver with a makefile, which is not how things are usually done in this java-only project. What I think would be simpler here is to keep the JMX metrics yaml as source of truth and then generate the weaver yaml files from it as part of the build process. The benefits I see for this approach would be the following:
I will need to prototype this to validate this is a viable approach, I will try to do this in the following weeks. This will need to happen in the instrumentation repository as the jmx-metrics implementation is part of it. In addition to that, we also need to deal with the following extra challenges:
Regarding the ability to push JMX metrics into semconv, this would be awesome but also challenging as product implementation might change across versions and would also require to create semconv SIG(s). I'd be happy to participate to that initiative when it starts. |
|
I've opened open-telemetry/opentelemetry-java-instrumentation#15196 to attempt to implement this directly into java instrumentation. |
Please upvote and consider participating in open-telemetry/weaver#792 I'm sure we can work out a gradle task to run a docker container, might take a bit to get the kinks out. The choice of jinja2 as a templating language seems like the least of all choices at this point, but please feel free to offer alternatives in the weaver repository?
It can work, but the weaver model is richer than the JMX metrics yaml though. |
| mapping: | ||
| {%- for m in ctx.groups if m.id.startswith(jvm_memory_prefix + "heap") %} | ||
| {%- if m.id == jvm_memory_prefix + "heap.committed" %} | ||
| HeapMemoryUsage.committed: |
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.
You could put an annotation on the metric and use it here if you wanted.
So your metrics.yaml would be something like:
groups:
- id: jvm.memory.head.committed
type: metric
...
annotations:
- jmx_name: HeapMemoryUsage.committed
Then here you could have the mapping generically pull the annotation:
{{ m.annotatiions.jmx_name }}:
metric: {{ m.id | ... }
...
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.
You could even go further and record the Bean / property and group-by bean, etc.
Description:
Feature / Improvement: use weaver model to describe metrics so that they can be standardized.
Testing:
Documentation:
Outstanding items: