-
Notifications
You must be signed in to change notification settings - Fork 145
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
Fix jvm.thread.count
semconv.
#1734
Fix jvm.thread.count
semconv.
#1734
Conversation
If this change is going to be merged it would be good to make an update also in JMX Metrics Gatherer |
For enhancing the JMX metrics, I have opened open-telemetry/opentelemetry-java-instrumentation#13238 to discuss what are the options we can use to make the transition as smooth as possible. I think we could argue for this particular case that if "old" jmx gatherer definitions are inconsistent with semconv they should be fixed anyway because we have a very clear definition of the metrics definitions provided by semconv. However, this does not prevent introducing a breaking change for the consumers of those metrics, stable semconv definitions help to ensure it's the last. For the same reason we did not change the metric attributes that now need to be properly namespaced. So, we could do this change here, but it's quite unlikely we can easily do the same for all the metrics that are not part of semconv, for those we need to discuss it in open-telemetry/opentelemetry-java-instrumentation#13238. While ideally I would like to have all JMX metrics part of semconv, I don't think this is something easily achievable but I'd be happy to be proven wrong on this. |
After discussing this with @robsunday today, we think it would be best to do the following:
For the particular case of JVM metrics, it means we keep 3 distinct implementations, but once they are all aligned with semconv they won't drastically change nor require maintenance:
@breedx-splk given this widens the scope of the change you initially wanted to make, I am volunteering to open a PR on your PR, or alternatively if that's simpler we can close this PR and I can take over and create a new one. |
I think it's just better overall to use the correct names everywhere...but that's just me. |
I could not agree more, especially when the "correct name" has been defined in semconv. Does it works for you if I open another PR to fix all those definitions mismatch at once for the JVM metrics or do you prefer to merge this one first then do all the other ones separately ? While it's more work I think it's simpler for end-users to have to deal with a single one-time change for all the JVM metrics rather than potentially spreading it over many iterations. |
Yeah that's fine, especially if you have some main list or know of all the gaps right now. Happy to close this one....I just tend to favor smaller incremental work over mega PRs. But if it's mostly name changes should be fine. |
I opened open-telemetry/opentelemetry-java-instrumentation#13392 PR in instrumentation to allow to adding semconv-compliant metrics in YAML format, those can then later be reused in jmx-scraper as-is. |
The new scraper code is reporting the wrong name for
jvm.thread.count
(semconv here).Even if this was done to keep parity with the old module, this new name is wrong and will have a negative impact on systems that rely on the correct name.