Remove dependency from jaeger-core to jaeger-thrift #449

jpkrohling · 2018-06-13T13:09:22Z

Resolves #448 - This PR removes the dependency from jaeger-core to jaeger-thrift by loading
the appropriate Sender via either environment variables or based on existing service providers
on the classpath, loaded via ServiceLoader.

Signed-off-by: Juraci Paixão Kröhling [email protected]

Resolves jaegertracing#448 - This PR removes the dependency from jaeger-core to jaeger-thrift by loading the appropriate Sender via either environment variables or based on existing service providers on the classpath, loaded via ServiceLoader. Signed-off-by: Juraci Paixão Kröhling <[email protected]>

codecov · 2018-06-13T13:16:16Z

Codecov Report

Merging #449 into master will increase coverage by 0.8%.
The diff coverage is 97.22%.

@@             Coverage Diff             @@
##             master     #449     +/-   ##
===========================================
+ Coverage     87.55%   88.35%   +0.8%     
+ Complexity      517      489     -28     
===========================================
  Files            65       63      -2     
  Lines          2000     1846    -154     
  Branches        263      240     -23     
===========================================
- Hits           1751     1631    -120     
+ Misses          160      137     -23     
+ Partials         89       78     -11

Impacted Files	Coverage Δ	Complexity Δ
...ava/io/jaegertracing/reporters/RemoteReporter.java	`83.33% <100%> (ø)`	`7 <0> (ø)`	⬇️
...main/java/io/jaegertracing/senders/NoopSender.java	`100% <100%> (ø)`	`5 <5> (?)`
.../src/main/java/io/jaegertracing/Configuration.java	`94.14% <100%> (+2.83%)`	`39 <0> (ø)`	⬇️
.../java/io/jaegertracing/senders/SenderResolver.java	`96.55% <96.55%> (ø)`	`9 <9> (?)`
...er-core/src/main/java/io/jaegertracing/Tracer.java	`89.66% <0%> (+2.47%)`	`23% <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 808d6cc...4831dfe. Read the comment docs.

objectiser · 2018-06-13T13:29:19Z

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

@@ -137,6 +136,11 @@
   */
  public static final String JAEGER_PROPAGATION = JAEGER_PREFIX + "PROPAGATION";

+  /**
+   * The fully qualified class name for the sender factory


Just wondering if we could select the factory based on a simpler name, which may be maps onto a predefined class name - so the configuration by the user requires less implementation knowledge?

This would be required only if there are multiple factories in the classpath. Do you think it would be a common use case to justify a map of "common" factories?

Not necessary common, but just thinking of the detail the user would need to know (i.e. the fully qualified class name) to select a transport.

In the usual case, they just need to have the transport on the classpath, there's no need to know the class name. If they end up somehow with two transports, the class name is used to disambiguate.

How about we go without this map for now, and add later if the case is more common than we thought?

Two other suggestions - (1) we treat multiple factories as an error - so if the case comes up to justify having two in the classpath, we can explore it further then, (2) add a type property on the factory, and that is what is specified in the environment variable?

add a type property on the factory, and that is what is specified in the environment variable?

I like this idea, as it's easily extensible.

I think FQN is better than just a class name.

add a type property on the factory,

is the type known when calling the factory?

I think FQN is better than just a class name.

This is also available: if the env var contains a class name, we try to load it. Otherwise, we try to get a single factory from the service loader. If there are multiple, we try to match the type with the env var. If none matches, NoopSender is returned.

objectiser · 2018-06-13T13:33:12Z

jaeger-core/src/main/java/io/jaegertracing/senders/SenderResolver.java

+      return sender;
+    }
+
+    return new NoopSender();


Should also log a warning that no sender has been found.

objectiser · 2018-06-13T13:41:53Z

jaeger-thrift/build.gradle

@@ -43,6 +48,11 @@ shadowJar {
    classifier 'thrift92'
 }

+shadowJar {


Don't we just want the single shaded jar, with the dependencies relocated?

Should I just remove the one from jaeger-core and have then jaeger-thrift-okhttp381?

http://central.maven.org/maven2/io/jaegertracing/jaeger-core/0.28.0/

Sorry, I think I get a better understanding now. Did you mean to have the shadowed okhttp on the main artifact as well? Or did you mean to have the optional okhttp shadow artifact for thrift, instead of for jaeger-core?

I just changed this PR to include both shadowed thrift and okhttp on the same "shadow" artifact

objectiser · 2018-06-13T13:44:20Z

jaeger-thrift/src/test/java/io/jaegertracing/thrift/senders/ThriftSenderFactoryTest.java

+
+    assertTrue(Configuration.SenderConfiguration.fromEnv().getSender() instanceof HttpSender);
+  }
+


Shouldn't there be some tests for getting the UdpSender?

objectiser · 2018-06-13T13:45:11Z

jaeger-zipkin/build.gradle

@@ -8,6 +8,7 @@ dependencies {
    compile group: 'io.zipkin.reporter2', name: 'zipkin-reporter', version: '2.6.0'

    compile project(path: ':jaeger-core', configuration: 'shadow')


Is a shadow configuration still required here? It might need to be on the jaeger-thrift dependency?

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

yurishkuro · 2018-06-13T15:19:04Z

jaeger-core/src/test/java/io/jaegertracing/senders/InMemorySender.java

@@ -34,33 +32,32 @@
  private Semaphore semaphore = new Semaphore(Integer.MAX_VALUE);

  public InMemorySender() {


we used to have inMemoryReporter. What's the point of InMemorySender?

The InMemorySender was in the reporters package. I moved it to this package, as it seems more appropriate.

ok, just for tests - nvm

yurishkuro · 2018-06-13T15:19:32Z

jaeger-core/src/test/java/io/jaegertracing/senders/InMemorySender.java

  }

  public List<Span> getAppended() {
-    return new ArrayList<Span>(appended);
+    return new ArrayList<>(appended);


what are the threading expectations?

Given that it's used only for tests, I would say that we don't guarantee anything. Concurrent modification will cause a ConcurrentModificationException, for instance.

yurishkuro · 2018-06-13T15:20:32Z

jaeger-thrift/build.gradle

@@ -6,9 +6,14 @@ apply plugin: 'com.github.johnrengelman.shadow'
 description = 'Generated thrift code'


This description wasn't changed in this PR. Would you like this description changed?

it's no longer accurate since you're extending the purpose and footprint of this module.

yurishkuro · 2018-06-13T15:26:06Z

jaeger-core/src/main/java/io/jaegertracing/senders/SenderFactory.java

+   * @param senderConfiguration the sender configuration
+   * @return an appropriate sender based on the configuration, or {@link NoopSender}.
+   */
+  Sender getSender(Configuration.SenderConfiguration senderConfiguration);


passing the configuration is kind of an abstraction leak in this case - the configuration must know about the needs of every possible sender factory.

Agree. This would require a bigger refactor than I'd like to focus on at this point. For consistency's sake, other configuration objects would also need such a change :/

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

pavolloffay · 2018-06-14T09:10:06Z

jaeger-core/src/main/java/io/jaegertracing/senders/SenderFactory.java

+  /**
+   * Builds and/or selects the appropriate sender based on the given {@link Configuration.SenderConfiguration}
+   * @param senderConfiguration the sender configuration
+   * @return an appropriate sender based on the configuration, or {@link NoopSender}.


The NoopSender is an implementation detail of SenderResolver

Not really -- I expect concrete SenderFactory implementations to never return null. The contract is: either a real Sender, or NoopSender.

What can't we return null? It's invoked only in SenderFactory which takes care about null/noop.

This is a contract between the implementation and the consumer, meaning that this documentation is a reference to both sides. By stating that it might return NoopSender, we are ruling out a null object, which makes it far easier for consuming code to not break (no null checks, no null pointer exceptions).

I still think the Noop here is just redundant. This API does not need it and it's just creating a dependency on specific sender impl

pavolloffay · 2018-06-14T09:11:17Z

jaeger-core/src/main/java/io/jaegertracing/senders/SenderFactory.java

+
+/**
+ * Represents a class that knows how to select and build the appropriate {@link Sender} based on the given
+ * {@link Configuration.SenderConfiguration}


I would expect some comments that this class is resolved via service loader. Also some explanation about meta-inf/services

The resolver loads this via the service loader, but there's nothing preventing a consumer from calling a concrete implementation directly. Like: SenderFactory f = new InMemorySenderFactory()

pavolloffay · 2018-06-14T09:12:51Z

jaeger-core/src/main/java/io/jaegertracing/senders/SenderResolver.java

+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * Provides a way to resolve an appropriate {@link Sender}


I am missing some comments about how the sender is resolved. Is it using service loader or what approach? What should I do to provide my own sender?

This is implementation-specific. The ThriftSender, for instance, builds an UdpSender or HttpSender based on the contents of the SenderConfiguration. More information is provided at the method-level javadoc.

Then how consumers of this API know how to provide their own sender?

Consumer of the API != Service Provider.

The Service Provider will know how to instantiate a new Sender and will return it as a result of this method call (see jaeger-thrift).

The consumer of the API (target application) will, at most, set the appropriate configuration options on SenderConfiguration and will get an appropriate sender in return.

I have my own sender impl. How do I use jaeger-core to load it from classpath? Where can I found more info about it? I just want to use Configuration.fromEnv()

You implement the SenderFactory, which returns your own Sender on the resolve() method. See the unit tests and the jaeger-thrift for an example on how to do that.

I implement SenderFactory in my app, add jaeger-core. then I use Configuration.fromEnv() without specifying any env var. Would be my sender factory used?

Yes. The only thing missing in your example is the META-INF/services/io.jaegertracing.senders.SenderFactory .

https://github.com/jpkrohling/jaeger-client-java/blob/6a83cd7c56877bc17aeb8f8fd432a00a2e2e99b7/jaeger-thrift/src/main/resources/META-INF/services/io.jaegertracing.senders.SenderFactory

pavolloffay · 2018-06-14T09:14:35Z

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

@@ -137,6 +136,11 @@
   */
  public static final String JAEGER_PROPAGATION = JAEGER_PREFIX + "PROPAGATION";

+  /**
+   * The fully qualified class name for the sender factory


I think FQN is better than just a class name.

pavolloffay · 2018-06-14T09:15:14Z

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

@@ -137,6 +136,11 @@
   */
  public static final String JAEGER_PROPAGATION = JAEGER_PREFIX + "PROPAGATION";

+  /**
+   * The fully qualified class name for the sender factory


add a type property on the factory,

is the type known when calling the factory?

pavolloffay · 2018-06-14T09:20:17Z

jaeger-core/src/main/java/io/jaegertracing/senders/NoopSender.java

+
+  @Override
+  public int flush() {
+    return 1;


I think 0 is more appropriate - also for close.

Thought so too. But not for the append(Span), right?

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

objectiser · 2018-06-14T10:06:51Z

jaeger-core/src/main/java/io/jaegertracing/senders/SenderFactory.java

+  Sender getSender(Configuration.SenderConfiguration senderConfiguration);
+
+  /**
+   * The Factory's name. Can specified via {@link Configuration#JAEGER_SENDER_FACTORY} to disambiguate


nit: "Can be specified"

objectiser · 2018-06-14T10:08:32Z

jaeger-core/src/main/java/io/jaegertracing/senders/SenderResolver.java

+   * @return the resolved Sender, or NoopSender
+   */
+  public static Sender resolve(Configuration.SenderConfiguration senderConfiguration) {
+    String senderFactoryClass = System.getProperty(Configuration.JAEGER_SENDER_FACTORY);


nit: senderFactoryClass -> senderFactoryType ?

Isn't this still using the old mechanism - where the env var specifies a fq class name?

It can be both. At this point in the code, we assume it's a class (we try to use it on a Class.forName later on).

Isn't this still using the old mechanism - where the env var specifies a fq class name?

Yes. I thought the type is only used to disambiguate in case there are multiple factories available? The previous resolution was:

FQCN

Service Loader (first service available)

NoopSender

The current is:

FQCN

Service Loader (when only one service is available)

Service Loader + Type (when multiple services are available)

NoopSender

Type in SenderFactory seems redundant as you can still specify the wanted impl as FQCN or wouldn't it work?

It would, and it's how it was being done before. Looks at this thread: #449 (comment)

objectiser · 2018-06-14T10:30:10Z

jaeger-thrift/build.gradle

@@ -40,7 +43,8 @@ checkstyleTest.enabled = false
 shadowJar {
    baseName = 'jaeger-thrift'
    relocate 'org.apache.thrift', 'org.shadow.apache.thrift92'
-    classifier 'thrift92'
+    relocate 'okhttp', 'jaeger.okhttp'


nit: we should maybe have a consistent relocate pattern - so possibly prefix jaeger as here, so update the thrift one to jaeger.org.apache.thrift (without the 92)?
btw - might also be worth looking at the other dependencies included in this shaded jar, as they may be should be relocated aswell to avoid any potential clashes - or possibly this should be a separate PR.

or possibly this should be a separate PR

That would be my preference.

objectiser

LGTM - subject to resolving the factory env var issue. Couple of nits to look at.

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

pavolloffay · 2018-06-14T12:31:21Z

jaeger-core/src/main/java/io/jaegertracing/senders/SenderResolver.java

+  /**
+   * Resolves a sender by passing the given {@link Configuration.SenderConfiguration} down to the
+   * {@link SenderFactory}. The factory is loaded either based on the value from the environment variable
+   * JAEGER_SENDER_FACTORY or, in its absence or failure to deliver a {@link Sender}, via the {@link ServiceLoader}.


JAEGER_SENDER_FACTORY make it a javadoc link.

pavolloffay · 2018-06-14T12:44:35Z

jaeger-core/src/main/java/io/jaegertracing/senders/SenderResolver.java

+   * @return the resolved Sender, or NoopSender
+   */
+  public static Sender resolve(Configuration.SenderConfiguration senderConfiguration) {
+    String senderFactoryClass = System.getProperty(Configuration.JAEGER_SENDER_FACTORY);


Type in SenderFactory seems redundant as you can still specify the wanted impl as FQCN or wouldn't it work?

pavolloffay · 2018-06-14T12:46:44Z

jaeger-core/src/main/java/io/jaegertracing/senders/SenderResolver.java

+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * Provides a way to resolve an appropriate {@link Sender}


I implement SenderFactory in my app, add jaeger-core. then I use Configuration.fromEnv() without specifying any env var. Would be my sender factory used?

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

jpkrohling requested a review from objectiser June 13, 2018 13:09

ghost assigned jpkrohling Jun 13, 2018

ghost added the review label Jun 13, 2018

jpkrohling requested a review from pavolloffay June 13, 2018 13:15

objectiser reviewed Jun 13, 2018

View reviewed changes

Set shadow jar to jaeger-thrift only

043a4b0

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

jpkrohling force-pushed the 448-RemoveDependencyFromCoreToThrift branch from b21feed to 043a4b0 Compare June 13, 2018 14:32

yurishkuro reviewed Jun 13, 2018

View reviewed changes

jpkrohling added 2 commits June 13, 2018 18:33

Test exceptional cases

9cd3dec

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Change description for jaeger-thrift module

61e6a17

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

pavolloffay reviewed Jun 14, 2018

View reviewed changes

Added getType option for SenderFactory

28240b8

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

objectiser reviewed Jun 14, 2018

View reviewed changes

objectiser approved these changes Jun 14, 2018

View reviewed changes

jpkrohling mentioned this pull request Jun 14, 2018

Align shadow relocations #450

Closed

Fix documentation about JAEGER_SENDER_FACTORY

6a83cd7

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

pavolloffay reviewed Jun 14, 2018

View reviewed changes

jpkrohling added 2 commits June 14, 2018 14:56

Add javadoc link on JAEGER_SENDER_FACTORY

730791c

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Remove resolution based on class name via env var

57f53ec

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

jpkrohling force-pushed the 448-RemoveDependencyFromCoreToThrift branch from 9c8cec0 to 57f53ec Compare June 14, 2018 13:53

Improve javadoc for SenderFactory to state how it's usually used

4831dfe

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

jpkrohling mentioned this pull request Jun 14, 2018

HttpSender errors are not logged #406

Closed

pavolloffay approved these changes Jun 14, 2018

View reviewed changes

jpkrohling merged commit 0ece0fd into jaegertracing:master Jun 14, 2018

ghost removed the review label Jun 14, 2018

This was referenced Jun 15, 2018

HttpSender errors are not logged #406 #452

Closed

Make it easier for one process to report spans as multiple services #212

Closed

pavolloffay mentioned this pull request May 10, 2019

Automatically generate changelog #629

Closed


		assertTrue(Configuration.SenderConfiguration.fromEnv().getSender() instanceof HttpSender);
		}

		@@ -8,6 +8,7 @@ dependencies {
		compile group: 'io.zipkin.reporter2', name: 'zipkin-reporter', version: '2.6.0'

		compile project(path: ':jaeger-core', configuration: 'shadow')

		@@ -34,33 +32,32 @@
		private Semaphore semaphore = new Semaphore(Integer.MAX_VALUE);

		public InMemorySender() {

		@@ -6,9 +6,14 @@ apply plugin: 'com.github.johnrengelman.shadow'
		description = 'Generated thrift code'

Remove dependency from jaeger-core to jaeger-thrift #449

Remove dependency from jaeger-core to jaeger-thrift #449

Conversation

jpkrohling commented Jun 13, 2018

codecov bot commented Jun 13, 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

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

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

objectiser 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

codecov bot commented Jun 13, 2018 •

edited

Loading