jmx metrics: require explicit unit in yaml #13796

SylvainJuge · 2025-04-29T13:23:51Z

Related discussion: #13777
Resolves: #13812

With JMX metrics, the default metric unit is currently null, which means metrics are not properly exported unless a metric is explicitly provided in the YAML. Unfortunately this is quite easy to miss and it's probably better to report with an unexpected metric rather than not report anything.

The Java SDK currently explicitly tests for empty units, which means it should be something supported and thus reported.

As an alternative, we could maybe treat a missing metric as an error and throw an exception during parsing.

~~Bonus: take that opportunity to refactor and simplify a bit yaml parsing.~~

Update: as discussed in the comments, we switched the approach to throw an error when the unit is not explicitly set, this PR has thus been updated to reflect this new approach.

trask · 2025-04-29T19:42:02Z

I added to this week's SIG agenda to discuss

require unit in yaml, vs
default to empty unit

...etrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/RuleParserTest.java

…etry/instrumentation/jmx/engine/RuleParserTest.java Co-authored-by: Jay DeLuca <jaydeluca4@gmail.com>

SylvainJuge · 2025-04-30T06:51:33Z

I added to this week's SIG agenda to discuss

require unit in yaml, vs

default to empty unit

Thanks, I won't be able to attend both May 1st and May 8th SIG meetings though, my take on this would be to:

if we provide a default, we need to make sure it's the same default as the SDK, if there is any.
if we decide to throw an exception, then it makes mis-configuration early and and prevents unit-less metrics capture which might create other complications downstream.

jack-berg · 2025-05-01T16:55:55Z

As an alternative, we could maybe treat a missing metric as an error and throw an exception during parsing.

Maybe you meant throw an exception if the unit isn't provided?

We discussed in the 5/1/2025 java SIG and like this approach:

Make unit a required part of the schema, and throw if the key is not present or the value is null
Allow user to set the unit to an empty string "", since this is allowed in the SDK.
The effect is to strongly encourage users to set the unit since its an important part of the metrics data model, but still allow them to set an empty unit in cases where the unit is unknown

trask · 2025-05-01T17:20:46Z

@SylvainJuge @PeterF778 one open question is how disruptive this change might be for existing users, and whether we want to wait to make the change in 3.0 or not?

PeterF778 · 2025-05-01T18:43:36Z

@SylvainJuge @PeterF778 one open question is how disruptive this change might be for existing users, and whether we want to wait to make the change in 3.0 or not?

I think that all JMX metrics do have well-defined units, so it should be fine to demand that the units are specified in the yaml file. Also, as I understand, even today the users are forced to specify units in order to get the metrics exported (see the original motivation for the PR), even if this seems to be incorrect behavior on the SDK part.

I'm all behind the solution as summarized by @jack-berg .

SylvainJuge · 2025-05-02T11:44:07Z

Thanks for discussing this during last SIG meeting, I also think it's unlikely to impact users as they currently don't have metrics without units being exported.

Implementation-wise, while the parsing will throw an exception, the end result of this parsing error would be a warning in the agent log so we should not have to wait for 3.0 here.

For existing users with unspecified metrics they would still get more value with a warning rather than a silent failure.

If you all agree here I will update this PR accordingly.

…ry#13785)

…pen-telemetry#13800) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

…h2-http to v1.34.0 (open-telemetry#13807) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

…y#13803)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Lauri Tulmin <ltulmin@splunk.com>

…erimentalTelemetry (open-telemetry#13791)

…nstrumentation into jmx-use-empty-unit-as-default

SylvainJuge · 2025-05-06T12:34:26Z

...jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/BeanGroup.java

@@ -34,10 +33,6 @@ private BeanGroup(@Nullable QueryExp queryExp, List<ObjectName> namePatterns) {
    this.namePatterns = namePatterns;
  }

-  public static BeanGroup forSingleBean(String bean) throws MalformedObjectNameException {


[for reviewer] removed as not used anymore.

SylvainJuge · 2025-05-06T12:35:30Z

...ion/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java

+  public void addBean(String bean) {
+    if (beans == null) {
+      beans = new ArrayList<>();
+    }
+    beans.add(validateBean(bean));
+  }


[for reviewer] simplify a bit by allowing to add any number of beans using either bean (single) or beans (list)

SylvainJuge · 2025-05-06T12:36:46Z

...ion/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java

+              null,
+              "",


[for reviewer] using an empty string as unit seems an appropriate choice for state metrics.

...tion/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/Metric.java

SylvainJuge · 2025-05-06T12:43:16Z

...tion/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/Metric.java

+      metricType = jmxRule.getMetricType();
+    }
+    if (metricType == null) {
+      metricType = MetricInfo.Type.GAUGE;


[for reviewer] The same default is applied in MetricInfo constructor, moving it here makes it happen a bit earlier. I expect a later refactor of this would bring the complete building and validation logic into this method.

SylvainJuge · 2025-05-06T12:44:04Z

...metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/MetricStructure.java

+    try {
+      this.metricType = MetricInfo.Type.valueOf(t);
+    } catch (IllegalArgumentException e) {
+      throw new IllegalArgumentException("Invalid metric type: " + t, e);
+    }


[for reviewer] this allows to make the error message nicer for the end user when type is invalid.

SylvainJuge · 2025-05-06T12:45:15Z

...metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/MetricStructure.java

@@ -167,7 +174,7 @@ static MetricAttribute buildMetricAttribute(String key, String target) {
    // an optional modifier may wrap the target
    // - lowercase(param(STRING))
    boolean lowercase = false;
-    String lowerCaseExpr = tryParseFunction("lowercase", targetExpr, target);
+    String lowerCaseExpr = tryParseFunction("lowercase", targetExpr, errorMsg);


[for reviewer] this fixes a typo that made the error message harder to read, same for others below.

SylvainJuge · 2025-05-06T12:46:30Z

...etrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/RuleParserTest.java

-        () -> {
-          JmxConfig config = parseConf(yaml);
+  private static void runNegativeTest(String yaml, String message) {
+    assertThatThrownBy(


[for reviewer] using assertj to make testing for exception message easier.

SylvainJuge · 2025-05-06T12:46:57Z

...etrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/RuleParserTest.java

+      try {
+        rule.buildMetricDef();
+      } catch (Exception e) {
+        throw new IllegalStateException(e.getMessage(), e);


[for reviewer] here throwing the original message allows to keep the assertion on message content the same.

SylvainJuge · 2025-05-06T12:47:50Z

...etrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/RuleParserTest.java

@@ -616,7 +641,7 @@ void testNoBeans() {
            + "  - mapping:           # still no beans\n"
            + "      A:\n"
            + "        metric: METRIC_NAME\n";
-    runNegativeTest(yaml);
+    runNegativeTest(yaml, "No ObjectName specified");


[for reviewer] previously any parsing error would make the test pass, adding the expected exception message allows to ensure that we trigger the validation error we expect.

SylvainJuge · 2025-05-06T12:56:48Z

I've updated the PR to make the unit mandatory in YAML, this allowed to uncover that state metrics need to have a unit too, so I've picked empty string as unit for now.

trask · 2025-05-06T18:09:37Z

I've picked empty string as unit for now.

maybe "1" would be a better default unit?

https://ucum.org/ucum#section-Semantics:~:text=rather%20than%20the-,default%20unit%201,-.%20%C2%A0%C2%A0%E2%96%A03%20Curly

cc @jack-berg

PeterF778 · 2025-05-06T18:15:16Z

I've picked empty string as unit for now.

maybe "1" would be a better default unit?

https://ucum.org/ucum#section-Semantics:~:text=rather%20than%20the-,default%20unit%201,-.%20%C2%A0%C2%A0%E2%96%A03%20Curly

cc @jack-berg

I have been thinking about it as well, and it was my first thought. However, the unit of 1 suggests cardinality which is not really here. With state metrics we really deal with boolean values, which, by necessity, are mapped into 0 or 1.

trask · 2025-05-06T18:21:47Z

With state metrics we really deal with boolean values, which, by necessity, are mapped into 0 or 1.

yeah, though I think we're reporting them as updowncounters, which implies they can be aggregated?

PeterF778 · 2025-05-06T18:42:52Z

yeah, though I think we're reporting them as updowncounters, which implies they can be aggregated?

Good point. @SylvainJuge , do you see a use case for aggregation of state metrics? If not, perhaps we should revisit the type of the metric.
But the question of the proper metric unit remains.

SylvainJuge · 2025-05-07T08:23:03Z

For state metrics, the aggregation can be used to provide a "count" of the total number of observed items. Because the value is always 0 or 1 I think aggregation should always produce something useful as we are "counting things that have a state".

For example, with the Tomcat connector example:

the tomcat.connector aggregated (no attribute) metric provides the total number of connectors for which the state is known
the tomcat.connector metric provides breakdown per network port and connector state
the tomcat.connector aggregated metric could be used to measure the relative coverage of the connector states compared to the total number of tomcat instances. For example if you expect 2 connectors per instance and you have 5 instances, then when tomcat.connector is less than 10 it means something is missing on at least once instance.

I think we can get similar usages with the hw.state metric.

So, if the meaning attached to the 1 unit is to convey that it is something that be counted, then it should probably make sense to use it over empty string, however reading the semconv metrics recommendations we might as well interpret this as a "counting states" metric, in which case we could also use {state} (sorry for introducing another option here).

So we could have at least 4 possible choices for the unit of state metrics:

"" : matches current state of this PR, this is also the option that closely matches the hw.state in hardware semconv this state metrics was inspired from, where there is no unit for hw.state.
1 : may convey a sense of "something that can be counted", also aligns with the notion of "dimensionless metrics" as used here in semconv
{state} : makes it clear that we are "counting things in a state", and could also propagate the fact that the metric is a "state metric"
bit : reflects that the value is boolean and either 0 or 1, from https://ucum.org/ucum#section-Semantics:~:text=rather%20than%20the-,default%20unit%201,-.%20%C2%A0%C2%A0%E2%96%A03%20Curly.

I don't have any strong opinion here, but the empty string seems the simplest path for now:

it aligns with the current hw.state metric that has no unit
if we want to change the unit, it requires to work with HW semconv to ensure we are moving in the same direction, so likely quite a bit of effort to change things and deal with the downstream changes.
as a secondary quest, we could generalize the concept of "state metrics" into semantic conventions, so both the HW semconv and the JMX metrics documentations could refer to it, this would also cover the common definition of state metrics that are updowncounters with a value of 0 or 1 and whatever we decide for the metric unit.

...tion/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/Metric.java

instrumentation/jmx-metrics/README.md

robsunday · 2025-05-14T09:33:45Z

...ion/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/JmxRule.java

+    if (beans == null) {
+      beans = new ArrayList<>();
+    }
+    beans.add(validateBean(bean));


[minor] beans could actually be a Set to prevent duplicates

instrumentation/jmx-metrics/README.md

…nstrumentation into jmx-use-empty-unit-as-default

jmx metrics: use empty string '' as unit by default

51a6e31

SylvainJuge requested a review from a team as a code owner April 29, 2025 13:23

update documentation

db49849

SylvainJuge self-assigned this Apr 29, 2025

SylvainJuge added the bug Something isn't working label Apr 29, 2025

SylvainJuge added 2 commits April 29, 2025 17:24

fix ignore default value

ae5e9b4

refactor yaml: write 'bean' into 'beans'

aa4b07d

jaydeluca reviewed Apr 30, 2025

View reviewed changes

...etrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/engine/RuleParserTest.java Outdated Show resolved Hide resolved

Update instrumentation/jmx-metrics/library/src/test/java/io/opentelem…

2e8a33d

…etry/instrumentation/jmx/engine/RuleParserTest.java Co-authored-by: Jay DeLuca <jaydeluca4@gmail.com>

jaydeluca mentioned this pull request May 4, 2025

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

Closed

SylvainJuge and others added 14 commits May 6, 2025 14:26

remove unused method

1b60573

Fix test failure on latest redisson (open-telemetry#13798)

f68f8c0

fix doc: jvm.cpu.time JMX metric supported (open-telemetry#13795)

dd25a11

Implement stable semconv for db connection pool metrics (open-telemet…

cd57765

…ry#13785)

Remove operation name from graphql span name (open-telemetry#13794)

d42ab35

fix(deps): update dependency com.uber.nullaway:nullaway to v0.12.7 (o…

f677a91

…pen-telemetry#13800) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Fix NPE in netty 3.8 instrumentation (open-telemetry#13801)

f873e62

Update writing-instrumentation.md (open-telemetry#13805)

b809a7c

fix(deps): update dependency com.google.auth:google-auth-library-oaut…

c2121be

…h2-http to v1.34.0 (open-telemetry#13807) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Delete deprecated property for disabling kafka metrics (open-telemetr…

2fdc718

…y#13803)

chore(deps): update weekly update (open-telemetry#13813)

8457c42

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Lauri Tulmin <ltulmin@splunk.com>

make yaml metric unit mandatory

429d0e1

Try to fix link check rate limiting (open-telemetry#13816)

ee464b4

Polish variable naming from enableExperimentalJmxTelemetry to emitExp…

dacd9b8

…erimentalTelemetry (open-telemetry#13791)

add Cloud foundry resource provider (open-telemetry#13782)

1528d93

github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label May 6, 2025

SylvainJuge changed the title ~~jmx metrics: use empty string as unit by default~~ jmx metrics: require explicit unit in yaml May 6, 2025

SylvainJuge added 3 commits May 6, 2025 14:32

spotless

68c3571

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

24cd676

…nstrumentation into jmx-use-empty-unit-as-default

doc clarify unit of state metrics

66cdcb7

SylvainJuge commented May 6, 2025

View reviewed changes

PeterF778 approved these changes May 6, 2025

View reviewed changes

jaydeluca reviewed May 13, 2025

View reviewed changes

...tion/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/Metric.java Outdated Show resolved Hide resolved

instrumentation/jmx-metrics/README.md Outdated Show resolved Hide resolved

instrumentation/jmx-metrics/README.md Outdated Show resolved Hide resolved

robsunday reviewed May 14, 2025

View reviewed changes

SylvainJuge added 3 commits May 14, 2025 23:16

review changes

0d8c556

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

704d393

…nstrumentation into jmx-use-empty-unit-as-default

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

d0d7932

…nstrumentation into jmx-use-empty-unit-as-default

robsunday approved these changes May 20, 2025

View reviewed changes

jaydeluca approved these changes May 20, 2025

View reviewed changes

trask merged commit 1bc15f0 into open-telemetry:main May 20, 2025
87 checks passed

SylvainJuge deleted the jmx-use-empty-unit-as-default branch May 28, 2025 14:04

jmx metrics: require explicit unit in yaml #13796

jmx metrics: require explicit unit in yaml #13796

Uh oh!

Conversation

SylvainJuge commented Apr 29, 2025 • edited by jaydeluca Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trask commented Apr 29, 2025

Uh oh!

Uh oh!

SylvainJuge commented Apr 30, 2025

Uh oh!

jack-berg commented May 1, 2025

Uh oh!

trask commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PeterF778 commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SylvainJuge commented May 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SylvainJuge commented May 6, 2025

Uh oh!

trask commented May 6, 2025

Uh oh!

PeterF778 commented May 6, 2025

Uh oh!

trask commented May 6, 2025

Uh oh!

PeterF778 commented May 6, 2025

Uh oh!

SylvainJuge commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SylvainJuge commented Apr 29, 2025 •

edited by jaydeluca

Loading

trask commented May 1, 2025 •

edited

Loading

PeterF778 commented May 1, 2025 •

edited

Loading

SylvainJuge commented May 7, 2025 •

edited

Loading