JMX Metric Insight #6573

PeterF778 · 2022-09-09T22:54:29Z

Solving #6131 (JMX Support).

Solving open-telemetry#6131 (JMX Support).

Correcting a typo in a comment.

Using the latest version of snakeyaml. Version 1.30 had this vulnerability https://nvd.nist.gov/vuln/detail/CVE-2022-25857.

instrumentation/runtime-metrics/library/src/main/resources/jmx/rules/kafka-consumer.yaml

.../java/io/opentelemetry/instrumentation/javaagent/runtimemetrics/RuntimeMetricsInstaller.java

instrumentation/runtime-metrics/library/README.md

...library/src/main/java/io/opentelemetry/instrumentation/runtimemetrics/jmx/MetricService.java

instrumentation/runtime-metrics/library/README.md

...c/main/java/io/opentelemetry/instrumentation/runtimemetrics/jmx/AttributeValueExtractor.java

...cs/library/src/main/java/io/opentelemetry/instrumentation/runtimemetrics/jmx/BeanFinder.java

PeterF778 · 2022-09-23T20:50:32Z

Thank you @jack-berg for the review and your comments!

…strative purposes.

instrumentation/runtime-metrics/library/jetty.md

instrumentation/runtime-metrics/library/kafka-broker.md

PeterF778 · 2022-09-26T23:19:44Z

Thank you, @trask, for the review. I'm working on an update for the PR, I should have it ready later this week.

…upported platforms. Introducing system property to control time interval between MBean discovery attempts. Code refactoring.

trask

@PeterF778 thanks for your patience with the slow reviews

...brary/src/main/java/io/opentelemetry/instrumentation/jmx/engine/AttributeValueExtractor.java

instrumentation/jmx/javaagent/README.md

Renaming module 'jmx' to 'jmx-metrics'. Changing the syntax of the metric attributes retrieved from Mbean attributes. Editing docs and comments for better readability.

instrumentation/jmx-metrics/jmx-yaml/library/build.gradle.kts

…ttribute'. Just within the README file.

PeterF778 · 2022-10-07T16:54:00Z

My last commit changes the documentation for the YAML syntax, replacing label with attribute.
Once the new syntax is agreed upon, I'll change the code and the built-in metric rules. Right now they are out-of-sync with the documentation.

trask · 2022-10-20T19:22:55Z

I played around with a few possible changes to the yaml structure, with questionable results.

1. Changing `mapping` to `metric`

e.g., change

    mapping:
      Usage.used:
        metric: my.own.jvm.memory.pool.used
        type: updowncounter
        desc: Pool memory currently used
        unit: By

to

    metric:
      Usage.used:
        name: my.own.jvm.memory.pool.used
        type: updowncounter
        desc: Pool memory currently used
        unit: By

Reason: more explicit name for what the things under it represent.

My take: I kind of like this, though maybe mapping is better given the considerations below in # 2 (?)

2 Change `attribute` to `metricAttribute`

e.g., change

    attribute:
      pool: param(name)
      type: beanattr(Type)

to

    metricAttribute:
      pool: param(name)
      type: beanattr(Type)

Reason: more explicit name for what the things under it represent, especially given the confusion of bean attribute vs metric attribute.

My take: I kind of like this (but probably makes most sense with # 1 above also)

3 Changing `mapping` from a map to a list

e.g., change

    mapping:
      Usage.used:
        metric: my.own.jvm.memory.pool.used
        type: updowncounter
        desc: Pool memory currently used
        unit: By

to

    metrics:
      - beanAttr: Usage.used
        name: my.own.jvm.memory.pool.used
        type: updowncounter
        desc: Pool memory currently used
        unit: By

Reason: give names to the beanAttr field, instead of having it be "anonymous" key for the map.

My take: I don't think this helps. We are mapping from mbeans to otel metrics in this file, so I think it is not too confusing for the key to be the mbean attribute.

4 Change `attribute` from a map to a list

e.g., change

    attribute:
      pool: param(name)
      type: beanattr(Type)

to

    metricAttributes:
      - name: pool
        value: param(name)
      - name: type
        value: beanattr(Type)

Reason: give names to the attribute name field, instead of having it be "anonymous" key for the map.

My take: I don't think this helps.

PeterF778 · 2022-10-21T23:10:34Z

Thank you @trask for your experiments and suggestions. They all have merits, and I'll address them in the increasing order of my subjective agreement with them.

Re: 3 Changing mapping from a map to a list

It looks good, and is readable, but it does not allow a shortcut notations as demonstrated in the section Making shortcuts, with simplified one-line mapping from MBean attributes to metrics. I believe many users would appreciate such simplicity, even if only for experimenting with the available MBeans.

Re: 1. Changing mapping to metric

I think the key to understanding the essence of the proposed yaml schema for the metric rules is the realization that they provide a mapping from MBean attributes to OTEL metrics. Looking at

metric:
      Usage.used:
        name: my.own.jvm.memory.pool.used
        type: updowncounter
        desc: Pool memory currently used
        unit: By

one does not necessarily see that this is a mapping. It looks like Usage.used is a part of a metric or a metric definition name. In reality, the metric definition starts only below Usage.used, so saying metric above it can be confusing, IMHO.

On the other hand, the originally proposed schema does not say that the anonymous key is an MBean attribute, but I thought it was an acceptable sacrifice to achieve that one-line mappings mentioned above. It did say mapping and metric though, even if the latter is optional.

Re: 4 Change attribute from a map to a list

Modeling metric attributes as a list is a common mental shortcut, but they are not really a list of pairs (name, value), but a mapping from names to values. A nuance for some, I know ...

BTW, point 3 has the same minor flaw.

Also I believe that in most cases there will be just one element in such a metric attribute list. Many of the metrics we encountered in our metric modeling so far follow a pattern in which a rule has one metric attribute defined globally (for all MBean attributes) and one specific to each metric.

Re: 2 Change attribute to metricAttribute

It is longer than I would like. We went from tag to label and then to attribute, but if it clears potential confusion, I'm fine with it.

Please let me know your thoughts on the above. Thanks!

trask · 2022-10-26T03:58:12Z

Re: 1. Changing mapping to metric

I think the key to understanding the essence of the proposed yaml schema for the metric rules is the realization that they provide a mapping from MBean attributes to OTEL metrics. Looking at

👍

and I think eliminates 3. Changing mapping from a map to a list as well

I think that just leaves the attributes section to discuss a bit further.

One additional thought about the current attribute section is that while the mapping section is a mapping from MBean attributes to OTel metrics, the attribute map goes in the opposite direction, from OTel to MBean.

Interestingly, I find the attribute section pretty clear when its under the mapping section, since at that point we're talking about how to map the thing to OTel, so the context makes it clear (to me at least) that we're talking about how to populate otel attributes.

The top-level attribute section doesn't read as clearly for me though, since I'm thinking mbean, and so I'm thinking mbean attribute, and then thinking the attribute key is an mbean attribute name instead of an otel attribute name:

  - bean: kafka.streams:type=stream-thread-metrics,thread-id=*
    attribute:
      threadId: param(thread-id)

any other thoughts for making this part clearer?

…()" instead of "attr()", and "const(STRING)" instead of "STRING".

.../src/main/java/io/opentelemetry/instrumentation/javaagent/jmx/JmxMetricInsightInstaller.java

linux-foundation-easycla · 2022-11-03T21:13:02Z

The committers listed above are authorized under a signed CLA.

✅ login: PeterF778 / name: Peter Findeisen (d496304, 58f9e19, 5f29c94, 9dfe6aa, 7cebb63, 0ca7a0d, 009dff6, b2ea55d, 5c0783d, 7f89168, 7e151ae, 46198c2, 112a04d, 252be45, 830a18f, fcd6ca0, 5afd3ce, 87f166a, a3b40f3, c509c8b)
✅ login: trask / name: Trask Stalnaker (239b42a)

...n/jmx-metrics/library/src/main/java/io/opentelemetry/instrumentation/jmx/yaml/JmxConfig.java

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

trask · 2022-11-04T02:33:45Z

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

+ *
+ * <p>Objects of this class are immutable.
+ */
+public class MetricBanner {


I'm not following the association with the word Banner?

The class encapsulates metric properties which help identify/explain it to end users via a hypothetical UI, before looking at the metric values or other technical details. They represent the facet of the metric which will most likely be looked at first when browsing a list of metrics. Other possible names considered were MetricTitle or MetricManifest. What name would you propose?

maybe MetricInfo?

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

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

...trics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricAttribute.java

...rary/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricAttributeExtractor.java

trask · 2022-11-04T02:49:31Z

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

+  private final BeanPack beans;
+
+  // Describes how to get the metric values and their attributes, and how to report them
+  private final MetricExtractor[] metricExtractors;


we typically use List<> over array unless its super perf sensitive, or dealing with an inherently array-based underlying api

The constructors for MetricDef and a number of other classes take varargs which are translated directly to arrays. And the varargs are there to make the direct access to those classes easier when attempting to hard-code some JMX rules - with the examples of that missing from the first PR.

trask · 2022-11-04T02:49:53Z

...trics/library/src/main/java/io/opentelemetry/instrumentation/jmx/engine/MetricExtractor.java

+  private final BeanAttributeExtractor attributeExtractor;
+
+  // Defines the Measurement attributes to be used when reporting the metric value.
+  private final MetricAttribute[] attributes;


similar for List over array

It is the same case - the constructor uses varargs, so internal representation as array is a natural fit.

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

...ibrary/src/test/java/io/opentelemetry/instrumentation/jmx/engine/AttributeExtractorTest.java

trask · 2022-11-09T06:07:13Z

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

+ *
+ * <p>Objects of this class are immutable.
+ */
+public class MetricBanner {


maybe MetricInfo?

committed by mistake This reverts commit 0c93fe5.

trask

Thanks @PeterF778, this is a great addition!

svrnm · 2022-11-16T09:14:10Z

Wow! Amazing to see this merged, thank you @PeterF778 & @trask !

tylerbenson · 2022-12-05T20:12:50Z

@PeterF778 I assume that thread/memory metrics aren't given a "target" is because they're already covered in runtime-metrics. I suggest mentioning this in the readme to make it more clear.

PeterF778 · 2022-12-06T00:11:24Z

@PeterF778 I assume that thread/memory metrics aren't given a "target" is because they're already covered in runtime-metrics. I suggest mentioning this in the readme to make it more clear.

Yes, several JVM metrics (threads/memory/GC) as well as other types of metrics are covered independently by the agent. Where in the readme file would you suggest putting a note about it?

tylerbenson · 2022-12-06T20:32:00Z

instrumentation/jmx-metrics/javaagent/README.md

+
+To control the time interval between MBean detection attempts, one can use the `otel.jmx.discovery.delay` property, which defines the number of milliseconds to elapse between the first and the next detection cycle. JMX Metric Insight may dynamically adjust the time interval between further attempts, but it guarantees that the MBean discovery will run perpetually.
+
+## Predefined metrics


@PeterF778 this section is where I would suggest adding a reference/link to the runtime metrics, otherwise it seems like an oversight for people that are looking for jmx memory metrics but don't know that runtime metrics are separate.

Yes, I see your point. Could you please create an issue (https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues) for this so it gets tracked?

PeterF778 requested a review from a team September 9, 2022 22:54

PeterF778 added 3 commits September 22, 2022 11:32

JMX Metric Insight

d496304

Solving open-telemetry#6131 (JMX Support).

JMX Metric Insight

58f9e19

Correcting a typo in a comment.

JMX Metric Insight

5f29c94

Using the latest version of snakeyaml. Version 1.30 had this vulnerability https://nvd.nist.gov/vuln/detail/CVE-2022-25857.

PeterF778 force-pushed the jmx-metric-insight branch from 5dbb684 to 5f29c94 Compare September 23, 2022 00:10

jack-berg reviewed Sep 23, 2022

View reviewed changes

Updating the documentation to clarify that examples are just for illu…

9dfe6aa

…strative purposes.

trask reviewed Sep 26, 2022

View reviewed changes

PeterF778 added 3 commits September 28, 2022 10:03

Code refactoring and changes, moving to own module :instrumentation:jmx.

7cebb63

Cleanup in runtime-metrics.

0ca7a0d

Removing kafka-consumer and kafka-producer metrics from the list of s…

009dff6

…upported platforms. Introducing system property to control time interval between MBean discovery attempts. Code refactoring.

trask reviewed Oct 5, 2022

View reviewed changes

PeterF778 added 3 commits October 5, 2022 13:25

Fixing the issue with always null logger in AttributeValueExtractor.

b2ea55d

Renaming module 'jmx' to 'jmx-metrics'. Changing the syntax of the metric attributes retrieved from Mbean attributes. Editing docs and comments for better readability.

Correcting a typo

5c0783d

Merging libraries jmx-engine and jmx-yaml into one.

7f89168

trask reviewed Oct 6, 2022

View reviewed changes

instrumentation/jmx-metrics/jmx-yaml/library/build.gradle.kts Outdated Show resolved Hide resolved

PeterF778 added 2 commits October 6, 2022 15:21

Cleanup - removing leftover file.

7e151ae

Making changes to the accepted YAML syntax: replacing 'label' with 'a…

46198c2

…ttribute'. Just within the README file.

PeterF778 mentioned this pull request Oct 27, 2022

Replace runtime.jvm.gc.time/runtime.jvm.gc.count metrics with process.runtime.jvm.gc.duration histogram #6964

Merged

PeterF778 and others added 3 commits October 28, 2022 10:26

Using "metricAttribute" in place of "label" or "attribute", "beanattr…

112a04d

…()" instead of "attr()", and "const(STRING)" instead of "STRING".

Refining comments

252be45

Merge remote-tracking branch 'upstream/main' into jmx-metric-insight

239b42a

github-actions bot requested a review from theletterf October 31, 2022 22:55

trask reviewed Nov 3, 2022

View reviewed changes

trask reviewed Nov 4, 2022

View reviewed changes

trask reviewed Nov 9, 2022

View reviewed changes

PeterF778 added 6 commits November 15, 2022 14:10

Post-review code changes

830a18f

Revert "Post-review code changes"

fcd6ca0

committed by mistake This reverts commit 0c93fe5.

Post-review code changes

5afd3ce

Post-review code changes, renaming, more comments.

87f166a

Post-review code changes, more renaming.

a3b40f3

Post-review change: class renaming

c509c8b

PeterF778 force-pushed the jmx-metric-insight branch from 2ae7eda to c509c8b Compare November 15, 2022 22:12

trask approved these changes Nov 15, 2022

View reviewed changes

trask merged commit 4db65b6 into open-telemetry:main Nov 16, 2022

PeterF778 mentioned this pull request Nov 17, 2022

Kafka Streams metrics #6778

Closed

tylerbenson reviewed Dec 6, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JMX Metric Insight #6573

JMX Metric Insight #6573

PeterF778 commented Sep 9, 2022

PeterF778 commented Sep 23, 2022

PeterF778 commented Sep 26, 2022

trask left a comment

PeterF778 commented Oct 7, 2022

trask commented Oct 20, 2022

PeterF778 commented Oct 21, 2022

trask commented Oct 26, 2022 •

edited

Loading

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

edited

Loading

trask Nov 4, 2022

PeterF778 Nov 4, 2022

trask Nov 9, 2022

trask Nov 4, 2022

PeterF778 Nov 4, 2022

trask Nov 4, 2022

PeterF778 Nov 4, 2022

trask Nov 9, 2022

trask left a comment

svrnm commented Nov 16, 2022

tylerbenson commented Dec 5, 2022

PeterF778 commented Dec 6, 2022

tylerbenson Dec 6, 2022

PeterF778 Dec 7, 2022


		To control the time interval between MBean detection attempts, one can use the `otel.jmx.discovery.delay` property, which defines the number of milliseconds to elapse between the first and the next detection cycle. JMX Metric Insight may dynamically adjust the time interval between further attempts, but it guarantees that the MBean discovery will run perpetually.

		## Predefined metrics

JMX Metric Insight #6573

JMX Metric Insight #6573

Conversation

PeterF778 commented Sep 9, 2022

PeterF778 commented Sep 23, 2022

PeterF778 commented Sep 26, 2022

trask left a comment

Choose a reason for hiding this comment

PeterF778 commented Oct 7, 2022

trask commented Oct 20, 2022

1. Changing mapping to metric

2 Change attribute to metricAttribute

3 Changing mapping from a map to a list

4 Change attribute from a map to a list

PeterF778 commented Oct 21, 2022

trask commented Oct 26, 2022 • edited Loading

linux-foundation-easycla bot commented Nov 3, 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

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

Choose a reason for hiding this comment

svrnm commented Nov 16, 2022

tylerbenson commented Dec 5, 2022

PeterF778 commented Dec 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1. Changing `mapping` to `metric`

2 Change `attribute` to `metricAttribute`

3 Changing `mapping` from a map to a list

4 Change `attribute` from a map to a list

trask commented Oct 26, 2022 •

edited

Loading

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

edited

Loading