jmx reuse instrumentation metrics #1782
Conversation
| | `activemq` | Apache ActiveMQ | [`activemq.yaml`](src/main/resources/activemq.yaml) | | | ||
| | `cassandra` | Apache Cassandra | [`cassandra.yaml`](src/main/resources/cassandra.yaml) | | | ||
| | `hbase` | Apache HBase | [`hbase.yaml`](src/main/resources/hbase.yaml) | | | ||
| | `hadoop` | Apache Hadoop | [`hadoop.yaml`](src/main/resources/hadoop.yaml) | | | ||
| | `jetty` | Eclipse Jetty | [`jetty.yaml`](src/main/resources/jetty.yaml) | | | ||
| | `jvm` | JVM runtime metrics | [`jvm.yaml`](src/main/resources/jvm.yaml) | | | ||
| | `kafka` | Apache Kafka | [`kafka.yaml`](src/main/resources/kafka.yaml) | | | ||
| | `kafka-consumer` | Apache Kafka consumer | [`kafka-consumer.yaml`](src/main/resources/kafka-consumer.yaml) | | | ||
| | `kafka-producer` | Apache Kafka producer | [`kafka-producer.yaml`](src/main/resources/kafka-producer.yaml) | | | ||
| | `solr` | Apache Solr | [`solr.yaml`](src/main/resources/solr.yaml) | | | ||
| | `tomcat` | Apache Tomcat | [`tomcat.yaml`](src/main/resources/tomcat.yaml) | | | ||
| | `wildfly` | Wildfly | [`wildfly.yaml`](src/main/resources/wildfly.yaml) | | |
There was a problem hiding this comment.
[for reviewer] this table will have to be updated on every instrumentation dependency release if there are new systems or definitions in instrumentation.
| // TODO when JVM metrics will be added to instrumentation, the default "auto" source | ||
| // means that the definitions in instrumentation will be used, and thus this test will fail | ||
| // due to metrics differences, adding an explicit "legacy" source is required to continue testing | ||
| // the JVM metrics defined in this project. | ||
| // https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/13392 | ||
| // .withTargetSystem("legacy") |
There was a problem hiding this comment.
[for reviewer] leaving this is intentional for now as it ensures that the test will fail when the open-telemetry/opentelemetry-java-instrumentation#13392 PR has been merged, released and the dependency has been updated. Because we don't know yet when that will happen I think this is better than to forget about it. Also, this is a good reminder that we need to update the table in README for the supported systems.
| * @return input stream on target system yaml definitions | ||
| * @throws ConfigurationException when no yaml for system is available | ||
| */ | ||
| public InputStream getTargetSystemYaml(String system) { |
There was a problem hiding this comment.
[for reviewer] technically speaking this could be left outside of configuration however we now have to use the presence of yaml files to check if system is supported, which is something that the configuration should check for (this is already tested in tests like JmxScraperConfigTest#shouldFailValidation_invalidTargetSystem).
| // assertThat(config.properties.getProperty("otel.metric.export.interval")).isEqualTo("123"); | ||
| // } | ||
| // | ||
| @ParameterizedTest |
There was a problem hiding this comment.
[for reviewer] we could do the same tests with "real" yaml systems definitions, but this is not possible because we need to cover all the cases where a system is exclusively supported in legacy or instrumentation and when it's in both, in which case the priority will depend on the value of otel.jmx.target.source.
Adds a new configuration for jmx scraper:
otel.jmx.target.sourcethat defines how the values ofotel.jmx.target.systemare resolved to their respective YAML definitions.3 possible values:
auto(default) use instrumentation yaml if available, fallback to embedded if possiblelegacyonly use the embedded yaml (which is very close to JMX gatherer)instrumentationonly use the instrumentation yaml without fallbackThanks @robsunday for the idea of an automatic fallback, that will really help with the migration to having everything defined in instrumentation. In short, the metrics produced by default will be the ones from instrumentation repository (in the "library" part, not the "javaagent" as they currently are), so every release and update of instrumentation dependency will bring an updated set of metrics. For example this will be the case for
jvmsystem that will be added with open-telemetry/opentelemetry-java-instrumentation#13392 pull-request.I have tested it locally with a snapshot build of open-telemetry/opentelemetry-java-instrumentation#13392 to have the JVM metrics included in instrumentation, so as the time of writing this is "works on my computer certified".