Micrometer bridge instrumentation #4919
Conversation
...in/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryCounter.java
Show resolved
Hide resolved
| if (isUsingMicrometerHistograms()) { | ||
| measurements = new MicrometerHistogramMeasurements(clock, distributionStatisticConfig); |
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
...gent/src/test/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/TimerTest.java
Show resolved
Hide resolved
mateuszrzeszutek
requested review from
anuraaga,
iNikem,
jkwatson,
laurit,
pavolloffay,
trask and
tylerbenson
as code owners
|
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
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,21 @@ | |||
| plugins { | |||
| id("otel.javaagent-instrumentation") | |||
There was a problem hiding this comment.
Does this make sense as library instrumentation as well?
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
...main/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryGauge.java
Outdated
Show resolved
Hide resolved
...in/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryCounter.java
Outdated
Show resolved
Hide resolved
...main/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryTimer.java
Outdated
Show resolved
Hide resolved
...main/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryTimer.java
Outdated
Show resolved
Hide resolved
...main/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryTimer.java
Outdated
Show resolved
Hide resolved
| import io.opentelemetry.api.common.AttributesBuilder; | ||
| import io.opentelemetry.instrumentation.api.cache.Cache; | ||
|
|
||
| final class Bridging { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I don't feel strongly. If there's precedent for "Bridging", let's keep it.
| removed = true; | ||
| } | ||
|
|
||
| private interface Measurements { |
There was a problem hiding this comment.
I struggled to understand the purpose of this interface based on its name alone. Is there something that better captures the abstraction it represents?
There was a problem hiding this comment.
It quite literally represents the Measurements made by the timer, just grouped together. Do you have any suggestion on how to name this?
...in/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryCounter.java
Show resolved
Hide resolved
...in/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryCounter.java
Show resolved
Hide resolved
...main/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryTimer.java
Outdated
Show resolved
Hide resolved
...gent/src/test/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/GaugeTest.java
Show resolved
Hide resolved
...in/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryCounter.java
Show resolved
Hide resolved
...java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/AsyncInstrumentRegistry.java
Outdated
Show resolved
Hide resolved
...gent/src/test/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/GaugeTest.java
Outdated
Show resolved
Hide resolved
...agent/src/main/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/Bridging.java
Outdated
Show resolved
Hide resolved
...main/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/OpenTelemetryTimer.java
Outdated
Show resolved
Hide resolved
...n/java/io/opentelemetry/javaagent/instrumentation/micrometer/v1_5/UnsupportedReadLogger.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| dependencies { | ||
| library("io.micrometer:micrometer-core:1.5.0") |
There was a problem hiding this comment.
This is a pretty old version, it is not supported anymore, I would go with 1.8.x (latest).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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