Use io.opentracing.GlobalTracer instead of singleton in Configuration by vprithvi · Pull Request #255 · jaegertracing/jaeger-client-java

vprithvi · 2017-10-06T21:10:40Z

Register the tracer created by com.uber.jaeger.Configuration#getTracer
with opentracing's GlobalTracer mechanism so that opentracing instrumentations
may find it.
Disable the lazily loaded singleton field by default.

This is a prerequisite for #257

- Register the tracer created by com.uber.jaeger.Configuration#getTracer with opentracing's GlobalTracer mechanism so that opentracing instrumentations may find it. - Disable the lazily loaded singleton field by default. Signed-off-by: Prithvi Raj <p.r@uber.com>

codecov · 2017-10-06T21:25:57Z

Codecov Report

Merging #255 into master will increase coverage by 0.17%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #255      +/-   ##
============================================
+ Coverage     81.97%   82.15%   +0.17%     
- Complexity      524      528       +4     
============================================
  Files            87       87              
  Lines          2003     2006       +3     
  Branches        237      237              
============================================
+ Hits           1642     1648       +6     
+ Misses          262      259       -3     
  Partials         99       99

Impacted Files	Coverage Δ	Complexity Δ
...e/src/main/java/com/uber/jaeger/Configuration.java	`81.69% <100%> (+1.11%)`	`31 <6> (+2)`	⬆️
...java/com/uber/jaeger/dropwizard/Configuration.java	`37.5% <0%> (-12.5%)`	`2% <0%> (-1%)`
.../uber/jaeger/samplers/RemoteControlledSampler.java	`92.95% <0%> (+1.4%)`	`19% <0%> (+1%)`	⬆️
...com/uber/jaeger/samplers/ProbabilisticSampler.java	`81.81% <0%> (+4.54%)`	`12% <0%> (+1%)`	⬆️
.../com/uber/jaeger/samplers/HttpSamplingManager.java	`100% <0%> (+6.66%)`	`5% <0%> (+1%)`	⬆️

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 c1241e9...bdf915e. Read the comment docs.

yurishkuro · 2017-10-07T03:49:50Z

jaeger-core/src/main/java/com/uber/jaeger/Configuration.java

+    GlobalTracer.register(tracer);
+    log.info("Initialized and registered global tracer={}", tracer);
+    return tracer;
+  }


this seems unnecessary complicated - why not keep the single getTracer method and in the lazy init path (otherwise unchanged) just add if (!disableGlobalTracer) { GlobalTracer.register(tracer);}? Everything else keeps working off the local tracer var.

I wrote it this way because I wanted to remove tracer from jaeger config in a future release, and instead depend on opentracing's GlobalTracer for management of the singleton.

Do you think we need to continue support holding on to a tracer in the config?

The only case where I see this being useful is for advanced users who need multiple tracers in the same application. I expect that such users to use getTracerBuilder and not maintain multiple copies of JaegerConfiguration.

if or when we decide on that separation (I am not convinced of its value), we can do that, but right now I don't see a value in this big change that can be instead achieved with just 3 lines.

yurishkuro · 2017-10-07T03:51:40Z

jaeger-core/src/main/java/com/uber/jaeger/Configuration.java

      String serviceName,
      SamplerConfiguration samplerConfig,
      ReporterConfiguration reporterConfig) {
+    this(serviceName, samplerConfig, reporterConfig, true);


didn't we want to default this to disableGlobalTracer = false?

also, let's stop adding constructors and add a builder, which we should've done in the first place. The new constructor below should be private.

While I agree, I think it should be part of a separate commit

yurishkuro · 2017-10-07T03:53:40Z

jaeger-core/src/main/java/com/uber/jaeger/Configuration.java

        SamplerConfiguration.fromEnv(),
-        ReporterConfiguration.fromEnv());
+        ReporterConfiguration.fromEnv(),
+        Boolean.valueOf(getProperty(JAEGER_DISABLE_GLOBAL_TRACER)));


use getPropertyAsBoolean

yurishkuro · 2017-10-07T03:55:13Z

please link to the relevant issue (iirc this is a pre-requisite)

mabn · 2017-10-07T11:37:56Z

jaeger-core/src/main/java/com/uber/jaeger/Configuration.java

+   * Use {@link GlobalTracer} to store the tracer instead of
+   * storing an instance of the tracer in {@link #tracer}.
+   */
+  private final boolean disableGlobalTracer;


This comment should be negated: "Do not use.."

Signed-off-by: Prithvi Raj <p.r@uber.com>

yurishkuro · 2017-10-07T15:31:56Z

While I agree, I think it should be part of a separate commit

if you want to do a separate pr, it's fine, but right now you're adding a new public constructor that we won't be able to hide later for backwards compatibility.

Signed-off-by: Prithvi Raj <p.r@uber.com>

yurishkuro

lgtm

yurishkuro · 2017-10-07T17:23:50Z

jaeger-core/src/main/java/com/uber/jaeger/Configuration.java

-      return Boolean.valueOf(value);
-    }
-    return null;
+    return Boolean.valueOf(getProperty(name));


You're changing the semantics of the method, previously it could return null, now it performs defaulting of null string to false. It still works given how it's used in this class, but by accident. I would rename it to getPropertyAsBool and return bool, and add a comment about defaulting.

True, I didn't see any cases where it was necessary to differentiate null from false. I'll update as you recommended.

yurishkuro · 2017-10-07T17:25:00Z

jaeger-core/src/test/java/com/uber/jaeger/ConfigurationTest.java


    System.clearProperty(TEST_PROPERTY);
+
+    // Hack to reset opentracing's global tracer


I wouldn't call it a hack

yurishkuro · 2017-10-07T17:26:03Z

jaeger-core/src/test/java/com/uber/jaeger/ConfigurationTest.java

+  @Test
+  public void testDefaultGlobalTracer() {
+    Configuration config = new Configuration("Test");
+    config.getTracer();


maybe assertNotNull? Otherwise it looks weird.

yurishkuro · 2017-10-07T17:28:14Z

jaeger-dropwizard/src/test/java/com/uber/jaeger/dropwizard/JerseyServerFilterTest.java

-    } catch (NoSuchFieldException | IllegalAccessException e) {
-      throw new RuntimeException(e);
-    }
+    Tracer tracer = new Tracer.Builder("world service", reporter, new ConstSampler(true)).build();


nice! don't know what ^^ was about.

pavolloffay · 2017-10-09T11:51:45Z

jaeger-core/src/main/java/com/uber/jaeger/Configuration.java

    tracer = getTracerBuilder().build();
    log.info("Initialized tracer={}", tracer);
+
+    if (!(disableGlobalTracer || GlobalTracer.isRegistered())) {


The same tracer instance can be registered multiple times.

I am wondering whether it would be better to remove GlobalTracer.isRegistered() check and propagate potential issues with multiple registered traces at process start time.

Oh, you mean that if somebody accidentally calls fromEnv() many times, it'll lead to drift between the GlobalTracer and the local tracer.

I agree, I think it is better to error out. I'll update the code.

Signed-off-by: Prithvi Raj <p.r@uber.com>

ghost assigned vprithvi Oct 6, 2017

ghost added the review label Oct 6, 2017

vprithvi requested review from a team, black-adder, pavolloffay and yurishkuro and removed request for yurishkuro October 6, 2017 21:10

vprithvi force-pushed the use-global-tracer branch from fd57f29 to 8104f26 Compare October 6, 2017 21:12

yurishkuro reviewed Oct 7, 2017

View reviewed changes

mabn reviewed Oct 7, 2017

View reviewed changes

Address feedback and fix getPropertyAsBoolean

184b54f

Signed-off-by: Prithvi Raj <p.r@uber.com>

vprithvi added 2 commits October 7, 2017 12:20

Address feedback

660872d

Signed-off-by: Prithvi Raj <p.r@uber.com>

Make constructor private

524c6c7

Signed-off-by: Prithvi Raj <p.r@uber.com>

yurishkuro approved these changes Oct 7, 2017

View reviewed changes

pavolloffay reviewed Oct 9, 2017

View reviewed changes

vprithvi added 2 commits October 9, 2017 10:43

Address feedback

af07f98

Signed-off-by: Prithvi Raj <p.r@uber.com>

Address feedback

bdf915e

Signed-off-by: Prithvi Raj <p.r@uber.com>

vprithvi merged commit a5e2a85 into master Oct 9, 2017

ghost removed the review label Oct 9, 2017

vprithvi deleted the use-global-tracer branch October 9, 2017 15:55

pavolloffay mentioned this pull request May 10, 2019

Automatically generate changelog #629

Closed


		System.clearProperty(TEST_PROPERTY);

		// Hack to reset opentracing's global tracer

Conversation

vprithvi commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yurishkuro commented Oct 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yurishkuro commented Oct 7, 2017

Uh oh!

yurishkuro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vprithvi commented Oct 6, 2017 •

edited

Loading

codecov bot commented Oct 6, 2017 •

edited

Loading