-
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.
|
I think the weaver yaml should be used as a "contract" to describe and document what signals are captured by a specific compolent, this allows to automate documentation and maybe generate other artifacts derived from it. In a sense, this is quite similar to the work around metadata in instrumentation, what is needed is the ability to provide an up-to-date weaver model to allow consumers to diff/migrate using weaver tool. My intent here would be to provide the following:
In addition, the users of JMX metrics are also likely to provide their own metrics definitions, so having the ability to generate the weaver yaml for them would also provide the ability for end-users to also generate documentation, versioning and migration of their custom data. I don't think using weaver to generate the JMX metrics yaml provides anything beyond generating one yaml from another, it just makes the maintenance of those JMX metrics yaml extra complicated due to the templating, also it does not allow to provide to end-users the ability to generate their own weaver yaml. In a sense, it would be trying to re-implement the JMX metrics yaml into the weaver yaml (we could even push it further by making JMX metrics reuse weaver yaml format, but that would be a breaking change and likely an extra challenge to document and maintain and help users migrating. However, I'm not against using the weaver itself, for example I agree that it would make sense to use it to generate documentation from the weaver yaml, this is currently done manually. I plan to experiment a bit this week trying to implement open-telemetry/opentelemetry-java-instrumentation#15196 as I think this is a simpler approach. |
|
Bearing in mind that
I think moving the primary configuration to new format is the wrong approach. Having something that converts between two formats is great, but it shouldn't require an extra tool to use the jmx-scraper |
Description:
Feature / Improvement: use weaver model to describe metrics so that they can be standardized.
Testing:
Documentation:
Outstanding items: