Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Micrometer bridge instrumentation #4919

Merged
merged 10 commits into from
Jan 3, 2022

Conversation

mateuszrzeszutek
Copy link
Member

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

Comment on lines +52 to +53
if (isUsingMicrometerHistograms()) {
measurements = new MicrometerHistogramMeasurements(clock, distributionStatisticConfig);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mateuszrzeszutek
Copy link
Member Author

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this make sense as library instrumentation as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+100

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

import io.opentelemetry.api.common.AttributesBuilder;
import io.opentelemetry.instrumentation.api.cache.Cache;

final class Bridging {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BridgingUtil?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

removed = true;
}

private interface Measurements {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't think of a good one.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

}

dependencies {
library("io.micrometer:micrometer-core:1.5.0")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mateuszrzeszutek mateuszrzeszutek merged commit a022f0c into open-telemetry:main Jan 3, 2022
@mateuszrzeszutek mateuszrzeszutek deleted the micrometer branch January 3, 2022 12:33
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants