Allow subclassing of common classes #509

isaachier · 2018-08-06T16:38:55Z

Give protected access to core Jaeger classes so that users may extend them. The main motive here is to create a bridge between the legacy com.uber.jaeger classes to the io.jaegertracing classes (equivalent to a typedef in C/C++). Note, in general it would not be wise to implement a subclass of these classes because they are internal and may change without notice!

Additionally, implement abstract factory pattern to create instances of spans, span contexts, and span builders. This allows the caller to provide subclasses of the aforementioned classes. For example, when the tracer needs to create a JaegerSpanBuilder object, it invokes createSpanBuilder instead. createSpanBuilder acts as a hook to instantiate a subclass instead of the JaegerSpanBuilder base class.

yurishkuro · 2018-08-06T16:40:56Z

jaeger-core/src/main/java/io/jaegertracing/Configuration.java

@@ -462,7 +461,7 @@ public void apply(JaegerTracer.Builder builder) {
      registerCodec(builder, Format.Builtin.TEXT_MAP);
    }

-    protected void registerCodec(JaegerTracer.Builder builder, Format<TextMap> format) {
+    private void registerCodec(JaegerTracer.Builder builder, Format<TextMap> format) {


Find/replace mistake. Will remove.

codecov · 2018-08-06T16:45:42Z

Codecov Report

Merging #509 into master will increase coverage by 0.15%.
The diff coverage is 96.77%.

@@             Coverage Diff              @@
##             master     #509      +/-   ##
============================================
+ Coverage     88.27%   88.42%   +0.15%     
- Complexity      500      505       +5     
============================================
  Files            65       66       +1     
  Lines          1850     1883      +33     
  Branches        239      239              
============================================
+ Hits           1633     1665      +32     
- Misses          140      142       +2     
+ Partials         77       76       -1

Impacted Files	Coverage Δ	Complexity Δ
...ain/java/io/jaegertracing/internal/JaegerSpan.java	`93.45% <ø> (ø)`	`47 <0> (ø)`	⬇️
...n/java/io/jaegertracing/internal/JaegerTracer.java	`90.38% <100%> (+0.54%)`	`24 <4> (ø)`	⬇️
...io/jaegertracing/internal/JaegerObjectFactory.java	`100% <100%> (ø)`	`4 <4> (?)`
...a/io/jaegertracing/internal/JaegerSpanContext.java	`90.9% <100%> (+0.28%)`	`21 <4> (-1)`	⬇️
.../src/main/java/io/jaegertracing/Configuration.java	`94.6% <100%> (+0.43%)`	`42 <1> (+2)`	⬆️
...egertracing/internal/propagation/TextMapCodec.java	`91.11% <100%> (+0.52%)`	`22 <0> (ø)`	⬇️
...ertracing/internal/propagation/B3TextMapCodec.java	`85% <77.77%> (-1.54%)`	`19 <0> (ø)`

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 4934922...e61b97c. Read the comment docs.

yurishkuro · 2018-08-06T17:34:14Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerSpan.java

@@ -46,7 +46,7 @@
  private List<LogData> logs;
  private boolean finished = false; // to prevent the same span from getting reported multiple times

-  JaegerSpan(
+  protected JaegerSpan(


@isaachier I don't think this will be enough, is it? I.e. you'd also need an overridable method that actually creates the object.

I would prefer if you accompany this with a unit test showing the working subclassing.

I added a lot more. The test definitely helped me determine what was necessary.

yurishkuro

https://www.youtube.com/watch?v=0z_Qqnq8pI8

You have added a ton of irrelevant formatting. Where is it coming from? Don't mix formatting changes with tbhe business logic changes.

isaachier · 2018-08-06T20:59:19Z

Lol I'm not trying to. Travis CI kept failing the build because of the formatting and licensing issues.

yurishkuro · 2018-08-06T21:20:26Z

If you need to fix formatting, then let's do it in a separate PR.

isaachier · 2018-08-07T00:47:45Z

Undid the formatting fix for now.

yurishkuro · 2018-08-07T01:04:45Z

jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSubclassTest.java

+  }
+
+  private static class SpanContext extends JaegerSpanContext {
+    SpanContext(JaegerSpanContext ctx) {


That's not what I thought you were going after. If you extend the context, why do you need to accept it as agrument? The oss tracer currently does "new SpanContext". You can move that call into a function that you would override in the subclassed tracer and return your own subclass of SpanContext.

I could redo a lot of code. The idea is to avoid code duplication by allowing base classes to do 99% of the work. Imagine the case of a JaegerTracer starting a JaegerSpan. There is a lot of logic dedicated to determining the span parent, etc. Now I could override start because Tracer needs to return Span instead of JaegerSpan, but that's going to result in a huge amount of copy-paste. Isn't it easier to use a constructor Span(JaegerSpan span) (note package visibility, not public)?

I am not suggesting redoing any code. What I am saying is that, for example, Trace has this code in builder.createNewContext():

return new JaegerSpanContext(id, id, 0, flags);

createNewContext itself is a large method we don't want to replicate. But we can change the above line to

return instantiateNewSpanContext(id, id, 0, flags);

and add method:

protected JaegerSpanContext instantiateNewSpanContext(...) { return new JaegerSpanContext(id, id, 0, flags); }

Then in your subclassed Tracer you just need to override this method

@Override protected JaegerSpanContext instantiateNewSpanContext(...) { return new SubclassedJaegerSpanContext(id, id, 0, flags); }

Your subclassed tracer will have a total of 3 methods: constructor that directly delegates to super(....same args.....), and two instantiate methods, for span context and the Span that create
subclassed instances.

OK now I understand. I think this is called the abstract factory pattern. That definitely makes sense. I am usually hesitant to change so much of the existing code, but I guess this is a better design.

jpkrohling · 2018-08-07T09:40:28Z

jaeger-core/src/main/java/io/jaegertracing/Configuration.java

@@ -653,19 +652,19 @@ public static SenderConfiguration fromEnv() {
    }
  }

-  private static String stringOrDefault(String value, String defaultValue) {
+  protected static String stringOrDefault(String value, String defaultValue) {


Are you making them protected so that you can use them in subclasses? Those are not part of the business of this class, I'd rather see them in a, say, StringUtils class (as much as a I dislike utils classes).

Honestly, my subclass doesn't use these. I thought if they were static private utility methods here, then they ought to be static protected for subclasses. Nonetheless, I was considering adding a new class called ConfigUtils or something just for that purpose.

jpkrohling · 2018-08-07T09:42:00Z

jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSubclassTest.java

+import java.util.Map;
+
+public class JaegerSubclassTest {
+  private static class Configuration extends io.jaegertracing.Configuration {


s/Configuration/CustomConfiguration/ (or anything else that won't class with i.j.Configuration)

Right good idea.

jpkrohling · 2018-08-07T09:43:22Z

jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSubclassTest.java

+    }
+  }
+
+  private static class Tracer extends JaegerTracer {


s/Tracer/CustomTracer/ (or anything else that won't clash with io.opentracing.Tracer, which is part of the hierarchy)

jpkrohling · 2018-08-07T09:43:50Z

jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSubclassTest.java

+      }
+    }
+
+    Tracer(io.jaegertracing.internal.JaegerTracer tracer) {


You probably don't need the fully qualified class name here, do you?

Good catch.

jpkrohling · 2018-08-07T09:44:31Z

jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSubclassTest.java

+    }
+  }
+
+  private static class Span extends JaegerSpan {


s/Span/CustomSpan

jpkrohling · 2018-08-07T09:49:17Z

jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSubclassTest.java

+    final Configuration config = new Configuration("test-service");
+    final Tracer tracer = config.getTracer();
+    final Scope scope = tracer.buildSpan("test-operation").startActive(true);
+    Assert.assertNotNull(tracer.scopeManager().active());


I understand that if this assertion works, then a bunch of other stuff has also worked, but it would be nice to have your "base" assertions made explicit here.

Ya this test isn't perfect. Needs work.

Should be a bit better now.

yurishkuro · 2018-08-07T21:59:30Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerSpanContext.java

+  }
+
+  protected TracingFactory tracingFactory() {
+    return new TracingFactory();


factory should be instantiated only once

I was thinking about using a singleton. I guess I should.

not a singleton in the sense of a private static member, but just an externally provided object.

yurishkuro · 2018-08-07T22:01:04Z

jaeger-core/src/main/java/io/jaegertracing/Configuration.java

+    return config;
+  }
+
+  protected void initFromEnv() {


why? this is not the code we want to repeat, so why is it factored out into a method?

This is because Configuration has no factory method at the moment. All these static methods are problematic because they construct the concrete type instead of the subclass. In general, I was able to solve by passing or overriding the factory. I can try doing the same here.

just an idea - while subclassing tracer / span is necessary, I don't think the same holds for the configuration class. It's fine to wrap it, and not provide all the functionality that exists in the oss. For example, internally we don't init from env.

Agreed. I reverted the protected methods back to private.

yurishkuro · 2018-08-08T16:06:52Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerSpanContext.java


  public JaegerSpanContext(long traceId, long spanId, long parentId, byte flags) {
    this(traceId, spanId, parentId, flags, Collections.<String, String>emptyMap(), null);
  }

-  JaegerSpanContext(
+  protected JaegerSpanContext(


who would use this ctor?

It isn't exactly popular, but it is the more complete constructor. If the subclass wants to override the other constructor, it would need to use this constructor and pass in a custom factory for the last argument.

Sorry, now that I see the constructor I understand what you mean. Will remove.

yurishkuro · 2018-08-08T16:07:24Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerSpanContext.java

@@ -107,7 +120,11 @@ public String toString() {
    return contextAsString();
  }

-  public static JaegerSpanContext contextFromString(String value)
+  public static JaegerSpanContext contextFromString(String value) {


who uses this?

Well it is public and this isn't the legacy client, so I feel uncomfortable deleting it or adding a required argument. It is used internally to decode a span context from a TextMap.

I would deprecate this method. I think the only reason it is public is because Codecs are in a different package. Since you already have factory.createSpanContext() method, the only thing this is doing is parsing a string representation of the span context - arguably a function that should live in the codecs themselves, since there can be different representations like the upcoming W3C Trace Context spec. So I suggest refactoring this and moving the main logic to TextMapCodec (it can be a public static method there, always taking the factory).

I'll deprecate it and refactor logic in TextMapCodec.

yurishkuro · 2018-08-08T16:08:25Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerTracer.java


-  private JaegerTracer(
+  protected JaegerTracer(


I don't see why you need to duplicate constructors - it was private previously, nobody called it externally, so it should be ok to change its API

Good point. I'll remove this one and any others I find that were originally private or package scope.

yurishkuro · 2018-08-08T17:34:40Z

jaeger-core/src/main/java/io/jaegertracing/Configuration.java

@@ -177,10 +179,14 @@
   */
  private JaegerTracer tracer;

-  public Configuration(String serviceName) {
+  public Configuration(String serviceName, TracingFactory tracingFactory) {


did you see my comment? #509 (comment) ?

I did. I'll comment more there. Need the tracing factory to construct the JaegerTracer.Builder subclass used in the config to create the tracer.

yurishkuro · 2018-08-08T17:37:29Z

jaeger-core/src/main/java/io/jaegertracing/Configuration.java

+        withTracerTags(tracerTagsFromEnv())
+        .withReporter(ReporterConfiguration.fromEnv())
+        .withSampler(SamplerConfiguration.fromEnv())
+        .withCodec(CodecConfiguration.fromEnv());


doesn't look like there's a change here, please undo

yurishkuro · 2018-08-08T17:39:04Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerSpanContext.java

-        new BigInteger(parts[3], 16).byteValue());
+        new BigInteger(parts[3], 16).byteValue(),
+        Collections.<String, String>emptyMap(),
+        null);


what is null? should it be factory?

This is actually a reference to debugId. But this should definitely take a TracingFactory argument as well. Will fix.

Never mind. The factory passes this internally to the JaegerSpanContext constructor.

yurishkuro · 2018-08-08T17:40:51Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerSpanContext.java

@@ -166,14 +175,19 @@ boolean isDebugIdContainerOnly() {
   * debug trace, and the value of header recorded as a span tag to serve as a searchable
   * correlation ID.
   *
-   * @param debugId arbitrary string used as correlation ID
+   * @param debugId arbitrary string used as correlation ID.
+   * @param factory tracing object factory.


The name TracingFactory was making me uncomfortable, I was thinking ObjectFactory, but it too was too generic. I think TracingObjectFactory is much cleaner name

OK I struggled with the names. JaegerFactory or JaegerObjectFactory could work too.

+1 JaegerObjectFactory

yurishkuro · 2018-08-08T17:42:25Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerTracer.java

@@ -95,8 +99,10 @@ private JaegerTracer(
    this.metrics = metrics;
    this.zipkinSharedRpcSpan = zipkinSharedRpcSpan;
    this.scopeManager = scopeManager;
+    this.baggageRestrictionManager = baggageRestrictionManager;


so, like, a bug? 👍

No actually just wasn't needed outside of constructor. Originally needed this when I had constructor CustomTracer(JaegerTracer tracer), but don't need this anymore. Will remove.

yurishkuro · 2018-08-08T17:44:22Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerTracer.java

@@ -315,7 +321,7 @@ private JaegerSpanContext createNewContext(String debugId) {
        }
      }

-      return new JaegerSpanContext(id, id, 0, flags);
+      return JaegerTracer.this.tracingFactory.createSpanContext(id, id, 0, flags, Collections.<String, String>emptyMap(), debugId);


I am seeing a mix of factory and tracingFactory. If you agree with my above comment to call it TracingObjectFactory, then I'd suggest field name objectFactory, to be used consistently

I thought I renamed all factory references to tracingFactory. Will change to objectFactory throughout.

yurishkuro · 2018-08-08T17:45:06Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerTracer.java

@@ -354,7 +360,7 @@ private JaegerSpanContext createChildContext() {
        }
      }

-      return new JaegerSpanContext(
+      return JaegerTracer.this.tracingFactory.createSpanContext(


just curious, why fully qualified accessor instead of jus tracingFactory.createSpanContext ? I think the latter is also visible.

JaegerTracer.SpanBuilder is a non-static nested class. I am able to access the tracer's tracingFactory from the inner class. Personally, not a huge fan of non-static nested classes (prefer explicitly passing instance to static nested class instead).

yurishkuro · 2018-08-08T17:46:42Z

jaeger-core/src/main/java/io/jaegertracing/internal/TracingFactory.java

+import java.util.List;
+import java.util.Map;
+
+public class TracingFactory {


add javadoc

Ya. Just need to stabilize the API, then will do.

yurishkuro · 2018-08-08T17:51:54Z

jaeger-core/src/main/java/io/jaegertracing/internal/TracingFactory.java

+      ScopeManager scopeManager,
+      BaggageRestrictionManager baggageRestrictionManager,
+      boolean expandExceptionLogs) {
+    return new JaegerTracer(


per my comment on Configuration, I don't think this method needs to be in the factory. Here's what you can do instead, to reduce the surface area:

class Configuration { public JaegerTracer.Builder getTracerBuilder() { - JaegerTracer.Builder builder = tracingFactory.createTracerBuilder(serviceName) + JaegerTracer.Builder builder = this.createTracerBuilder(serviceName) } protected createTracerBuilder() // overridable method, no more dependency on the factory }

similar thing with JaegerTracer.Builder - just implement a protected method to create the tracer.

The only time where you need the abstract factory pattern is for creating SpanBuilder/Span/SpanContext, because those are done all the time from many places. But Tracer.Builder and Tracer are only created once, there's no need to complicate that with abstract factory pattern.

I thought about doing that, but sort of wanted to do everything in factory or everything as subclass methods. This makes sense as a compromise.

yurishkuro · 2018-08-08T23:26:39Z

jaeger-core/src/main/java/io/jaegertracing/Configuration.java

@@ -15,6 +15,7 @@
 package io.jaegertracing;

 import io.jaegertracing.internal.JaegerTracer;
+import io.jaegertracing.internal.JaegerObjectFactory;


shouldn't need this

Ya I use the Google formatting plugin that will adjust the import and formatting at once, so I held off to avoid the formatting issues from earlier.

yurishkuro · 2018-08-08T23:27:29Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerObjectFactory.java

+        references);
+  }
+
+  public JaegerSpanContext createSpanContext(long traceId,


formatting inconsistent with the previous method

Ya will fix formatting after main code is good to go.

yurishkuro · 2018-08-08T23:32:09Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerSpanContext.java

  public static JaegerSpanContext withDebugId(String debugId) {
-    return new JaegerSpanContext(0, 0, 0, (byte) 0, Collections.<String, String>emptyMap(), debugId);
+    return withDebugId(debugId, new JaegerObjectFactory());


pretty sure we can drop this method - it's internal package after all

Good idea. If we can use that logic, that would simplify things in a few places.

yurishkuro · 2018-08-08T23:32:57Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerTracer.java

@@ -95,8 +99,10 @@ private JaegerTracer(
    this.metrics = metrics;
    this.zipkinSharedRpcSpan = zipkinSharedRpcSpan;
    this.scopeManager = scopeManager;
+    this.baggageRestrictionManager = baggageRestrictionManager;


yurishkuro · 2018-08-08T23:33:46Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerSpanContext.java

-        new BigInteger(parts[1], 16).longValue(),
-        new BigInteger(parts[2], 16).longValue(),
-        new BigInteger(parts[3], 16).byteValue());
+  @Deprecated


since this is internal package, can probably just remove this altogether

yurishkuro · 2018-08-08T23:47:04Z

jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSubclassTest.java

+    public CustomSpan(
+        JaegerTracer tracer,
+        String operationName,
+        JaegerSpanContext context,


nit: this should be CustomSpanContext

yurishkuro · 2018-08-08T23:47:27Z

jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSubclassTest.java

+        byte flags,
+        Map<String, String> baggage,
+        String debugId,
+        JaegerObjectFactory objectFactory) {


CustomObjectFactory

yurishkuro · 2018-08-08T23:48:00Z

jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSubclassTest.java

+    }
+
+    @Override
+    public CustomSpanContext withFlags(byte flags) {


why do you need to override these?

Sadly the covariant return type adds a lot of boilerplate where I have to cast things.

For a practical example, here is what happens if I don't override them:

CustomSpan customSpan = anotherCustomSpan.withFlags(flags); // Error: cannot assign JaegerSpan to CustomSpan.

It will probably break user code without overriding.

Are you worried that someone would write

com.uber.jaeger.SpanContext _ = spanCtx.withFlags()

? I highly doubt it, but ok, we can leave these as a form of example.

For SpanContext, I found 19 cases in internal Uber code. For Span I found 281.

referring to the class, yes, but I doubt they use these methods. But ok if you want to keep them.

I'd just be skeptical about adding them to the actual subclass (not this test)

OK that helps me. Just a matter of seeing what code changes would be necessary.

yurishkuro · 2018-08-08T23:49:54Z

jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSubclassTest.java

+  }
+
+  private static class CustomSpanContext extends JaegerSpanContext {
+    public CustomSpanContext(


could it be private? go for minimum visibility

I think so. Didn't want to change visibility from base class. Constructors should be okay with that.

yurishkuro · 2018-08-08T23:51:33Z

jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSubclassTest.java

+
+    @Override
+    public CustomTracer.CustomSpanBuilder createSpanBuilder(JaegerTracer tracer, String operationName) {
+      return ((CustomTracer)tracer).new CustomSpanBuilder(operationName);


Do you actually need to override span builder? Wouldn't the default one do the trick?

I wish... I have not found a way to do it in Java. Maybe by somehow (ab)using a generic helper class to generate bridge methods that are JVM generated casts.

yurishkuro · 2018-08-08T23:52:22Z

looking much better!

pavolloffay · 2018-08-09T14:37:31Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerObjectFactory.java

+import java.util.List;
+import java.util.Map;
+
+public class JaegerObjectFactory {


What is a purpose of this class? Can you please document it here?

I will very soon.

yurishkuro · 2018-08-14T16:54:04Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerSpanContext.java

@@ -107,44 +114,22 @@ public String toString() {
    return contextAsString();
  }

-  public static JaegerSpanContext contextFromString(String value)


why is this still here? Didn't you merge it as a separate PR already? Maybe rebase from master?

No just haven't gotten to fixing it yet. Sorry.

yurishkuro · 2018-08-14T16:58:00Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerTracer.java


    this.version = loadVersion();

-    tags.put(Constants.JAEGER_CLIENT_VERSION_TAG_KEY, this.version);
-    if (tags.get(Constants.TRACER_HOSTNAME_TAG_KEY) == null) {
+    builder.tags.put(Constants.JAEGER_CLIENT_VERSION_TAG_KEY, this.version);


nit: instead of modifying builder.tags I would create a local variable as a copy

Map<String, String> tags = new HashMap<String, String>(builder.tags);

yurishkuro · 2018-08-14T17:00:08Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerTracer.java

-              computeDurationViaNanoTicks,
-              tags,
-              references);
+      JaegerSpan jaegerSpan =


Just curious, why not JaegerSpan jaegerSpan = getObjectFactory().createSpan( on one line?
Would be less code changes overall.

This was auto-formatted. I will try your style and see what the auto-formatter does with it.

Didn't let me use this format. Changed it for me.

What do you mean? Last I recall formatting isn't changed as part of the build.

yurishkuro · 2018-08-14T17:02:13Z

jaeger-core/src/main/java/io/jaegertracing/internal/PropagationRegistry.java

@@ -22,7 +22,7 @@
 import lombok.RequiredArgsConstructor;
 import lombok.extern.slf4j.Slf4j;

-class PropagationRegistry {
+public class PropagationRegistry {


looking at https://github.com/jaegertracing/legacy-client-java/pull/20/files, I don't see it being constructed there. So why public?

My mistake. Forgot to remove.

yurishkuro · 2018-08-14T17:07:05Z

jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSubclassTest.java

+
+      @Override
+      protected JaegerTracer createTracer() {
+        return new CustomTracer(this);


neat! Managed without making builder fields public/protected :-)

Ya I forgot this feature to, but it worked.

isaachier · 2018-08-14T17:20:23Z

Thanks for the review! I will update ASAP.

yurishkuro · 2018-08-14T17:27:00Z

overall lgtm. Let's make sure to minimize the unnecessary changes (like formatting only, or the removal of SpanContext.fromString()). Also, what's with the broken build?

isaachier · 2018-08-14T17:45:32Z

I think Travis is having issues with the checkstyle job. I reverted a change I made in a JavaDoc that crashed the checkstyle plugin.

yurishkuro · 2018-08-14T19:38:39Z

jaeger-core/src/test/java/io/jaegertracing/internal/JaegerObjectFactoryTest.java

+import org.junit.Assert;
+import org.junit.Test;
+
+public class JaegerObjectFactoryTest {


technically, this is not testing the object factory, but the extension mechanism.

Fair enough. Will revert renaming.

yurishkuro · 2018-08-14T19:39:20Z

jaeger-core/src/test/java/io/jaegertracing/internal/JaegerObjectFactoryTest.java

+
+  @Test
+  public void testTracer() {
+    final CustomObjectFactory tracingObjectFactory = new CustomObjectFactory();


is this used?

Will check.

yurishkuro · 2018-08-14T19:41:53Z

please update PR description to more clearly explain the purpose of this change.

vprithvi

It looks like this diff has a lot of unnecessary formatting changes which makes it difficult to read. Could you move the formatting changes to it's own diff?

vprithvi · 2018-08-15T14:58:26Z

gradle/wrapper/gradle-wrapper.properties

 distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
 zipStoreBase=GRADLE_USER_HOME
 zipStorePath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-4.7-bin.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-4.7-all.zip


This seems like an unnecessary change

This happened automatically somehow... will revert.

vprithvi · 2018-08-15T15:07:58Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerSpanContext.java

-        .append(Long.toHexString(traceId)).append(":")
-        .append(Long.toHexString(spanId)).append(":")
-        .append(Long.toHexString(parentId)).append(":")
+        .append(Long.toHexString(traceId))


Seems to be an unnecessary change

vprithvi · 2018-08-15T15:08:50Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerSpanContext.java

+        );
+  }
+
+  @Override


Seems to be an unnecessary move

vprithvi · 2018-08-15T15:09:49Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerSpanContext.java

-   * Previously this would've returned null from the extract method, but now it returns a dummy
-   * context with only debugId filled in.
-   *
+   *     from extract() method. This happens in the situation when "jaeger-debug-id" header is


Seems to be an unnecessary change

vprithvi · 2018-08-15T15:11:01Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerTracer.java

-   * Retrieves the currently active span from the {@link ScopeManager}. It cannot be guaranteed that this span
-   * will be a {@link JaegerSpan}, as other libraries might have set this active span there. Consumers expecting
-   * this to return a {@link JaegerSpan} should always check the type of the return and act accordingly.
+   * Retrieves the currently active span from the {@link ScopeManager}. It cannot be guaranteed that


Seems to be an unnecessary change

vprithvi · 2018-08-15T15:11:12Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerTracer.java

   *
   * @return the currently active span from the {@link ScopeManager}
   */
  @Override
  public Span activeSpan() {
-    // the active scope might have been added there through an API extension, similar to what the OT java-metrics
+    // the active scope might have been added there through an API extension, similar to what the OT


Seems to be an unnecessary change

vprithvi · 2018-08-15T15:12:03Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerTracer.java

@@ -219,14 +213,15 @@ public void close() {
    private String operationName;
    private long startTimeMicroseconds;
    /**
-     * In 99% situations there is only one parent (childOf), so we do not want to allocate
-     * a collection of references.
+     * In 99% situations there is only one parent (childOf), so we do not want to allocate a


Seems to be an unnecessary change

vprithvi · 2018-08-15T15:12:19Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerTracer.java

@@ -304,7 +299,7 @@ private JaegerSpanContext createNewContext(String debugId) {
        tags.put(Constants.DEBUG_ID_HEADER_KEY, debugId);
        metrics.traceStartedSampled.inc(1);
      } else {
-        //TODO(prithvi): Don't assume operationName is set on creation
+        // TODO(prithvi): Don't assume operationName is set on creation


Seems to be an unnecessary change

vprithvi · 2018-08-15T15:17:39Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerTracer.java

-              computeDurationViaNanoTicks,
-              tags,
-              references);
+      JaegerSpan jaegerSpan =


What do you mean? Last I recall formatting isn't changed as part of the build.

vprithvi · 2018-08-15T15:18:03Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerTracer.java

@@ -326,7 +322,7 @@ private JaegerSpanContext createNewContext(String debugId) {
        return references.get(0).getSpanContext().baggage();
      }

-      for (Reference reference: references) {
+      for (Reference reference : references) {


Seems to be an unnecessary change

yurishkuro · 2018-08-15T16:44:19Z

I would like @jpkrohling or @pavolloffay or @objectiser to approve before we merge, since they've done the most work on Java client recently.

isaachier · 2018-08-15T16:48:40Z

I'm cool with that. I will admit this review is blocking some work internally. Using snapshot releases isn't easy for the projects I am dealing with, so I'd like to wrap this up ASAP.

jpkrohling

The code looks OK to me. There are a couple of things that could be done to resolve an appropriate object factory, but that's optional.

I would also consider changing the method signatures to public instead of protected: if you expect people to extend, even in an unsupported way, then make their lives easier and allow them to extend the way they want. I got bitten recently by this on TracerResolver#resolve ;)

jpkrohling · 2018-08-16T14:01:58Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerObjectFactory.java

@@ -0,0 +1,72 @@
+/*
+ * Copyright (c) 2018, Uber Technologies, Inc


nit: The Jaeger Authors

Good point. This happens to be copied from other files, but we should correct them.

jpkrohling · 2018-08-16T14:16:39Z

jaeger-core/src/main/java/io/jaegertracing/internal/propagation/B3TextMapCodec.java

@@ -129,6 +140,7 @@ public JaegerSpanContext extract(TextMap carrier) {

  public static class Builder {
    private String baggagePrefix = BAGGAGE_PREFIX;
+    private JaegerObjectFactory objectFactory = new JaegerObjectFactory();


You could use the service loader API here, so that having a JaegerObjectFactory in the classpath would be sufficient to use an alternate object factory. Look at how we did it for the MetricsFactory/Sender for a reference.

I had actually been considering something along those lines, but didn't realize there were other implementations. Thanks!

I feel service loader is an overkill here. The goal is to allow subclassing, which is not a runtime decision.

jpkrohling · 2018-08-16T14:20:47Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerSpanContext.java

@@ -107,7 +117,7 @@ public String toString() {
    return contextAsString();
  }

-  public static JaegerSpanContext contextFromString(String value)
+  public static JaegerSpanContext contextFromString(String value, JaegerObjectFactory objectFactory)


To avoid breaking current code (there should be none, but still), keep the old signature, add @Deprecated and use the new one with new JaegerObjectFactory() (or whatever the default value should be):

/** * @deprecated see {@link #contextFromString(String, JaegerObjectFactory)} */ @Deprecated public static JaegerSpanContext contextFromString(String value) { return contextFromString(value, new JaegerObjectFactory()); }

@yurishkuro points out these internal classes aren't part of official API and users are very unlikely to be calling contextFromString directly.

jpkrohling · 2018-08-16T14:22:09Z

jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSubclassTest.java

@@ -0,0 +1,174 @@
+/*
+ * Copyright (c) 2018, Uber Technologies, Inc


Got it thanks :).

yurishkuro · 2018-08-16T16:25:02Z

I would also consider changing the method signatures to public instead of protected

That would extend the API surface, even if internal. What does it prevent if we keep them protected? A subclass can always increase visibility if it needs that.

Signed-off-by: Isaac Hier <ihier@uber.com>

isaachier · 2018-08-21T16:00:24Z

I just squashed the commits and rebased them on top of latest master branch (includes TextMapCodec changes).

yurishkuro

LGTM

yurishkuro reviewed Aug 6, 2018

View reviewed changes

isaachier force-pushed the override-config branch from f1d1085 to 1587771 Compare August 6, 2018 16:46

yurishkuro reviewed Aug 6, 2018

View reviewed changes

isaachier changed the title ~~Add protected methods to Configuration so subclasses may extend it~~ Add protected methods to common classes so subclasses may extend them Aug 6, 2018

yurishkuro suggested changes Aug 6, 2018

View reviewed changes

isaachier force-pushed the override-config branch from 5b167c6 to bcf47f1 Compare August 7, 2018 00:24

yurishkuro reviewed Aug 7, 2018

View reviewed changes

jpkrohling reviewed Aug 7, 2018

View reviewed changes

isaachier force-pushed the override-config branch from 5a5540e to 848881a Compare August 7, 2018 19:39

yurishkuro reviewed Aug 7, 2018

View reviewed changes

yurishkuro reviewed Aug 8, 2018

View reviewed changes

pavolloffay reviewed Aug 9, 2018

View reviewed changes

ghost assigned yurishkuro Aug 14, 2018

ghost added the review label Aug 14, 2018

yurishkuro reviewed Aug 14, 2018

View reviewed changes

yurishkuro approved these changes Aug 14, 2018

View reviewed changes

isaachier force-pushed the override-config branch from 688a061 to b323d5d Compare August 15, 2018 14:57

vprithvi reviewed Aug 15, 2018

View reviewed changes

isaachier mentioned this pull request Aug 15, 2018

Consider google-java-format for auto-formatting diffs #515

Closed

jpkrohling approved these changes Aug 16, 2018

View reviewed changes

Use abstract factory pattern to override core Jaeger types

e61b97c

Signed-off-by: Isaac Hier <ihier@uber.com>

isaachier force-pushed the override-config branch from 01727da to e61b97c Compare August 21, 2018 15:59

isaachier mentioned this pull request Aug 21, 2018

Implement TracerFactory #527

Merged

yurishkuro approved these changes Aug 21, 2018

View reviewed changes

yurishkuro merged commit e248ce0 into jaegertracing:master Aug 21, 2018

ghost removed the review label Aug 21, 2018

isaachier deleted the override-config branch August 22, 2018 14:58

pavolloffay mentioned this pull request May 10, 2019

Automatically generate changelog #629

Closed

		@@ -0,0 +1,72 @@
		/*
		* Copyright (c) 2018, Uber Technologies, Inc

		@@ -0,0 +1,174 @@
		/*
		* Copyright (c) 2018, Uber Technologies, Inc

Allow subclassing of common classes #509

Allow subclassing of common classes #509

Conversation

isaachier commented Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 6, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

isaachier commented Aug 6, 2018

yurishkuro commented Aug 6, 2018

isaachier commented Aug 7, 2018

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaachier Aug 8, 2018 • 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

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

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

yurishkuro Aug 8, 2018 • 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

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

isaachier commented Aug 6, 2018 •

edited

Loading

codecov bot commented Aug 6, 2018 •

edited

Loading

isaachier Aug 8, 2018 •

edited

Loading

yurishkuro Aug 8, 2018 •

edited

Loading