Provide spring autoconfiguration for the API extensions tracer #11

objectiser · 2017-08-08T14:12:43Z

This PR is moving the API extensions tracer auto-configure code from opentracing-contrib/java-spring-cloud#15.

objectiser · 2017-08-08T14:13:51Z

@jpkrohling can you review aswell please?

sjoerdtalsma

Shouldn't 'spring' be included in the autoconfigure module name somewhere?

sjoerdtalsma · 2017-08-08T14:45:23Z

...e/src/main/java/io/opentracing/contrib/api/tracer/autoconfigure/TracerBeanPostProcessor.java

+                }
+            }
+            if (observerFound) {
+                log.info("Use extensions API tracer with observers=" + tracerObservers);


Maybe log original tracer somewhere in that message as well, as it might be the last chance to do so 😉 ?

BTW: why is commons logging used? Is it an integral dependency already available from Spring?

Its the preferred logger for spring (boot).

Regarding 'spring' in the name - possibly but just made it longer - not sure.

pavolloffay · 2017-08-08T14:55:20Z

I don't know whether this is the right place for this.

I am still wondering why we cannot avoid this and rather make bindings to GlobalTracer in tracerresolver impls
opentracing-contrib/java-tracerresolver#15 (comment)

jpkrohling · 2017-08-08T15:00:35Z

...e/src/main/java/io/opentracing/contrib/api/tracer/autoconfigure/TracerBeanPostProcessor.java

+            }
+            if (observerFound) {
+                log.info("Use extensions API tracer with observers=" + tracerObservers);
+                return tracer;


There's a chance that this tracer is different than the one currently available via GlobalTracer.get(), right? Is that on purpose?

Its possible - but I think we need to separate the stages - tracer resolution using whatever mechanism is appropriate for the runtime (so Bean in spring), and the next step is to set that in the GlobalTracer for global access.

@objectiser I think @pavolloffay has a point in case there's a pre-registered GlobalTracer.
I agree with him that that should always take precedence in this case.
I added PR for review: opentracing-contrib/java-tracerresolver#16

As it happens I also agree with you that registering the GlobalTracer should happen after the resolving (and potential wrapping).

objectiser · 2017-08-08T15:20:02Z

@pavolloffay Not sure if it is the tracerresolver's responsibility to set the GlobalTracer - as mentioned in response to Juca, I think we need to separate the tracer resolution step (TracerResolver, spring Bean, CDI producer, etc) from setting the GlobalTracer. So in spring the TracerResolver is optional - with this PR, even if the user provides their own Bean resolution for Tracer, it will be wrapped. Some other component could then consume the resolved Bean and set the GlobalTracer?

sjoerdtalsma · 2017-08-08T15:48:13Z

@objectiser If I understand this PR correctly, it will automatically postprocess any declared Tracer (spring-) Bean by wrapping the API extensions when necessary?
In this case I like the auto-config part.

Maybe don't try to auto-resolve or auto-register Tracer beans for now (we're using Spring for a reason and there may be specific auto-config jars around for Tracer implementations in the future).

What I'd love to see is an additional case of non-wrapping for native support of the API extensions by Tracer implementations (see my comment here: #5 (comment))

objectiser · 2017-08-08T16:06:05Z

@sjoerdtalsma This PR is only decorating a Tracer bean resolved using some other mechanism - e.g. opentracing-contrib/java-spring-web#29 has just been merged, which will now by default use the GlobalTracer if no other Tracer bean has been explicitly provided. I'll have a think about your other comment.

… added 'spring' to autoconfigure artifact id and package

objectiser · 2017-08-10T10:23:09Z

Applied updates based on current comments.

As discussed in opentracing-contrib/java-spring-cloud#29 the spring context's Tracer bean will either be provided directly by the app (taking precedence) or be resolved via an existing GlobalTracer registered instance or TracerResolver. However the Tracer bean is resolved, this PR would result in it being wrapped (if observers have been defined). So this PR isn't directly affected by the approach used to resolve the Tracer.

pavolloffay · 2017-08-10T10:23:57Z

@objectiser As the outcome of opentracing-contrib/java-spring-cloud#29 it makes sense to include tracerresolver directly in TracerAutoConfiguration and do not have a separate artifact for it.

objectiser · 2017-08-10T10:25:03Z

@pavolloffay That sounds fine. But as explained that is not relevant in this PR - so is this PR ok as is?

pavolloffay · 2017-08-10T10:32:54Z

oh sorry, I though this is auto-config for tracerresolver, my fault. I will re-review.

sjoerdtalsma

I like it; a nice way to add the API extensions in spring by adding a simple dependency.

pavolloffay

LGTM, just minor comments

pavolloffay · 2017-08-10T11:44:37Z

pom.xml

    <version.io.opentracing>0.30.0</version.io.opentracing>
    <version.junit>4.12</version.junit>
    <version.org.mockito-mockito-all>1.10.19</version.org.mockito-mockito-all>
+    <version.org.springframework.boot>1.4.3.RELEASE</version.org.springframework.boot>


minor thing could we use 1.4.1?

pavolloffay · 2017-08-10T11:45:03Z

pom.xml

  </modules>

  <properties>
    <maven.compiler.source>1.7</maven.compiler.source>
    <maven.compiler.target>1.7</maven.compiler.target>
    <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>

+    <version.commons-logging>1.2</version.commons-logging>


Shouldn't this be consumed from spring?

pavolloffay · 2017-08-10T11:46:09Z

...ain/java/io/opentracing/contrib/api/tracer/spring/autoconfigure/TracerBeanPostProcessor.java

+            boolean observerFound = false;
+            APIExtensionsTracer tracer = new APIExtensionsTracer((Tracer)bean);
+            for (TracerObserver observer : tracerObservers) {
+                if (observer != null) {


IIRC spring does not autowire null so this might be not necessary.

If the produces @bean method returns a null, then it does get included in the set unfortunately. I did have a look for a solution, but couldn't find one. If one is found later then it can be updated in a separate PR.

It seems that it works when autowire uses required = false. https://stackoverflow.com/a/26483356/4158442

I was wondering because some time ago I wanted to create a null bean a spring was complaining...

pavolloffay · 2017-08-10T11:49:34Z

...ain/java/io/opentracing/contrib/api/tracer/spring/autoconfigure/TracerBeanPostProcessor.java

+                }
+            }
+            if (observerFound) {
+                log.info("Use extensions API tracer to wrap tracer=" + bean + " with observers=" + tracerObservers);


nit: I don't know how toString on this set is defined, maybe just add a log statement on each observer.

It currently just outputs a comma separate list of the toString for each observer - so depends whether the observer reports something meaningful - but it would have the same situation if having separate log statements, so not sure it would be a benefit.

pavolloffay · 2017-08-10T11:52:03Z

opentracing-api-extensions-tracer-spring-autoconfigure/pom.xml

+    </dependency>
+
+    <dependency>
+      <groupId>commons-logging</groupId>


nit: I think this should be consumed from spring. Maybe just import this from spring bom.

pavolloffay · 2017-08-10T11:58:32Z

@jpkrohling regarding yesterday's discussion. if we were using only GlobalTracer(not spring context) to propagate tracer across different auto-configs (e.g. feign, jms, web...). There wouldn't we a way to implement this. Or I am not aware of any other way.

…t 1.4.1.RELEASE

objectiser · 2017-08-10T14:54:55Z

@pavolloffay @sjoerdtalsma @jpkrohling Thanks!

Provide spring autoconfiguration for the API extensions tracer

2da06d1

objectiser requested a review from pavolloffay August 8, 2017 14:13

objectiser mentioned this pull request Aug 8, 2017

WIP: Auto-config for Extensions API tracer wrapper, metrics (with promethe… opentracing-contrib/java-spring-cloud#15

Closed

sjoerdtalsma reviewed Aug 8, 2017

View reviewed changes

jpkrohling reviewed Aug 8, 2017

View reviewed changes

Updated the log message to include the tracer bean being wrapped, and…

d46f11a

… added 'spring' to autoconfigure artifact id and package

sjoerdtalsma approved these changes Aug 10, 2017

View reviewed changes

pavolloffay approved these changes Aug 10, 2017

View reviewed changes

Remove direct dependency on commons-logging and align with spring boo…

d41742f

…t 1.4.1.RELEASE

objectiser merged commit f55a126 into opentracing-contrib:master Aug 10, 2017

objectiser deleted the spring branch August 10, 2017 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide spring autoconfiguration for the API extensions tracer #11

Provide spring autoconfiguration for the API extensions tracer #11

objectiser commented Aug 8, 2017

objectiser commented Aug 8, 2017

sjoerdtalsma left a comment

sjoerdtalsma Aug 8, 2017

sjoerdtalsma Aug 8, 2017

objectiser Aug 8, 2017

objectiser Aug 8, 2017

pavolloffay commented Aug 8, 2017

jpkrohling Aug 8, 2017

objectiser Aug 8, 2017

sjoerdtalsma Aug 8, 2017

objectiser commented Aug 8, 2017 •

edited

Loading

sjoerdtalsma commented Aug 8, 2017

objectiser commented Aug 8, 2017

objectiser commented Aug 10, 2017

pavolloffay commented Aug 10, 2017

objectiser commented Aug 10, 2017

pavolloffay commented Aug 10, 2017

sjoerdtalsma left a comment

pavolloffay left a comment

pavolloffay Aug 10, 2017

pavolloffay Aug 10, 2017

pavolloffay Aug 10, 2017

objectiser Aug 10, 2017

pavolloffay Aug 10, 2017

pavolloffay Aug 10, 2017

objectiser Aug 10, 2017

pavolloffay Aug 10, 2017

pavolloffay commented Aug 10, 2017

objectiser commented Aug 10, 2017

Provide spring autoconfiguration for the API extensions tracer #11

Provide spring autoconfiguration for the API extensions tracer #11

Conversation

objectiser commented Aug 8, 2017

objectiser commented Aug 8, 2017

sjoerdtalsma 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

pavolloffay commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

objectiser commented Aug 8, 2017 • edited Loading

sjoerdtalsma commented Aug 8, 2017

objectiser commented Aug 8, 2017

objectiser commented Aug 10, 2017

pavolloffay commented Aug 10, 2017

objectiser commented Aug 10, 2017

pavolloffay commented Aug 10, 2017

sjoerdtalsma left a comment

Choose a reason for hiding this comment

pavolloffay 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Aug 10, 2017

objectiser commented Aug 10, 2017

objectiser commented Aug 8, 2017 •

edited

Loading