Rename `runtime-metrics` to `runtime-telemetry-jmx` #8165

roberttoyonaga · 2023-03-29T20:49:26Z

Summary of Changes
Rename the JMX metrics gatherer to runtime-telemetry-jmx to be consistent with its JFR counterpart. This change was also talked about a bit here: open-telemetry/opentelemetry-java-contrib#749 (comment)

This is a follow up to: #7886

Edit after discussion below

Restructuring and renaming has been done according to "Alternative C"
The Java17 entry point RuntimeMetrics sets up the metric gatherers from both JMX and JFR implementations.
The agent instrumentation in the Java8 module does not get applied if the Java17 instrumentation is being applied (so the JMX gatherers don't get registered twice.)
Set the scope for both JMX and JFR implementations to be io.opentelemetry.instrumentation.runtime-metrics
Made the JMX observables closeable similar to the JFR ones

trask · 2023-04-01T18:28:06Z

hey @roberttoyonaga!

we discussed the naming of these modules in Java SIG meeting this week

I had a couple of concerns with the current proposal (runtime-telemetry-jmx and runtime-telemetry-jfr):

from user perspective (especially for library instrumentation users), they are "instrumenting the Java Runtime", and jmx or jfr are just implementation details
the difference between the runtime-telemetry-jmx and jmx-metrics artifacts seems potentially confusing

A couiple of alternatives were discussed:

Alternative A

runtime-metrics/
- runtime-metrics-java8/
- runtime-metrics-java17/

this mirrors other instrumentations which apply to different versions

@mateuszrzeszutek one thought I had since we discussed is that it seems confusing that Java 17 library users would want to use both -java8 and -java17 artifacts (and initialize them both in their code)

Alternative B

have a single runtime-metrics module which is a multi-release jar

@laurit @mateuszrzeszutek is the only argument against mrjar the added complexity?

we have one mrjar example module:

opentelemetry-java-instrumentation/instrumentation/resources/library/build.gradle.kts

Line 5 in c59c370

val mrJarVersions = listOf(9, 11)

and while that's not being used by the javaagent, it does look like AgentClassLoader supports mrjars:

opentelemetry-java-instrumentation/javaagent-bootstrap/src/test/groovy/io/opentelemetry/javaagent/bootstrap/AgentClassLoaderTest.groovy

Line 56 in c59c370

def "multi release jar"() {

so maybe added complexity is not too bad?

mateuszrzeszutek · 2023-04-03T11:52:43Z

and while that's not being used by the javaagent, it does look like AgentClassLoader supports mrjars:

AFAIR we use that in the resource providers module.

so maybe added complexity is not too bad?

I suspect most of the complexity will be in implementing the runtime-metrics module in a way that applies as many metrics as possible, depending on the Java version used. E.g. do we have a single entrypoint for all metrics (like JFR) or separate classes (JMX-style)? For the Java 17+ features, how should they appear in the public API of the module (which must be Java 8 compatible)?

roberttoyonaga · 2023-04-03T15:13:27Z

one thought I had since we discussed is that it seems confusing that Java 17 library users would want to use both -java8 and -java17 artifacts (and initialize them both in their code)

Could an Alternative C be: Use the same structure as in "Alternative A" but have the entry point in runtime-metrics-java17 also set up the metrics from runtime-metrics-java8. Since we would be drawing a distinction between versions and not underlying implementations, it would be fine to make it transparent to the user that we are using both JMX and JFR. This way a user only needs to directly use the java 8 or java 17 module, depending on their java version. Then we could rename the JFR entry point to a more generalized name.

I'm not that knowledgeable about multi release jars. But would the following make sense:

For the java agent, I think we could have a Java 8 implementation that only registers the JMX handlers, and a Java 17 version which registers JFR as well.

With respect to the libraries, I think it could be good to introduce a single entry point for JMX (similar to JFR) that can register all the JMX handlers (since they are all required anyway). Once that's in place, we can have an over-arching GeneralRuntimeMetricsEntryPoint that abstracts over the JMX and JFR single entry points and provides the public API for users. In the mrjar this GeneralRuntimeMetricsEntryPoint can either call the JMX and JFR or only the JMX entrypoints, depending on the java version.

laurit · 2023-04-03T15:49:43Z

One potential issue with mrjar is that class loader needs to support mrjar for it to work. If user includes the metrics library in his war and deploys it into his favourite app server then this app server has to support mrjar to get access to the jdk17 features. As mrjars have been around for a while it is quite possible that support for them has been implemented everywhere where it matters.

trask · 2023-04-04T02:38:01Z

For the Java 17+ features, how should they appear in the public API of the module (which must be Java 8 compatible)?

ah, yes, like opt-ing in to some higher-cardinality metrics, e.g. open-telemetry/opentelemetry-specification#3352 (comment)

Could an Alternative C be: Use the same structure as in "Alternative A" but have the entry point in runtime-metrics-java17 also set up the metrics from runtime-metrics-java8

this idea makes good sense to me

In this case I don't think we need mrjar, and the javaagent instrumentation can be written so that it also only applies to java8 or java17 (but not both)

trask · 2023-05-10T02:27:24Z

@roberttoyonaga sorry for the delay, will merge this right after the 1.26.0 release tomorrow

mateuszrzeszutek · 2023-05-10T09:57:11Z

.../library/src/main/java/io/opentelemetry/instrumentation/runtimemetricsjava8/BufferPools.java

@@ -3,7 +3,7 @@
 * SPDX-License-Identifier: Apache-2.0
 */

-package io.opentelemetry.instrumentation.runtimemetrics;
+package io.opentelemetry.instrumentation.runtimemetricsjava8;


WDYT about renaming the package to ...runtimemetrics.java8? (or ..runtimemetrics.j8)

I think that the java8/java17 implementations are somewhat similar to our usual v1_2 package name suffixes; this is the same feature/instrumentation, but for different runtime versions.

ok I've renamed to ...runtimemetrics.java8 and ...runtimemetrics.java17

mateuszrzeszutek · 2023-05-10T10:02:33Z

.../library/src/main/java/io/opentelemetry/instrumentation/runtimemetricsjava8/BufferPools.java

-        .setDescription("The number of buffers in the pool")
-        .setUnit("{buffers}")
-        .buildWithCallback(callback(bufferBeans, BufferPoolMXBean::getCount));
+  public static void closeObservers() {


Instead of keeping a global stateful list of observers, WDYT about making the registerObservers(OpenTelemetry) function return a Closeable? The users can decide how to handle these themselves.

I think this is a good idea. I have applied your suggestion.

mateuszrzeszutek · 2023-05-10T10:03:48Z

...rc/main/java/io/opentelemetry/instrumentation/runtimemetricsjava8/JmxRuntimeMetricsUtil.java


-  private static final String INSTRUMENTATION_NAME = "io.opentelemetry.runtime-metrics";
+  private static final String INSTRUMENTATION_NAME =
+      "io.opentelemetry.instrumentation.runtime-metrics";


We usually name our instrumentations io.opentelemetry.<name-version>

Suggested change

"io.opentelemetry.instrumentation.runtime-metrics";

"io.opentelemetry.runtime-metrics-java8";

...c/main/java/io/opentelemetry/instrumentation/runtimemetricsjava17/RuntimeMetricsBuilder.java

...ary/src/main/java/io/opentelemetry/instrumentation/runtimemetricsjava17/HandlerRegistry.java

...pentelemetry/instrumentation/javaagent/runtimemetricsjava8/Java8RuntimeMetricsInstaller.java

mateuszrzeszutek · 2023-05-10T10:25:19Z

...ntelemetry/instrumentation/javaagent/runtimemetricsjava17/Java17RuntimeMetricsInstaller.java

+    if (config.getBoolean("otel.instrumentation.runtime-metrics-java17.enable-all", false)) {
+      runtimeMetrics = RuntimeMetrics.builder(openTelemetry).enableAllFeatures().build();
+    } else if (config.getBoolean("otel.instrumentation.runtime-metrics-java17.enabled", false)) {
+      runtimeMetrics = RuntimeMetrics.create(openTelemetry);


I wonder if we should enable both JMX+JFR default metrics if otel.instrumentation.runtime-metrics.enabled is true.

Hmm yeah we talked a bit about this here. It was decided that we'd wait for the semantic conventions to be updated first in-case the JFR metrics end up changing.

Hmm, I see.
At the very least, let's add a condition checking otel.instrumentation.runtime-metrics.enabled somewhere in this method -- the current version of that is impossible to turn off on java 17+

Yes that's a good point. I'll add the same check that's done in Java8RuntimeMetricsInstaller before enabling the JMX gathered metrics.

roberttoyonaga · 2023-05-10T13:10:16Z

@roberttoyonaga sorry for the delay, will merge this right after the 1.26.0 release tomorrow

Thanks @trask ! Actually, can you hold off on merging for another day? I'm going to try implementing Mateusz's new suggestions.

…nstrumentation into jmx-rename

roberttoyonaga · 2023-05-11T01:27:55Z

ok @trask I'm done with the updates!

instrumentation/runtime-metrics/runtime-metrics-java17/library/README.md

instrumentation/runtime-metrics/runtime-metrics-java17/library/build.gradle.kts

...ary/src/main/java/io/opentelemetry/instrumentation/runtimemetrics/java17/RuntimeMetrics.java

mateuszrzeszutek · 2023-05-11T09:07:10Z

...c/main/java/io/opentelemetry/instrumentation/runtimemetrics/java8/JmxRuntimeMetricsUtil.java

@@ -12,10 +12,9 @@
 import java.util.List;
 import javax.annotation.Nullable;

-class JmxRuntimeMetricsUtil {
+public class JmxRuntimeMetricsUtil {


Can you move it to an .internal package?

Ok I've moved it to io.opentelemetry.instrumentation.runtimemetrics.java8.internal now

…java17

trask · 2023-05-11T19:52:05Z

@dependabot update

trask · 2023-05-11T19:52:20Z

@opentelemetrybot update

CHANGELOG.md

…java-instrumentation into jmx-rename

trask · 2023-05-16T00:30:50Z

thx @roberttoyonaga!

rename

5217379

roberttoyonaga requested a review from a team March 29, 2023 20:49

github-actions bot requested a review from theletterf March 29, 2023 20:49

mateuszrzeszutek approved these changes Mar 30, 2023

View reviewed changes

roberttoyonaga added 3 commits April 12, 2023 12:46

restructure

221f4a6

fix conflicts

7cc4c24

fix broken link

ee6981f

mateuszrzeszutek reviewed May 10, 2023

View reviewed changes

roberttoyonaga added 2 commits May 10, 2023 17:41

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

fd62bfc

…nstrumentation into jmx-rename

pr comments

d95a0ab

roberttoyonaga requested a review from mateuszrzeszutek May 11, 2023 01:27

mateuszrzeszutek reviewed May 11, 2023

View reviewed changes

roberttoyonaga added 2 commits May 11, 2023 14:39

review comments and add to Changelog.md

484dcbf

check if metric gathering is enabled before enabling JMX metrics for …

bf9c04e

…java17

This was referenced May 11, 2023

Integrate JMX metrics gatherers into JfrFeature enum #8473

Open

GarbageCollector metrics gatherer should return a closeable similar to the other JMX gatherers #8475

Closed

Merge remote-tracking branch 'origin/main' into jmx-rename

2ceece2

mateuszrzeszutek approved these changes May 15, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

roberttoyonaga added 2 commits May 15, 2023 11:09

merge main

33069c0

Merge branch 'jmx-rename' of github.com:roberttoyonaga/opentelemetry-…

9743244

…java-instrumentation into jmx-rename

changelog and style fixes

0de4f51

trask merged commit 3d0971b into open-telemetry:main May 16, 2023

laurit mentioned this pull request May 16, 2023

Fix runtime-metrics-java17 tests on openj9 #8499

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename `runtime-metrics` to `runtime-telemetry-jmx` #8165

Rename `runtime-metrics` to `runtime-telemetry-jmx` #8165

roberttoyonaga commented Mar 29, 2023 •

edited

Loading

trask commented Apr 1, 2023

mateuszrzeszutek commented Apr 3, 2023

roberttoyonaga commented Apr 3, 2023

laurit commented Apr 3, 2023

trask commented Apr 4, 2023

trask commented May 10, 2023

mateuszrzeszutek May 10, 2023

roberttoyonaga May 11, 2023

mateuszrzeszutek May 10, 2023

roberttoyonaga May 11, 2023

mateuszrzeszutek May 10, 2023

roberttoyonaga May 11, 2023

mateuszrzeszutek May 10, 2023

roberttoyonaga May 11, 2023

mateuszrzeszutek May 11, 2023

roberttoyonaga May 11, 2023

roberttoyonaga commented May 10, 2023

roberttoyonaga commented May 11, 2023

mateuszrzeszutek May 11, 2023

roberttoyonaga May 11, 2023

trask commented May 11, 2023

trask commented May 11, 2023

trask commented May 16, 2023

	"io.opentelemetry.instrumentation.runtime-metrics";
	"io.opentelemetry.runtime-metrics-java8";

Rename runtime-metrics to runtime-telemetry-jmx #8165

Rename runtime-metrics to runtime-telemetry-jmx #8165

Conversation

roberttoyonaga commented Mar 29, 2023 • edited Loading

trask commented Apr 1, 2023

Alternative A

Alternative B

mateuszrzeszutek commented Apr 3, 2023

roberttoyonaga commented Apr 3, 2023

laurit commented Apr 3, 2023

trask commented Apr 4, 2023

trask commented May 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberttoyonaga commented May 10, 2023

roberttoyonaga commented May 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trask commented May 11, 2023

trask commented May 11, 2023

trask commented May 16, 2023

Rename `runtime-metrics` to `runtime-telemetry-jmx` #8165

Rename `runtime-metrics` to `runtime-telemetry-jmx` #8165

roberttoyonaga commented Mar 29, 2023 •

edited

Loading