Use nextSpan instead of joinSpan

[simao]

To account for the case when contextOrFlags.context() == null

[codefromthecrypt]

this will make it always make a child. I think it is better to guard for now.

[shimamoto]

@adriancole Do you mean it is better to fix as follows:

Option(contextOrFlags.context())
      .map(tracer.joinSpan)
      .getOrElse(tracer.nextSpan(contextOrFlags))

[simao]

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?

[codefromthecrypt]

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 ***@***.***> 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 <#53?email_source=notifications&email_token=AAAPVV34IWG2IXIHJ7PTIULP6WB35A5CNFSM4H7HHUV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZSP3IQ#issuecomment-509935010>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAPVV3RH37CDVEV76ETHH3P6WB35ANCNFSM4H7HHUVQ> .

[codefromthecrypt]

also here is the correct code which can be copy paste until such time as the http abstraction is implemented here.. https://github.com/openzipkin/brave/blob/master/instrumentation/http/src/main/java/brave/http/HttpServerHandler.java#L96

[shimamoto]

I think this looks good.

[codefromthecrypt]

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);
  }

[codefromthecrypt]

sorry actually this code looks ok because you don't have http sampler anyway yet. it does need test case though

[codefromthecrypt]

this is good, just need test case. you can use something like only sampled header to test.

ex. test that X-B3-Sampled: 0 results in a new unsampled trace

[simao]

It seems like I would need #32 to write a test for this?

[codefromthecrypt]

unit test could be done now I suspect.

[codefromthecrypt]

been a long time. just rebased the project so it has CI etc

[simao]

just rebased and seems ci is green. I'll try to write that test.

[simao]

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.