Open simao opened 5 years ago
this will make it always make a child. I think it is better to guard for now.
@adriancole Do you mean it is better to fix as follows:
Option(contextOrFlags.context())
.map(tracer.joinSpan)
.getOrElse(tracer.nextSpan(contextOrFlags))
To be honest I am a bit confused about nextSpan
, joinSpan
and similar... This method is called when calling a remote api for example, in that case shouldn't a new newChild
always be called?
If there is a parent span already, that would be the span of something that called this service and there should be a child span of that span?
maybe the javadoc are not rendering for you. I think it is explained exactly including how to handle conditional here
https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/Tracer.java#L184
On Wed, Jul 10, 2019, 3:57 PM Simão Mata notifications@github.com wrote:
To be honest I am a bit confused about nextSpan, joinSpan and similar... This method is called when calling a remote api for example, in that case shouldn't a new newChild always be called?
If there is a parent span already, that would be the span of something that called this service and there should be a child span of that span?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openzipkin-contrib/play-zipkin-tracing/pull/53?email_source=notifications&email_token=AAAPVV34IWG2IXIHJ7PTIULP6WB35A5CNFSM4H7HHUV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZSP3IQ#issuecomment-509935010, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAPVV3RH37CDVEV76ETHH3P6WB35ANCNFSM4H7HHUVQ .
also here is the correct code which can be copy paste until such time as the http abstraction is implemented here..
I think this looks good.
sorry to be more literal, both old and new are broken code. scala variant of this can fix it.
Span nextSpan(TraceContextOrSamplingFlags extracted, Req request) {
Boolean sampled = extracted.sampled();
// only recreate the context if the http sampler made a decision
if (sampled == null && (sampled = sampler.trySample(adapter, request)) != null) {
extracted = extracted.sampled(sampled.booleanValue());
}
return extracted.context() != null
? tracer.joinSpan(extracted.context())
: tracer.nextSpan(extracted);
}
sorry actually this code looks ok because you don't have http sampler anyway yet. it does need test case though
been a long time. just rebased the project so it has CI etc
just rebased and seems ci is green. I'll try to write that test.
Hi,
I added a unit test, but I think I didn't get the idea, as this test passes even without my changes. Could you explain a bit more what do you mean I should be testing here?
Thanks.
To account for the case when
contextOrFlags.context() == null