Change return types from Jaeger Span/Tracer/Context to Jaeger types #469
Conversation
ghost
assigned
| @@ -119,7 +119,7 @@ public static JaegerSpanContext contextFromString(String value) | |||
| } | |||
|
|
|||
| /* | |||
| TODO(oibe) because java doesn't like to convert large hex strings to longs | |||
There was a problem hiding this comment.
I removed the TODO marker here because it doesn't sounds like it's a TODO...
jpkrohling
force-pushed
the
468-Change-return-types-to-Jaegers
branch
from
ce717bf
to
fb05e97
Compare
Codecov Report
@@ Coverage Diff @@
## master #469 +/- ##
=========================================
Coverage 88.12% 88.12%
Complexity 493 493
=========================================
Files 63 63
Lines 1852 1852
Branches 241 241
=========================================
Hits 1632 1632
Misses 142 142
Partials 78 78
Continue to review full report at Codecov.
|
| @@ -168,13 +168,15 @@ public ScopeManager scopeManager() { | |||
| } | |||
|
|
|||
| @Override | |||
| public Span activeSpan() { | |||
| public JaegerSpan activeSpan() { | |||
There was a problem hiding this comment.
I think this will need to remain Span, to allow other Spans to be stored in the ScopeManager - e.g. java-metrics (using api-extensions lib). Don't think this should be a problem - we can focus on returning Jaeger specific implementations based on the fluent api - from build span -> start. It would be the apps responsibility to then return the Jaeger impl if they needed to continue using it
There was a problem hiding this comment.
Do you have an example that I could test this? I could then add a new test based on the example.
There was a problem hiding this comment.
I think the following test should be sufficient:
// prepare
Span activeSpan = mock(Span.class);
ScopeManager scopeManager = tracer.scopeManager();
// test
scopeManager.activate(activeSpan, false);
// check
assertEquals(activeSpan, tracer.activeSpan());| @@ -218,24 +219,24 @@ public void close() { | |||
| private final Map<String, Object> tags = new HashMap<String, Object>(); | |||
| private boolean ignoreActiveSpan = false; | |||
|
|
|||
| SpanBuilder(String operationName) { | |||
| public SpanBuilder(String operationName) { | |||
There was a problem hiding this comment.
Why does this need to change?
There was a problem hiding this comment.
Looks like it doesn't :)
Resolves jaegertracing#468 Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
jpkrohling
force-pushed
the
468-Change-return-types-to-Jaegers
branch
from
65d9c3a
to
46b64d5
Compare
Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de
Which problem is this PR solving?
Short description of the changes