Bring in micrometer shim #4258

anuraaga · 2022-03-10T09:16:09Z

anuraaga

Comments below for the only two locations business logic changed. Tests were modified fairly significantly though should still be testing the same cases

anuraaga · 2022-03-10T09:16:53Z

micrometer-shim/build.gradle.kts

+otelJava.moduleName.set("io.opentelemetry.micrometershim")
+
+dependencies {
+  api("io.micrometer:micrometer-core")


In instrumentation repo this is compileOnly but here went with api for now to match pattern in opentracing shim but may change it.

anuraaga · 2022-03-10T09:18:02Z

micrometer-shim/src/main/java/io/opentelemetry/micrometershim/Bridging.java

+
+final class Bridging {
+
+  static Attributes tagsAsAttributes(Meter.Id id, NamingConvention namingConvention) {


Removed caches from this class since we don't (yet) have a concurrent cache in this repo

I've always wondered whether we should cache AttributesBuilder.put(String key invocations in the API itself. I don't think there's anything unique to micrometer to do it specifically here

Implement metric identity specification #4222 will land soon to remove need for description cache

I've always wondered whether we should cache AttributesBuilder.put(String key invocations in the API itself. I don't think there's anything unique to micrometer to do it specifically here

That would be awesome, I think - we've been recommending using AttributeKey everywhere to avoid additional allocations, but if we could fix that at the source that would be great.

I'm a bit hesitant, only because we'd need to be careful about how much we cache... we wouldn't want a pathological API usage to OOM us.

anuraaga · 2022-03-10T09:18:39Z

...er-shim/src/main/java/io/opentelemetry/micrometershim/OpenTelemetryMeterRegistryBuilder.java

+  private final OpenTelemetry openTelemetry;
+  private Clock clock = Clock.SYSTEM;
+  private TimeUnit baseTimeUnit = TimeUnit.MILLISECONDS;
+  private NamingConvention namingConvention = NamingConvention.identity;


No config property read here like in instrumentation repo

codecov · 2022-03-10T09:26:12Z

Codecov Report

Merging #4258 (12d7431) into main (85b33d4) will decrease coverage by 0.24%.
The diff coverage is 80.27%.

@@             Coverage Diff              @@
##               main    #4258      +/-   ##
============================================
- Coverage     90.18%   89.93%   -0.25%     
- Complexity     4718     4797      +79     
============================================
  Files           553      568      +15     
  Lines         14544    14909     +365     
  Branches       1399     1414      +15     
============================================
+ Hits          13116    13409     +293     
- Misses          978     1043      +65     
- Partials        450      457       +7

Impacted Files	Coverage Δ
...lemetry/micrometer1shim/UnsupportedReadLogger.java	`0.00% <0.00%> (ø)`
.../opentelemetry/micrometer1shim/TimeUnitHelper.java	`22.22% <22.22%> (ø)`
...ntelemetry/micrometer1shim/OpenTelemetryGauge.java	`63.15% <63.15%> (ø)`
...ry/micrometer1shim/OpenTelemetryFunctionTimer.java	`64.70% <64.70%> (ø)`
.../micrometer1shim/OpenTelemetryFunctionCounter.java	`65.00% <65.00%> (ø)`
...elemetry/micrometer1shim/OpenTelemetryCounter.java	`72.00% <72.00%> (ø)`
...ntelemetry/micrometer1shim/OpenTelemetryMeter.java	`81.81% <81.81%> (ø)`
...rometer1shim/OpenTelemetryDistributionSummary.java	`85.41% <85.41%> (ø)`
...ntelemetry/micrometer1shim/OpenTelemetryTimer.java	`86.53% <86.53%> (ø)`
...ry/micrometer1shim/OpenTelemetryLongTaskTimer.java	`87.50% <87.50%> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85b33d4...12d7431. Read the comment docs.

mateuszrzeszutek · 2022-03-10T10:55:45Z

...er-shim/src/main/java/io/opentelemetry/micrometershim/OpenTelemetryMeterRegistryBuilder.java

+  /** Sets the {@link NamingConvention} for this registry. */
+  public OpenTelemetryMeterRegistryBuilder setNamingConvention(NamingConvention namingConvention) {
+    this.namingConvention = namingConvention;
+    return this;
+  }


NamingConvention can be overriden by using meterRegistry.config().namingConvention(...) - do we need a builder method for that?

I think having common options here is good since it keeps all the configuration in one place and discoverable. Naming convention is quite key

That being said better to start with less than more API, so went back, we'll probably see how important naming convention is when working out the prometheus compatibility mode.

mateuszrzeszutek · 2022-03-10T10:57:06Z

micrometer-shim/src/main/java/io/opentelemetry/micrometershim/UnsupportedReadLogger.java

+final class UnsupportedReadLogger {
+
+  static {
+    Logger logger = Logger.getLogger(OpenTelemetryMeterRegistry.class.getName());


Micrometer has an InternalLogger that can be used instead (it forwards either to slf4j or JUL, depending on the classpath) -- WDYT about using it instead?

Just like we don't want people to use our internal classes I don't want to use others' as well :P This logger class seems fine for OTel users

Hmm, I had the impression that it could be used inside MeterRegistry implementations - but I've double checked it just now, it seems it's only used in micrometer-core itself.
Yeah, let's keep using JUL then.

mateuszrzeszutek · 2022-03-10T10:58:49Z

micrometer-shim/build.gradle.kts

+
+testing {
+  suites {
+    // Test older LTS versions


mateuszrzeszutek · 2022-03-10T11:06:13Z

micrometer-shim/src/main/java/io/opentelemetry/micrometershim/Bridging.java

+
+final class Bridging {
+
+  static Attributes tagsAsAttributes(Meter.Id id, NamingConvention namingConvention) {


I've always wondered whether we should cache AttributesBuilder.put(String key invocations in the API itself. I don't think there's anything unique to micrometer to do it specifically here

That would be awesome, I think - we've been recommending using AttributeKey everywhere to avoid additional allocations, but if we could fix that at the source that would be great.

mateuszrzeszutek · 2022-03-10T17:24:37Z

Now that I think of it, there's going to be Micrometer 2.0 release soon - we'll most likely need another shim for that, I think we can safely assume that there're going to be incompatible changes in there. Should we include the version in the artifact/module name? E.g. micrometer-1-shim

anuraaga · 2022-03-14T23:06:00Z

@jack-berg @jkwatson Would be great if you could give this an ack, thanks!

jkwatson · 2022-03-15T02:43:27Z

Will look tomorrow first thing

jkwatson · 2022-03-15T14:47:07Z

micrometer1-shim/src/main/java/io/opentelemetry/micrometershim/OpenTelemetryCounter.java

+import java.util.Collections;
+import javax.annotation.Nullable;
+
+@SuppressWarnings("HashCodeToString")


what is this warning about?

I think since hashcode is defined but toString isn't. It seems like it'd be worth defining toString but will follow up

jkwatson

Didn't give it anything close to a thorough review, but it's marked as alpha, so I'm not overly concerned.

jack-berg · 2022-03-15T16:49:33Z

micrometer1-shim/build.gradle.kts

+}
+
+description = "OpenTelemetry Micrometer Bridge"
+otelJava.moduleName.set("io.opentelemetry.micrometershim")


The directory name is micrometer1-shim which doesn't align with the module name. Was this intentional?

jack-berg · 2022-03-15T16:51:38Z

micrometer1-shim/build.gradle.kts

+dependencies {
+  api("io.micrometer:micrometer-core")
+
+  implementation(project(":api:all"))


I believe we need an api dependency on :api:all since OpenTelemetry is used in the shim's public API.

…nto micrometer-shim-import

anuraaga added 2 commits March 10, 2022 04:58

Move in micrometer-shim

0c09601

Finish

1c091a2

anuraaga requested a review from mateuszrzeszutek March 10, 2022 09:16

anuraaga requested review from jack-berg, jkwatson and jsuereth as code owners March 10, 2022 09:16

anuraaga requested a review from a user March 10, 2022 09:16

anuraaga requested a review from Oberon00 as a code owner March 10, 2022 09:16

anuraaga commented Mar 10, 2022

View reviewed changes

mateuszrzeszutek approved these changes Mar 10, 2022

View reviewed changes

mateuszrzeszutek mentioned this pull request Mar 10, 2022

Implement "Prometheus mode" for better micrometer->OTel->Prometheus support open-telemetry/opentelemetry-java-instrumentation#5537

Merged

anuraaga added 2 commits March 11, 2022 03:06

micrometer1

9a412a2

Tweak

5419e32

mateuszrzeszutek approved these changes Mar 11, 2022

View reviewed changes

jkwatson reviewed Mar 15, 2022

View reviewed changes

jkwatson approved these changes Mar 15, 2022

View reviewed changes

jack-berg reviewed Mar 15, 2022

View reviewed changes

jack-berg approved these changes Mar 15, 2022

View reviewed changes

anuraaga added 4 commits March 16, 2022 06:01

micrometer1 module

2fa6e57

Fix build file

bc8d97e

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java i…

3ffe252

…nto micrometer-shim-import

Merge

12d7431

anuraaga merged commit 338966e into open-telemetry:main Mar 16, 2022

mateuszrzeszutek mentioned this pull request Mar 21, 2022

Implement "Prometheus mode" for better micrometer->OTel->Prometheus support #4274

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring in micrometer shim #4258

Bring in micrometer shim #4258

anuraaga commented Mar 10, 2022

anuraaga left a comment

anuraaga Mar 10, 2022

anuraaga Mar 10, 2022

mateuszrzeszutek Mar 10, 2022

jkwatson Mar 15, 2022

anuraaga Mar 10, 2022

codecov bot commented Mar 10, 2022 •

edited

Loading

mateuszrzeszutek Mar 10, 2022

anuraaga Mar 10, 2022

anuraaga Mar 11, 2022 •

edited

Loading

mateuszrzeszutek Mar 10, 2022

anuraaga Mar 11, 2022

mateuszrzeszutek Mar 11, 2022

mateuszrzeszutek Mar 10, 2022

mateuszrzeszutek Mar 10, 2022

mateuszrzeszutek commented Mar 10, 2022

anuraaga commented Mar 14, 2022

jkwatson commented Mar 15, 2022

jkwatson Mar 15, 2022

anuraaga Mar 16, 2022

jkwatson left a comment

jack-berg Mar 15, 2022

jack-berg Mar 15, 2022


		final class Bridging {

		static Attributes tagsAsAttributes(Meter.Id id, NamingConvention namingConvention) {

Bring in micrometer shim #4258

Bring in micrometer shim #4258

Conversation

anuraaga commented Mar 10, 2022

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

codecov bot commented Mar 10, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga Mar 11, 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

mateuszrzeszutek commented Mar 10, 2022

anuraaga commented Mar 14, 2022

jkwatson commented Mar 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkwatson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 10, 2022 •

edited

Loading

anuraaga Mar 11, 2022 •

edited

Loading