OT API using Brave 4 #14
Conversation
|
@adriancole I've never used retrolambda, can you look at the config to double check? |
…s from opentracing-impl
| * | ||
| * @return the SpanContext that encapsulates Span state that should propagate across process boundaries. | ||
| */ | ||
| @Override |
There was a problem hiding this comment.
if the javadoc are copied from the parent, maybe use
/**
* {@inheritDoc}
*/
pom.xml
Outdated
| </dependency> | ||
| <dependency> | ||
| <groupId>io.zipkin.brave</groupId> | ||
| <artifactId>brave-http</artifactId> | ||
| <version>3.17.0</version> | ||
| <version>4.0.3</version> |
There was a problem hiding this comment.
not sure you need this dep anymore
| <goal>process-main</goal> | ||
| </goals> | ||
| <configuration> | ||
| <target>${main.java.version}</target> |
|
for those reading along, eventually this pull request will look like a cleaned up version of this https://github.com/openzipkin/brave/tree/master/brave/src/test/java/brave/features/opentracing |
|
Interesting :) |
|
In which version of brave will this be included? Will OT become the main api for brave?
There's no plan for OT to be the main api for brave, and this wouldn't
make sense to do. This has been discussed in several forums, reasons
including leaky abstraction, lack of engagement, non-goal of interop.
There's also practical reasons like OpenTracing java being 0.X and
also having deprecated methods included (some of which are things
zipkin needs). Note there is also no plan to make Brave's primary
interface HTrace or Finagle's tracer, despite both having large
ecosystem and/or influence with far less marketing.
So, this will not be the main api for brave as it would be a step
backwards and couple compatibility through a leaky and unstable (<1.0)
api.
brave-opentracing is the main effort towards bridging between
*various* versions of opentracing and would be versioned appropriately
for that. For example, we also have zipkin-finagle, which has the same
bridging goal, and similar to here, since decisions are primarily made
out of the openzipkin org, we have a bridge project to align
appropriately.
Hope this makes sense.
|
|
When I originally commended, I thought I was at the /openzipkin/brave repo 😅 . Since then, I rephrased my question. Your response answered it anyway, thx :) |
|
@adriancole Not sure if its needed or not to add this to the brave3 context / thread binder. I know you want to do a thread context API in brave4, should we use brave3's context until that is worked out? |
There was a problem hiding this comment.
I don't think this PR should use any brave-specific propagation api, as OT does not define propagation at this level of abstraction. Rather, it would using the "global-tracer/active-span" contrib stuff.
In other words, no reason to include brave 3 apis directly, unless it is a part of supporting the OT extension apis
| */ | ||
| @Override | ||
| public String getBaggageItem(String key) { | ||
| // brave does not support baggage |
There was a problem hiding this comment.
maybe move this to javadoc instead of inheritdoc.
/**
* Returns null as neither <a href="https://github.com/openzipkin/b3-propagation">B3</a>
* nor Brave include baggage support.
*/
// OpenTracing could one day define a way to plug-in arbitrary baggage handling similar to how
// it has feature-specific apis like active-span
| public Span log(Map<String, ?> fields) { | ||
| if (fields.isEmpty()) return this; | ||
| // in real life, do like zipkin-go-opentracing: "key1=value1 key2=value2" | ||
| return log(fields.toString()); |
There was a problem hiding this comment.
I'd go ahead and make the utility function now
ex.
static String toAnnotation(Map<String, ?> fields) {
// special-case the "event" field which is similar to the semantics of a zipkin annotation
Object event = fields.get("event");
if (event != null && fields.size() == 1) return event.toString();
return joinOnEqualsSpace(fields);
}
static String joinOnEqualsSpace(Map<String, ?> fields) {
if (fields.isEmpty()) return "";
StringBuilder result = new StringBuilder();
for (Iterator<Map.Entry<String, String>> i = fields.entrySet().iterator(); i.hasNext(); ) {
Map.Entry<String, String> next = i.next();
result.append(next.getKey()).append('=').append(next.getValue());
if (i.hasNext()) result.append(' ');
}
return result.toString();
}| static final TraceContext.Extractor<TextMapView> EXTRACTOR = Propagation.B3_STRING.extractor(TextMapView::get); | ||
|
|
||
| private brave.Tracer brave4; | ||
| private Brave brave3; // Not sure if I need this or not. Might want to attach to the local tracer context |
There was a problem hiding this comment.
I think the adapter pattern is sufficient rather than make a permanent dependency on brave-core
| } | ||
|
|
||
| @Test | ||
| public void extractTraceContext() throws Exception { |
There was a problem hiding this comment.
Hey @devinsba,
great work 👍 .
I have a question. Does this PR solve extract when there is no span context in a carrier? Current impl returns NoopSpanBuilder...
There was a problem hiding this comment.
looks like a missing test is all.
currently...
- if the sampling flag is "0", but there's no ids, trace ids will be generated (but still have sampling false)
- if there's nothing valid, trace ids will be generated along with a new sampling decision
If the semantics of OpenTracing say that no-op is for extract failure or unsampled, the logic could be changed.
There was a problem hiding this comment.
Thanks, this is great 👍
There was a problem hiding this comment.
I had a look at this, and it is an interesting topic.
Seems the api says to throw IllegalArgumentException when you cannot extract a context due to something corrupt https://github.com/opentracing/opentracing-java/blob/master/opentracing-api/src/main/java/io/opentracing/Tracer.java#L76
It doesn't say if null is permitted or not as a return value. I just noticed that at least in Jaeger null is returned when fields are missing etc.
sampled=0 without ids isn't corrupt, rather an unsampled context. Probably grey area whether to generate span ids here when absent (Historically in B3, ids were ignored when unsampled, but some prefer to propagate them anyway)
There was a problem hiding this comment.
I was also looking at null returned in Jaeger..
Seems the api says to throw IllegalArgumentException when you cannot extract a context due to something corrupt https://github.com/opentracing/opentracing-java/blob/master/opentracing-api/src/main/java/io/opentracing/Tracer.java#L76
I understand this like: there is X─B3─ParentSpanId but X─B3─TraceId is missing (or similar combinations).
There was a problem hiding this comment.
PS I thought about this last night.. we can raise a separate issue about policy. This could be configuration specified when building the opentracer
Ex.
- ExtractFailurePolicy.RESTART
- default and what exists on most tracers
- ExtractFailurePolicy.THROW
- throw IllegalStateException knowing it might break apps, like assertions on
- ExtractFailurePolicy.TAG_AND_RESTART
- add a tag with the cause of the extraction failure, but restart normally
cc @basvanbeek who I discussed this lightly with..
There was a problem hiding this comment.
I like it..
Isn't ExtractFailurePolicy.TAG_AND_RESTART redundant? if there is a standard tag for it, it makes sense, otherwise it would by tracer specific.
There was a problem hiding this comment.
Should I defer this change to a separate PR?
There was a problem hiding this comment.
yeah different PR for sure
|
I understand this like: there is X─B3─ParentSpanId but X─B3─TraceId is
missing (or similar combinations).
perhaps.. anyway it is a bit of a bike shed topic
|
Ok, cool. Then this is close. Just want to look over the tests a bit more for completeness. |
| import java.util.Collections; | ||
| import java.util.Map; | ||
|
|
||
| public class BraveSpanContext implements SpanContext { |
There was a problem hiding this comment.
make this final and add javadoc
/**
* Holds the {@linkplain TraceContext} used by the underlying {@linkplain brave.Tracer}. An
* {@link TraceContext#sampled() unsampled} context results in a {@link opentracing...NoopSpan}.
*
* <p>This type also includes hooks to integrate with the underlying {@linkplain brave.Tracer}.
* Ex you can access the underlying trace context with {@link #unwrap}
*/
|
|
||
| public class BraveSpanContext implements SpanContext { | ||
|
|
||
| private TraceContext traceContext; |
| this.traceContext = traceContext; | ||
| } | ||
|
|
||
| final TraceContext unwrap() { |
There was a problem hiding this comment.
public (final implied when you final the type)
There was a problem hiding this comment.
also add javadoc
/** Returns the underlying trace context for use in Brave apis */
| } | ||
|
|
||
| /** | ||
| * {@inheritDoc} |
There was a problem hiding this comment.
change to..
/**
* Returns empty as neither <a href="https://github.com/openzipkin/b3-propagation">B3</a>
* nor Brave include baggage support.
*/
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public class BraveTracer implements Tracer { |
There was a problem hiding this comment.
add final
add javadoc similar to what's on brave.tracer
| static final TraceContext.Injector<TextMap> INJECTOR = Propagation.B3_STRING.injector(TextMap::put); | ||
| static final TraceContext.Extractor<TextMapView> EXTRACTOR = Propagation.B3_STRING.extractor(TextMapView::get); | ||
|
|
||
| private brave.Tracer brave4; |
| * {@inheritDoc} | ||
| */ | ||
| @Override | ||
| public Span start() { |
There was a problem hiding this comment.
covariant override as BraveSpan
There was a problem hiding this comment.
Not sure what you mean by this?
There was a problem hiding this comment.
I mean @Override BraveSpan start()
|
|
||
| public class BraveSpan implements Span { | ||
|
|
||
| static BraveSpan wrap(brave.Span span) { |
There was a problem hiding this comment.
make this public and javadoc
/** Converts an existing {@linkplain brave.Span} for use in OpenTracing apis */
|
|
||
| import java.util.Map; | ||
|
|
||
| public class BraveSpan implements Span { |
There was a problem hiding this comment.
make type final with javadoc similar to BraveSpanContext
| import java.util.LinkedHashMap; | ||
| import java.util.Map; | ||
|
|
||
| public class BraveSpanBuilder implements Tracer.SpanBuilder { |
There was a problem hiding this comment.
add javadoc similar to BraveSpanContext
| @@ -30,6 +30,14 @@ | |||
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | |||
| <project.build.resourceEncoding>UTF-8</project.build.resourceEncoding> | |||
|
|
|||
| <!-- default bytecode version for src/main --> | |||
There was a problem hiding this comment.
add or replace the root README with one that looks like the one from openzipkin/brave/brave (particularly the section about converting in-out from brave 3, as this is similar to converting in and out of opentracing)
|
Isn't ExtractFailurePolicy.TAG_AND_RESTART redundant? if there is a
standard tag for it, it makes sense, otherwise it would by tracer specific.
I don't know of such a tag, but anyway whether or not to add a tag is a
policy choice.. the goal was to kindof think about this since there's
literally opposing valid view points about how to handle malformed stuff.
|
|
It's definitely a good idea 👍 thanks. Could you please raise it in OT/spec repo as it is your idea 👍 ? Or I can do it. |
|
It's definitely a good idea 👍 thanks.
np
Could you please raise it in OT/spec repo as it is your idea 👍 ? Or I
can do it.
feel free to raise it if you feel it should proceed.
I think the topic is how to address ambiguity in extraction. The motivation
is that nuance limits effectiveness of layered software (as right now they
don't know whether to expect null or catch an exception etc).
possible solutions:
* add policy to how a tracer is constructed, so that the tracer's method of
extraction follows rules.
* expose the policy w/o indicating how it was set, so that layered
instrumentation know what to expect
* punt policy to instrumentation layer who guess on grey areas (ex look for
null, catch exceptions etc)
|
| return new BraveSpan(span); | ||
| } | ||
|
|
||
| final brave.Span unwrap() { |
There was a problem hiding this comment.
Wondering if this should be public?
| * {@inheritDoc} | ||
| */ | ||
| @Override | ||
| public Span start() { |
There was a problem hiding this comment.
Not sure what you mean by this?
| } | ||
|
|
||
| @Test | ||
| public void extractTraceContext() throws Exception { |
There was a problem hiding this comment.
Should I defer this change to a separate PR?
|
Honestly I'm not sure what exactly to modify in the current README. The old one is still completely applicable (the magic of using the same interfaces). I guess there might be some context from the discussion in this thread missing though. @adriancole you have edit permission if you want to take a shot |
|
wrt the README, this is probably not valid "BraveTracer tracer = new
BraveTracer();"
also the status section is completely invalid and can be removed
how about fixing those two things?
|
|
ex. for the README
Tracer braveTracer = Tracer.newBuilder().....build();
BraveTracer opentracing = BraveTracer.wrap(brave);
later we can do more, but minimally the above and removing the status
section should be good enough
|
|
thanks @devinsba for all the work here! Thanks also @felixbarny and @pavolloffay for taking a look. will release shortly |
|
going out now as v0.18.0 |
|
This is great 👍 👍 👍 thanks! |
|
Agreed, thank you @devinsba and @adriancole! |
Move to Brave 4 implementation for backing the OpenTracing API layer
BraveTracerBraveSpanBraveSpanBuilderBraveSpanContextbravepackage spacebrave 3 context??...