Micrometer bridge instrumentation by mateuszrzeszutek · Pull Request #4919 · open-telemetry/opentelemetry-java-instrumentation

mateuszrzeszutek · 2021-12-16T16:05:09Z

I started working on upstreaming splunk's micrometer bridge instrumentation, now that metrics API is stable (well, almost stable). This PR contains the first few instruments - and some questions about how we want to handle translating micrometer -> otel calls.

CC @jsuereth @jack-berg

...in/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryCounter.java

mateuszrzeszutek · 2021-12-16T16:10:31Z

...main/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryTimer.java

+    if (isUsingMicrometerHistograms()) {
+      measurements = new MicrometerHistogramMeasurements(clock, distributionStatisticConfig);


Micrometer histograms are configurable via the API, the OTel ones aren't - in this PR I left the possibility to define "micrometer-style" histograms using micrometer settings (this is very similar to how DropwizardMeterRegistry works), completely separately from otel histograms.

...gent/src/test/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/TimerTest.java

mateuszrzeszutek · 2021-12-17T16:18:48Z

I figured this PR is pretty much ready for review -- there are only 3 instruments implemented right now, but all the other ones are derivative and do not introduce any new concepts. And this PR is already kinda big and I don't really want to make it any bigger.

...a/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryMeterRegistry.java

jack-berg · 2021-12-17T19:53:52Z

instrumentation/micrometer/micrometer-1.5/javaagent/build.gradle.kts

@@ -0,0 +1,21 @@
+plugins {
+  id("otel.javaagent-instrumentation")


Does this make sense as library instrumentation as well?

I realized that this would make a very useful library instrumentation some time after opening this PR 😅
I'll split it out in one of the next PRs - once all meters/instruments are implemented.

...a/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryMeterRegistry.java

...main/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryGauge.java

...in/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryCounter.java

...main/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryTimer.java

jack-berg · 2021-12-17T20:37:13Z

...agent/src/main/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/Bridging.java

+import io.opentelemetry.api.common.AttributesBuilder;
+import io.opentelemetry.instrumentation.api.cache.Cache;
+
+final class Bridging {


BridgingUtil?

I don't think that Util adds anything useful to the name; and we already have a couple of *Bridging classes in the codebase. I don't really have a strong opinion though, both names are fine.

I don't feel strongly. If there's precedent for "Bridging", let's keep it.

jack-berg · 2021-12-17T20:40:32Z

...main/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryTimer.java

+    removed = true;
+  }
+
+  private interface Measurements {


I struggled to understand the purpose of this interface based on its name alone. Is there something that better captures the abstraction it represents?

It quite literally represents the Measurements made by the timer, just grouped together. Do you have any suggestion on how to name this?

Can't think of a good one.

...in/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryCounter.java

...main/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryTimer.java

trask

nice!

...gent/src/test/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/GaugeTest.java

...in/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryCounter.java

...java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/AsyncInstrumentRegistry.java

...gent/src/test/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/GaugeTest.java

...agent/src/main/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/Bridging.java

...main/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryTimer.java

...n/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/UnsupportedReadLogger.java

jonatan-ivanov · 2021-12-21T19:25:18Z

instrumentation/micrometer/micrometer-1.5/javaagent/build.gradle.kts

+}
+
+dependencies {
+  library("io.micrometer:micrometer-core:1.5.0")


This is a pretty old version, it is not supported anymore, I would go with 1.8.x (latest).

This is a compile dependency, users bring in the version that they want. Our standard practice is to target the lowest version we support as the compile dependency, while running tests on that and the latest version

I get that but do you want to support a version that reached its end of life and nobody should use it in prod?
Currently the oldest version that we support is the 1.7.x line which will reach its EOL in May.

We will support all versions from 1.5 upwards, 1.8 included. As Anuraag mentioned, we generally aim to support the lowest version possible - the javaagent is often attached to applications that haven't been updated for some time; and it is supposed to work with no code changes (like upgrading the micrometer dependency version) at all.

* Micrometer bridge instrumentation * gauges with the same name and different attributes * weak ref gauge * one more test * disable by default + muzzle * code review comments * log one-time warning * make AsyncInstrumentRegistry actually thread safe * code review comments * one more minor fix

Micrometer bridge instrumentation

579ee6f

mateuszrzeszutek commented Dec 16, 2021

View reviewed changes

...in/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryCounter.java Show resolved Hide resolved

mateuszrzeszutek commented Dec 16, 2021

View reviewed changes

...gent/src/test/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/TimerTest.java Outdated Show resolved Hide resolved

mateuszrzeszutek commented Dec 16, 2021

View reviewed changes

...gent/src/test/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/TimerTest.java Show resolved Hide resolved

Mateusz Rzeszutek added 2 commits December 17, 2021 12:15

gauges with the same name and different attributes

944b071

weak ref gauge

1f958f9

mateuszrzeszutek mentioned this pull request Dec 17, 2021

Configuring histograms in the API (hints?) open-telemetry/opentelemetry-specification#2229

Closed

one more test

bb6278c

mateuszrzeszutek marked this pull request as ready for review December 17, 2021 16:17

mateuszrzeszutek requested review from anuraaga, iNikem, jkwatson, laurit, pavolloffay, trask and tylerbenson as code owners December 17, 2021 16:17

disable by default + muzzle

8419ea5

jack-berg reviewed Dec 17, 2021

View reviewed changes

jsuereth reviewed Dec 17, 2021

View reviewed changes

trask approved these changes Dec 20, 2021

View reviewed changes

Mateusz Rzeszutek added 2 commits December 20, 2021 10:04

code review comments

4b38619

log one-time warning

7f8bbd3

This was referenced Dec 20, 2021

Allow to unregister/stop/destroy instruments open-telemetry/opentelemetry-specification#2232

Open

Allow to unregister/stop/destroy metric instruments open-telemetry/opentelemetry-java#4013

Closed

make AsyncInstrumentRegistry actually thread safe

1a9fe56

jack-berg approved these changes Dec 20, 2021

View reviewed changes

anuraaga approved these changes Dec 21, 2021

View reviewed changes

code review comments

88457a0

pc-alves mentioned this pull request Dec 21, 2021

Opentelemetry support zalando/riptide#1169

Merged

6 tasks

jonatan-ivanov reviewed Dec 21, 2021

View reviewed changes

one more minor fix

aee1779

mateuszrzeszutek merged commit a022f0c into open-telemetry:main Jan 3, 2022

mateuszrzeszutek deleted the micrometer branch January 3, 2022 12:33

mateuszrzeszutek mentioned this pull request Jan 3, 2022

Micrometer instrumentation, part 2 #5001

Merged

marcalff mentioned this pull request Jun 8, 2023

[API] Remove Meters open-telemetry/opentelemetry-cpp#2184

Closed

jonatan-ivanov mentioned this pull request Jun 15, 2023

REQUEST: New membership for jonatan-ivanov open-telemetry/community#1548

Closed

6 tasks

		if (isUsingMicrometerHistograms()) {
		measurements = new MicrometerHistogramMeasurements(clock, distributionStatisticConfig);

		@@ -0,0 +1,21 @@
		plugins {
		id("otel.javaagent-instrumentation")

Conversation

mateuszrzeszutek commented Dec 16, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mateuszrzeszutek commented Dec 17, 2021

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

trask left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants