Avoid direct access to apache thrift from jaeger-core via transitive … #374

objectiser · 2018-04-04T13:37:20Z

…dependency through jaeger-thrift.

Fixes #228

Aim is to minimise changes while removing all direct usage of Apache Thrift from jaeger-core.

Signed-off-by: Gary Brown gary@brownuk.com

codecov · 2018-04-04T13:43:39Z

Codecov Report

Merging #374 into master will increase coverage by 0.16%.
The diff coverage is 86.2%.

@@             Coverage Diff              @@
##             master     #374      +/-   ##
============================================
+ Coverage     84.56%   84.73%   +0.16%     
+ Complexity      621      607      -14     
============================================
  Files            95       94       -1     
  Lines          2468     2410      -58     
  Branches        276      271       -5     
============================================
- Hits           2087     2042      -45     
+ Misses          284      276       -8     
+ Partials         97       92       -5

Impacted Files	Coverage Δ	Complexity Δ
...c/main/java/com/uber/jaeger/senders/UdpSender.java	`77.77% <100%> (+4.44%)`	`5 <2> (ø)`	⬇️
...ain/java/com/uber/jaeger/senders/ThriftSender.java	`75% <80%> (+1.53%)`	`10 <3> (ø)`	⬇️
.../main/java/com/uber/jaeger/senders/HttpSender.java	`90.47% <88.88%> (+0.47%)`	`8 <2> (ø)`	⬇️

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 c927b80...3df898d. Read the comment docs.

jpkrohling · 2018-04-04T15:18:27Z

jaeger-core/src/main/java/com/uber/jaeger/senders/HttpSender.java

  }

  @Override
-  public void send(Process process, List<Span> spans) throws TException {
+  public void send(Process process, List<Span> spans) throws Exception {


Is it possible to have a more specialized exception?

Actually I think it could be changed to SenderException - will give it a go.

jpkrohling · 2018-04-04T15:23:10Z

jaeger-thrift/src/main/java/com/uber/jaeger/thrift/senders/ThriftSenderBase.java

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


s/Uber Technologies, Inc/The Jaeger Authors/

jpkrohling · 2018-04-04T15:24:40Z

...er-thrift/src/test/java/com/uber/jaeger/thrift/reporters/protocols/ThriftSenderBaseTest.java

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


s/Uber Technologies, Inc/The Jaeger Authors/

jpkrohling · 2018-04-04T15:25:28Z

...er-thrift/src/test/java/com/uber/jaeger/thrift/reporters/protocols/ThriftSenderBaseTest.java

+    int a = calculateBatchOverheadDifference(1);
+    int b = calculateBatchOverheadDifference(2);
+
+    // This value has been empirically observed to be 56.


I know it's just a code being moved from the old test to the new one, but is this comment accurate? Which value is it referring to? If it's EMIT_BATCH_OVERHEAD, then it's 33, not 56...

Yeah noticed that too - but wanted to preserve what was already there.
@yurishkuro ok to update the comment, as it looks like it should be 33?

vprithvi · 2018-04-04T17:24:01Z

jaeger-core/src/main/java/com/uber/jaeger/senders/UdpSender.java

+    try {
+      agentClient.emitBatch(new Batch(process, spans));
+    } catch (Exception e) {
+      throw new SenderException(String.format("Could not send %d spans", spans.size()), e, 1);


I think the last parameter needs to be spans.size().
That being said, I don't like constructor for SenderException. It seems in most of the usages, the spans dropped is mentioned as part of the string in the first parameter. This could be an opportunity to refactor the API to be more friendly.

Fixed. Could be refactored, although the dropped span count is currently used in ThriftSender - so not sure whether having access to the count has other uses.

vprithvi · 2018-04-04T17:38:56Z

Semi related question: Does it make sense to move the senders and transports that depend on thrift to the jaeger-thrift package also (Keeping only the base Sender interface in jaeger-core)?
This way, the transport layer becomes easier to manage from a dependency management perspective (and allow for easy migration to things like grpc, etc)

objectiser · 2018-04-05T08:38:28Z

@vprithvi Did think about this change initially, but the problem is that the senders use classes in jaeger-core (e.g. com.uber.jaeger.Span) so would result in circular dependency.

objectiser · 2018-04-05T21:31:06Z

@vprithvi Are you ok with the changes? Would be good to get this one merged.

vprithvi · 2018-04-05T21:54:02Z

LGTM!

Did think about this change initially, but the problem is that the senders use classes in jaeger-core (e.g. com.uber.jaeger.Span) so would result in circular dependency.

This is unfortunate

…dependency through jaeger-thrift. Signed-off-by: Gary Brown <gary@brownuk.com>

…t and other comment Signed-off-by: Gary Brown <gary@brownuk.com>

Signed-off-by: Gary Brown <gary@brownuk.com>

yurishkuro · 2018-04-14T20:21:49Z

jaeger-thrift/src/main/java/com/uber/jaeger/thrift/senders/ThriftSenderBase.java

+    Compact
+  }
+
+  public static final int EMIT_BATCH_OVERHEAD = 33;


this value is dependent on the protocol

Will take a look at this.

yurishkuro · 2018-04-15T02:10:05Z

jaeger-core/src/main/java/com/uber/jaeger/senders/ThriftSender.java

@@ -17,67 +17,52 @@
 import com.uber.jaeger.Span;
 import com.uber.jaeger.exceptions.SenderException;
 import com.uber.jaeger.reporters.protocols.JaegerThriftSpanConverter;
-import com.uber.jaeger.reporters.protocols.ThriftUdpTransport;
+import com.uber.jaeger.thrift.reporters.protocols.ThriftUdpTransport;
+import com.uber.jaeger.thrift.senders.ThriftSenderBase;


I am not sure I fully understand this change. Don't these imports still retain the dependency on jaeger-thrift?

Yes there is still a dependency from core to jaeger-thrift. This PR was about removing direct use of apache thrift from core, as a dependency leak.

More work is required to make a true transport abstraction. Is this something also for 1.0?

ghost assigned objectiser Apr 4, 2018

ghost added the review label Apr 4, 2018

objectiser requested review from jpkrohling, yurishkuro and pavolloffay April 4, 2018 13:37

jpkrohling approved these changes Apr 4, 2018

View reviewed changes

vprithvi reviewed Apr 4, 2018

View reviewed changes

objectiser force-pushed the thrift branch from 5c18956 to 536cfa2 Compare April 5, 2018 21:21

vprithvi approved these changes Apr 5, 2018

View reviewed changes

ghost assigned jpkrohling Apr 6, 2018

objectiser added 3 commits April 6, 2018 09:30

Avoid direct access to apache thrift from jaeger-core via transitive …

66341f9

…dependency through jaeger-thrift. Signed-off-by: Gary Brown <gary@brownuk.com>

Changed abstract send method to throw SenderException, fixed copyrigh…

36a3454

…t and other comment Signed-off-by: Gary Brown <gary@brownuk.com>

Fix exception to use spans.size()

7b19fae

Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser force-pushed the thrift branch from 4778fa3 to 7b19fae Compare April 6, 2018 08:30

jpkrohling removed their assignment Apr 6, 2018

objectiser added 2 commits April 6, 2018 10:00

Refactored to improve test code coverage

a809a02

Signed-off-by: Gary Brown <gary@brownuk.com>

More test coverage

3df898d

Signed-off-by: Gary Brown <gary@brownuk.com>

jpkrohling merged commit 6d822c3 into jaegertracing:master Apr 6, 2018

ghost removed the review label Apr 6, 2018

objectiser deleted the thrift branch April 6, 2018 09:44

objectiser mentioned this pull request Apr 10, 2018

Tracerresolver fat jar request #332

Closed

yurishkuro reviewed Apr 14, 2018

View reviewed changes

yurishkuro reviewed Apr 15, 2018

View reviewed changes

pavolloffay mentioned this pull request May 10, 2019

Automatically generate changelog #629

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid direct access to apache thrift from jaeger-core via transitive … #374

Avoid direct access to apache thrift from jaeger-core via transitive … #374

objectiser commented Apr 4, 2018

codecov bot commented Apr 4, 2018 •

edited

Loading

jpkrohling Apr 4, 2018

objectiser Apr 4, 2018

jpkrohling Apr 4, 2018

jpkrohling Apr 4, 2018

jpkrohling Apr 4, 2018

objectiser Apr 4, 2018

vprithvi Apr 4, 2018

objectiser Apr 5, 2018

vprithvi commented Apr 4, 2018

objectiser commented Apr 5, 2018

objectiser commented Apr 5, 2018

vprithvi commented Apr 5, 2018

yurishkuro Apr 14, 2018

objectiser Apr 16, 2018

yurishkuro Apr 15, 2018

objectiser Apr 16, 2018

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

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

Avoid direct access to apache thrift from jaeger-core via transitive … #374

Avoid direct access to apache thrift from jaeger-core via transitive … #374

Conversation

objectiser commented Apr 4, 2018

codecov bot commented Apr 4, 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

vprithvi commented Apr 4, 2018

objectiser commented Apr 5, 2018

objectiser commented Apr 5, 2018

vprithvi commented Apr 5, 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

codecov bot commented Apr 4, 2018 •

edited

Loading