Skip to content

JMX Rule unit is required but not documented or validated #13812

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.

Sign up for GitHub

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

Jump to bottom
Closed
john-hyun-eb opened this issue May 4, 2025 · 5 comments · Fixed by #13796
Closed

JMX Rule unit is required but not documented or validated #13812

john-hyun-eb opened this issue May 4, 2025 · 5 comments · Fixed by #13796
Labels
bug Something isn't working needs triage New issue that requires triage

Comments

@john-hyun-eb
Copy link

Describe the bug

It appears JMX Rules require a unit be specified but it is not mentioned in the documentation and there is not a validation for it in the code. The rule is silently dropped.

Steps to reproduce

Create a JMX rules yaml file for either JMX Scraper, Agent or SDK. A simple one would be:

rules:
  - bean: "java.lang:type=Runtime"
    prefix: my.
    type: gauge
    mapping:
      Pid:
        desc: Process ID

This is just a simple example to show the behavior.

Expected behavior

The logs should show

<date> INFO status.yaml: found 1 metric rules
<date> INFO Created Gauge for my.Pid

And should emit the metrics for my.Pid

Actual behavior

The logs show

<date> INFO status.yaml: found 1 metric rules

But does not indicate it created a metric nor does it emit the metric.

Javaagent or library instrumentation version

2.15.0

Environment

JDK: Temurin 17
OS: MacOS

Additional context

There should probably be some validation for this further up the chain but I determined this by stepping through the code and seeing an NPE get swallowed when the metric was being registered in:

https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricRegistrar.java#L76

UnitConverter unitConverter = UnitConverter.getInstance(sourceUnit, unit);

The call to UnitConverter.getInstance(sourceUnit, unit) throws an NPE because unit is null. Again, it should probably be validated somewhere else as I've seen error messages for when other properties are missing, etc.

Note - this stems from trying to create a "State" metric and the example for it does not specify a unit.

https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/jmx-metrics
@john-hyun-eb john-hyun-eb added bug Something isn't working needs triage New issue that requires triage labels May 4, 2025
@john-hyun-eb
Copy link
Author

Sorry, to clarify the above - the expected behavior is there should be some error message if unit is missing. Everything works as long as a unit is specified. The fix is to add validation to surface that a required field is missing (or let the NPE bubble up but that is probably not preferred). Also, update the documentation to indicate unit is required and update the the State metric example to specify a unit. I was trying to implement a State metric by copy and pasting the example and this led to several hours of trying to figure out why it wasn't working.

@jaydeluca
Copy link
Member

Hey @john-hyun-eb, there is actually a PR open where this issue is being discussed (and addressed): #13796

the expected behavior is there should be some error message if unit is missing

👍 This is what was discussed and decided upon at the recent SIG meeting as well. The PR name/description might say that it intends to add a default value, but there will be an update that will instead switch to throwing an exception when it is missing.

@john-hyun-eb
Copy link
Author

Oh awesome! I guess I only looked at the issues and not the pull requests and discussions. Closing.

@jaydeluca
Copy link
Member

I'm going to re-open this issue until we merge that PR, just in case someone else encounters this and also doesn't see the PR or discussion

@jaydeluca jaydeluca reopened this May 6, 2025
@jaydeluca jaydeluca mentioned this issue May 6, 2025
Merged
@SylvainJuge
Copy link
Contributor

@jaydeluca #13796 has been merged last week, so I think we can close it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working needs triage New issue that requires triage
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

jmx metrics: require explicit unit in yaml SylvainJuge/opentelemetry-java-instrumentation
3 participants
@SylvainJuge @jaydeluca @john-hyun-eb