Add TracingTimer #207

checketts · 2017-11-12T02:55:56Z

Alternative solution to #28

This creates a TracingTimer that could be used to automatically generate tracing spans.

Instead of instrumenting at the registry level, I wrap the Timer that is returned by the register call.

checketts · 2017-11-12T02:56:42Z

...spring-legacy/src/samples/java/io/micrometer/spring/samples/components/PersonController.java

-        } catch (InterruptedException e) {
-            e.printStackTrace();
-        }
+        TracingTimer.builder("outerTimer", tracer)


Usage example here

codefromthecrypt · 2017-11-12T03:04:56Z

micrometer-spring-legacy/src/samples/java/io/micrometer/spring/samples/PrometheusSample.java

+
+    @Bean
+    public Tracer tracer() {
+        Tracer tracer = new com.uber.jaeger.Configuration(


why do you need to use uber's tracer.. I thought opentracing has a test variant?

This is in the samples and won't be checked in long term.

It is essentially a spring boot app that I run to play around and explore. Thanks for the point about using the test variant in the unit tests.

I left it in the PR to demonstrate if anyone wanted to check it out and try running it.

codefromthecrypt · 2017-11-12T03:24:08Z

cool. in the future, maybe add a comment for temporary code, so it is obvious what to review

checketts · 2017-11-12T05:41:31Z

Removed temp code. Added in correct unit test

shakuzen · 2017-11-12T11:52:39Z

micrometer-core/src/main/java/io/micrometer/core/instrument/TracingTimer.java

+import java.util.concurrent.TimeUnit;
+import java.util.function.Supplier;
+
+public class TracingTimer implements Timer{


Perhaps OpenTracingTimer is a better name as it is more specific. Users who do tracing and are interested in this concept but aren't using OpenTracing might be confused by the current name.

jkschneider · 2017-11-13T14:09:15Z

micrometer-core/src/main/java/io/micrometer/core/instrument/TracingTimer.java

+        }
+
+        @Override
+        public Timer register(MeterRegistry registry) {


any reason we can't return the more specific type TracerTimer?

Good idea. I had a flashback of Java 4 not supporting covariant return types.

jkschneider · 2017-11-13T14:16:53Z

@adriancole I'm trying to understand a few comments from the earlier incarnation of this PR (#206).

I guess my feedback is this is a good example of why census has tracing apis separate from metrics apis. You don't get into the tagging trouble as you can access propagated tags independently from metrics or tracing. There's also a data model which means you can derive metrics later and forward them to micrometer that way. http://opencensus.io/

I'm comfortable with an optional dependency that provides opentracing support and also provide opencensus support separately -- all in the spirit of there's no need for Micrometer to pick winners. That said, I don't know enough about the tracing world to know: how entrenched is opentracing even if it is objectively lacking in some way? Is long-term support of it worthwhile?

Both opentracing and opencensus are in 0.x releases, implying continued API instability. If we do support one or both, it would have to be clear that it is incubating or experimental support. Spring's BOM-driven model makes it less than obvious how to pick up new dependencies, so even if we do releases of Micrometer between releases of Boot, it will be hard to propagate it. This isn't a "no", just weighs on me a bit.

You don't get into the tagging trouble as you can access propagated tags independently from metrics or tracing.

What is the tagging trouble you are referring to?

jkschneider · 2017-11-13T14:17:47Z

micrometer-core/build.gradle

@@ -36,6 +36,11 @@ dependencies {

    compile 'com.squareup.okhttp3:okhttp:3.9.0',optional

+    //tracing
+    compile 'io.opentracing:opentracing-api:0.30.0', optional


re: @adriancole earlier comment that 0.31.0 has a breaking API change.

I just upgrade to 0.31.0-RC1 and it does break the API I was using. Should I program against the RC? It appears the Scope API is the way forward (and matches the OpenCensus examples I looked at)

I really don't know. Let's hang tight and wait for Adrian's input.

This PR (opentracing/opentracing-java#189) covers the changes. Looks like they will include a backwards compatible shim. So coding against 0.30.0 is probably the right decision. But I agree that @adriancole 's input will be good.

codefromthecrypt · 2017-11-14T00:24:32Z

Main thing is to not compile against a dependency which can cause thrash in core. For example, adding a class compiled against a type that can drift means any release now includes a type that can conflict yet has nothing to do with core. Even optional has this problem as you cannot remove this type from the jar. It is quite unusual to be expected to On 14 Nov 2017 12:55 am, "checketts" <[email protected]> wrote: *@checketts* commented on this pull request. ------------------------------ In micrometer-core/build.gradle <#207 (comment)> :

@@ -36,6 +36,11 @@ dependencies {

compile 'com.squareup.okhttp3:okhttp:3.9.0',optional + //tracing + compile 'io.opentracing:opentracing-api:0.30.0', optional This PR (opentracing/opentracing-java#189 <opentracing/opentracing-java#189>) covers the changes. Looks like they will include a backwards compatible shim. So coding against 0.30.0 is probably the right decision. But I agree that @adriancole <https://github.com/adriancole> 's input will be good. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#207 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD618-H6qLfPx4igX29wy83MOjtOMUfks5s2HScgaJpZM4QauQZ> .

codefromthecrypt · 2017-11-14T00:26:28Z

Wrt tags the opentracing api forces many things through the tracer api. For example to access propagated tags in census you do not need to have a reference to a span model. In opentracing the span is a heavy reference and much bigger api than needed just to access tags. On 13 Nov 2017 10:16 pm, "Jon Schneider" <[email protected]> wrote: @adriancole <https://github.com/adriancole> I'm trying to understand a few comments from the earlier incarnation of this PR (#206 <#206>). I guess my feedback is this is a good example of why census has tracing apis separate from metrics apis. You don't get into the tagging trouble as you can access propagated tags independently from metrics or tracing. There's also a data model which means you can derive metrics later and forward them to micrometer that way. http://opencensus.io/ I'm comfortable with an optional dependency that provides opentracing support *and also* provide opencensus support separately -- all in the spirit of there's no need for Micrometer to pick winners. That said, I don't know enough about the tracing world to know: how entrenched is opentracing even if it is objectively lacking in some way? Is long-term support of it worthwhile? You don't get into the tagging trouble as you can access propagated tags independently from metrics or tracing. What is the tagging trouble you are referring to? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#207 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD61_ZrOovCXZsM7Vbe-pN0jDXQaUYMks5s2E9VgaJpZM4QauQZ> .

codefromthecrypt

Seems very odd to me that this code isn't in a contrib module until such time as the api is stable. We are basically being asked to trust a stability statement of an api with a very poor compatibility track record and no representation from micrometer.

Feel free to merge this anyway, but understand that this is adding a compile step and drift, and as soon as a change is made you'll have to do a release in micrometer.

Very odd

jkschneider · 2017-11-14T01:09:33Z

Seems very odd to me that this code isn't in a contrib module

I think this should be in a contrib module. Probably in a separate github repo in fact.

I want to avoid the explosion of dependencies that something like Prometheus has as I believe there is a discoverability problem inherent in that. So I'm comfortable having optional deps on libraries where drift is unlikely (e.g. Guava cache). But a pre-1.0 library with demonstrated churn is not a good fit for core.

shakuzen · 2017-11-14T01:12:49Z

Has it been considered to ask the OpenTracing Java maintainers if they'd be interested in hosting this code in their library or org? It would seem easier for them to maintain it and keep it in sync with their API, as the assumption is that micrometer's API is going to be more stable.

jkschneider · 2017-11-14T01:14:27Z

Good point @shakuzen. We'll be committed to 3 year support of the 1.0 line.

checketts · 2017-11-14T01:54:34Z

I'm totally fine with this code being hosted elsewhere. The one finding I wanted to expose pre-1.0 is changing the visibility of the Timer.Builder so it can be sub-classes/extended as in this usecase, allowing extensions like this to be possible.

If that isn't desire-able, then I could investigate based on that feedback.

I'm going to refactor this into a separate package to see if there are other visibility issues that need resolution. (Or move it to a contrib module if that exists)

codefromthecrypt · 2017-11-14T02:51:21Z

I'm sure someone will approve creating a repo here btw https://github.com/opentracing-contrib/ thanks for considering this pov, almost all dependencies can bite you eventually. Ex even jsr305 can bite you in java 9!

checketts · 2017-12-01T19:59:02Z

micrometer-core/src/main/java/io/micrometer/core/instrument/Timer.java

@@ -143,7 +143,7 @@ static Builder builder(String name) {
        private String description;
        private final HistogramConfig.Builder histogramConfigBuilder;

-        private Builder(String name) {
+        protected Builder(String name) {


@jkschneider This single line change is the only really 'required' change in Micrometer that will allow creating the other Tracing timer classes in other repos. I could remove the rest of the PR if needed.

I would love to leave the Opentracing/OpenCensus implementations in Micrometer as Optional deps. I did research and the breaking API change that Adrian brought up will be properly shimmed, and should actually behave as a deprecation for a good while.

Alternatively I could add an abstraction so the Tracer is an interface that could be implemented with a few lines of code that is added to a typical timer.

jkschneider · 2018-09-05T21:09:29Z

From my understanding, we are thinking that this pursuit should be external to micrometer core?

checketts · 2018-09-05T21:50:13Z

Yes. This PR was just exploring to ensure the underlying timer was extensible enough to allow the user to extend it. If we could make the builder protected then that would allow a user to subclass Timer and make their own TracingTimer. I think closing this for now is fine.

checketts commented Nov 12, 2017

View reviewed changes

codefromthecrypt reviewed Nov 12, 2017

View reviewed changes

checketts force-pushed the tracing-timer branch 2 times, most recently from 2499686 to 81f39a2 Compare November 12, 2017 05:40

checketts force-pushed the tracing-timer branch from 81f39a2 to 059debe Compare November 12, 2017 06:24

shakuzen reviewed Nov 12, 2017

View reviewed changes

jkschneider reviewed Nov 13, 2017

View reviewed changes

codefromthecrypt reviewed Nov 14, 2017

View reviewed changes

Create TracingTimer that leverages OpenTracing

3bd4465

checketts force-pushed the tracing-timer branch from 059debe to aec0062 Compare December 1, 2017 18:55

Add opencensus tracing

91b3a6b

checketts force-pushed the tracing-timer branch from aec0062 to 91b3a6b Compare December 1, 2017 19:55

checketts commented Dec 1, 2017

View reviewed changes

jkschneider added this to the 1.1.0 milestone Feb 15, 2018

jkschneider force-pushed the master branch from ab3e6aa to da58c59 Compare March 8, 2018 02:42

checketts mentioned this pull request Apr 20, 2018

Support interval counters #32

Closed

jkschneider closed this Sep 5, 2018

jkschneider removed this from the 1.1.0-rc.1 milestone Oct 29, 2018

jkschneider deleted the tracing-timer branch January 7, 2019 15:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TracingTimer #207

Add TracingTimer #207

checketts commented Nov 12, 2017

checketts Nov 12, 2017

codefromthecrypt Nov 12, 2017

checketts Nov 12, 2017

codefromthecrypt commented Nov 12, 2017 via email

checketts commented Nov 12, 2017

shakuzen Nov 12, 2017

jkschneider Nov 13, 2017

checketts Nov 13, 2017

jkschneider commented Nov 13, 2017 •

edited

Loading

jkschneider Nov 13, 2017

checketts Nov 13, 2017

jkschneider Nov 13, 2017

checketts Nov 13, 2017

codefromthecrypt commented Nov 14, 2017 via email

codefromthecrypt commented Nov 14, 2017 via email

codefromthecrypt left a comment

jkschneider commented Nov 14, 2017 •

edited

Loading

shakuzen commented Nov 14, 2017

jkschneider commented Nov 14, 2017 •

edited

Loading

checketts commented Nov 14, 2017

codefromthecrypt commented Nov 14, 2017 via email

checketts Dec 1, 2017 •

edited

Loading

jkschneider commented Sep 5, 2018

checketts commented Sep 5, 2018

Add TracingTimer #207

Add TracingTimer #207

Conversation

checketts commented Nov 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Nov 12, 2017 via email

checketts commented Nov 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkschneider commented Nov 13, 2017 • 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

codefromthecrypt commented Nov 14, 2017 via email

codefromthecrypt commented Nov 14, 2017 via email

codefromthecrypt left a comment

Choose a reason for hiding this comment

jkschneider commented Nov 14, 2017 • edited Loading

shakuzen commented Nov 14, 2017

jkschneider commented Nov 14, 2017 • edited Loading

checketts commented Nov 14, 2017

codefromthecrypt commented Nov 14, 2017 via email

checketts Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

jkschneider commented Sep 5, 2018

checketts commented Sep 5, 2018

jkschneider commented Nov 13, 2017 •

edited

Loading

jkschneider commented Nov 14, 2017 •

edited

Loading

jkschneider commented Nov 14, 2017 •

edited

Loading

checketts Dec 1, 2017 •

edited

Loading