Thread count and classes loaded handlers #571

roberttoyonaga · 2022-11-07T15:14:49Z

Description:
Add handlers for thread count and loaded classes count metrics. These are derived from jdk.ClassLoadingStatistics and jdk.JavaThreadStatistics JFR events

Testing:
A new unit test was added to test these handlers in jfr-streaming/src/test/java/io/opentelemetry/contrib/jfr/metrics.

linux-foundation-easycla · 2022-11-07T15:14:52Z

The committers listed above are authorized under a signed CLA.

✅ login: roberttoyonaga / name: Robert Toyonaga (b51a053, d244f91, fc24a17)

breedx-splk

Hi @roberttoyonaga. Thanks for this contribution. I made a few suggestions, but it looks pretty good overall. Appreciate it.

breedx-splk · 2022-11-08T20:06:10Z

...in/java/io/opentelemetry/contrib/jfr/metrics/internal/classLoading/ClassesLoadedHandler.java

+  @Override
+  public void initializeMeter(Meter meter) {
+    meter
+        .gaugeBuilder(METRIC_NAME)


This event has unloadedClassCount and loadedClassCount, both of which seem like they would be better modeled by monotonically increasing counters instead of a gauge. While unloads may be uncommon, they do happen, and if one of the goals of this class is to give an idea of the total classes currently loaded, both values should be tracked and the delta reported in the callback.

Also, please take a look at the OpenTelemetry JVM Metrics specification, which currently lists these metrics, which you should conform to:

process.runtime.jvm.classes.loaded

process.runtime.jvm.classes.unloaded

process.runtime.jvm.classes.current_loaded

I think this handler can probably provide all 3, but if you wanted to wait an do current_loaded in another implementation/PR that would be fine too.

Thank you for the review! I've changed it to use monotonically increasing counters and also added an implementation for process.runtime.jvm.classes.current_loaded with an upDownCounter. I've corrected the instrument, unit, and descriptions so that they now should conform to the specification.

breedx-splk · 2022-11-08T20:07:41Z

...in/java/io/opentelemetry/contrib/jfr/metrics/internal/classLoading/ClassesLoadedHandler.java

+  public Optional<Duration> getPollingDuration() {
+    return Optional.of(Duration.ofSeconds(1));
+  }
+}


Would appreciate a unit test that covers the behavior of this class in isolation.

breedx-splk · 2022-11-08T20:09:44Z

...ming/src/main/java/io/opentelemetry/contrib/jfr/metrics/internal/cpu/ThreadCountHandler.java

+  public Optional<Duration> getPollingDuration() {
+    return Optional.of(Duration.ofSeconds(1));
+  }
+}


Would like to see a unit test that covers this class in isolation as well.

breedx-splk · 2022-11-08T20:38:10Z

jfr-streaming/src/test/java/io/opentelemetry/contrib/jfr/metrics/JfrPeriodicMetricsTest.java

+  }
+
+  @Test
+  public void shouldHaveJfrPeriodicEvents() throws Exception {


I think there's bad precedent elsewhere in this repo, but with JUnit5 you shouldn't need the test class nor test method to be public I think. Preference is to just make them package to save chars and improve readability. :)

mateuszrzeszutek · 2022-11-09T12:38:25Z

...ming/src/main/java/io/opentelemetry/contrib/jfr/metrics/internal/cpu/ThreadCountHandler.java

+import jdk.jfr.consumer.RecordedEvent;
+
+public final class ThreadCountHandler implements RecordedEventHandler {
+  private static final String METRIC_NAME = "process.runtime.jvm.cpu.active_threads";


The OpenTelemetry spec defines process.runtime.jvm.threads.count, can you make sure that this implementation of JVM metrics conforms to it?

Thanks for the review! I've changed the thread count handler so that it now properly implements the specification.

breedx-splk

Thanks for following up with these changes! Looking good to me.

It's a slight bummer that the tests currently have these sleeps in them. That approach tends not to scale well as the size of the codebase grows, but we can circle back on that another time. I still would have liked to see unit tests for the handlers and not just integration style tests, but that too can be added later.

kittylyst

LGTM. I +1 @breedx-splk comments about not using sleep() in tests, but I don't think that should hold up a merge of this.

kittylyst · 2022-11-10T07:43:16Z

...rc/main/java/io/opentelemetry/contrib/jfr/metrics/internal/classes/ClassesLoadedHandler.java

+public final class ClassesLoadedHandler implements RecordedEventHandler {
+  private static final String METRIC_NAME_LOADED = "process.runtime.jvm.classes.loaded";
+  private static final String METRIC_NAME_UNLOADED = "process.runtime.jvm.classes.unloaded";
+  private static final String METRIC_NAME_CURRENT = "process.runtime.jvm.classes.current_loaded";


Do we document somewhere what the difference is between classes.loaded and classes.current_loaded? Maybe link to that definition here?

It's in the JVM metrics spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/runtime-environment-metrics.md#jvm-metrics

I added some comments for further clarity. The metric descriptions also add some information.

mateuszrzeszutek · 2022-11-10T11:56:06Z

...rc/main/java/io/opentelemetry/contrib/jfr/metrics/internal/classes/ClassesLoadedHandler.java

+public final class ClassesLoadedHandler implements RecordedEventHandler {
+  private static final String METRIC_NAME_LOADED = "process.runtime.jvm.classes.loaded";
+  private static final String METRIC_NAME_UNLOADED = "process.runtime.jvm.classes.unloaded";
+  private static final String METRIC_NAME_CURRENT = "process.runtime.jvm.classes.current_loaded";


It's in the JVM metrics spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/runtime-environment-metrics.md#jvm-metrics

.../src/main/java/io/opentelemetry/contrib/jfr/metrics/internal/threads/ThreadCountHandler.java

jfr-streaming/src/main/java/io/opentelemetry/contrib/jfr/metrics/internal/Constants.java

jfr-streaming/src/test/java/io/opentelemetry/contrib/jfr/metrics/JfrThreadCountTest.java

…cs/internal/threads/ThreadCountHandler.java Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>

…:roberttoyonaga/opentelemetry-java-contrib into thread-count-and-classes-loaded-handlers

jfr-streaming/src/test/java/io/opentelemetry/contrib/jfr/metrics/JfrThreadCountTest.java

…cs/JfrThreadCountTest.java Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>

trask · 2022-11-11T16:57:21Z

thx @roberttoyonaga!

roberttoyonaga added 3 commits November 4, 2022 14:31

add thread count and tests

b51a053

add handler to registry

d244f91

add classes loaded count metric

fc24a17

roberttoyonaga requested a review from a team November 7, 2022 15:14

github-actions bot assigned breedx-splk, jack-berg and kittylyst Nov 7, 2022

github-actions bot requested review from breedx-splk, jack-berg and kittylyst November 7, 2022 15:15

breedx-splk requested changes Nov 8, 2022

View reviewed changes

breedx-splk reviewed Nov 8, 2022

View reviewed changes

mateuszrzeszutek reviewed Nov 9, 2022

View reviewed changes

Conform to spec. Update tests.

931a130

roberttoyonaga requested review from mateuszrzeszutek and breedx-splk and removed request for kittylyst, jack-berg, mateuszrzeszutek and breedx-splk November 9, 2022 19:21

minor touch up

4a7602e

github-actions bot requested review from jack-berg and kittylyst November 9, 2022 19:29

breedx-splk approved these changes Nov 10, 2022

View reviewed changes

kittylyst approved these changes Nov 10, 2022

View reviewed changes

mateuszrzeszutek reviewed Nov 10, 2022

View reviewed changes

Update jfr-streaming/src/main/java/io/opentelemetry/contrib/jfr/metri…

b02a36d

…cs/internal/threads/ThreadCountHandler.java Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>

roberttoyonaga added 2 commits November 10, 2022 11:23

comment. boolean attribute. simplify test case.

153a743

Merge branch 'thread-count-and-classes-loaded-handlers' of github.com…

182a2e1

…:roberttoyonaga/opentelemetry-java-contrib into thread-count-and-classes-loaded-handlers

mateuszrzeszutek approved these changes Nov 10, 2022

View reviewed changes

mateuszrzeszutek reviewed Nov 10, 2022

View reviewed changes

jfr-streaming/src/test/java/io/opentelemetry/contrib/jfr/metrics/JfrThreadCountTest.java Outdated Show resolved Hide resolved

Update jfr-streaming/src/test/java/io/opentelemetry/contrib/jfr/metri…

435c02d

…cs/JfrThreadCountTest.java Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>

trask merged commit ab64bfd into open-telemetry:main Nov 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thread count and classes loaded handlers #571

Thread count and classes loaded handlers #571

roberttoyonaga commented Nov 7, 2022

linux-foundation-easycla bot commented Nov 7, 2022 •

edited

Loading

breedx-splk left a comment

breedx-splk Nov 8, 2022

breedx-splk Nov 8, 2022

roberttoyonaga Nov 9, 2022

breedx-splk Nov 8, 2022

breedx-splk Nov 8, 2022

breedx-splk Nov 8, 2022 •

edited

Loading

roberttoyonaga Nov 9, 2022

mateuszrzeszutek Nov 9, 2022

roberttoyonaga Nov 9, 2022

breedx-splk left a comment

kittylyst left a comment

kittylyst Nov 10, 2022

mateuszrzeszutek Nov 10, 2022

roberttoyonaga Nov 10, 2022

mateuszrzeszutek Nov 10, 2022

trask commented Nov 11, 2022

Thread count and classes loaded handlers #571

Thread count and classes loaded handlers #571

Conversation

roberttoyonaga commented Nov 7, 2022

linux-foundation-easycla bot commented Nov 7, 2022 • edited Loading

breedx-splk left a 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

breedx-splk Nov 8, 2022 • edited Loading

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

breedx-splk left a comment

Choose a reason for hiding this comment

kittylyst left a 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

trask commented Nov 11, 2022

linux-foundation-easycla bot commented Nov 7, 2022 •

edited

Loading

breedx-splk Nov 8, 2022 •

edited

Loading