Consider Baggage when creating new root traces

[Xyaren]

What Does This Do

Solves #4516

This PR modifies which extracted information is used when creating a new trace.

Motivation

Due to Baggage mapping, there now is a possible need to map headers into new created traces.

Additional Notes

No failing test - If someone can point me to the correct test cases to improve to cover this change please do so.

[bantonsson]

Thanks @Xyaren

I think you need to re-authenticate with CircleCI or push something to the branch to make CI run again.

[Xyaren]

I think you need to re-authenticate with CircleCI or push something to the branch to make CI run again.

What should I do here ?

[bantonsson]

@Xyaren from the build number in your image, it looks like CircleCI is enabled on your fork of the repo, and it is trying to build it using your account. The CircleCI setup is a bit messy, but you need to disable CircleCI for your fork (in CircleCI).

[Xyaren]

@bantonsson I "unfollowed" the project, can you trigger the CI again ?

[bantonsson]

@Xyaren I'm unfortunately unable to trigger anything, since CircleCI seems to think that you want to run CI on your fork https://github.com/Xyaren/dd-trace-java and not on https://github.com/Datadog/dd-trace-java

If you go to the projects for your GitHub account organization in CircleCI, you should see something like this (for your GitHub user):

Make sure that you disable the dd-trace-java project there by clicking the ... on the dd-trace-java line (mine is greyed out since I haven't enabled CircleCI for my fork).

[Xyaren]

That's how it looks like already.
I'll try to re-trigger by adding a new commit.

[Xyaren]

Rebased.

[bantonsson]

@Xyaren there is one code formatting issue. Running ./gradlew formatCode will fix it.

[Xyaren]

@bantonsson I really struggle to find the test failures in the CI Logs.
Some ci steps seem to require an API Key ?

[bantonsson]

@Xyaren sorry about that.

• You can ignore all the ones named system-tests-* since they need an API key secret that is not available to outside contributors.
• If you see several failing jobs named with a similar pattern, i.e. z_test_??_base, then there is probably an issue in that category of tests.

There are two failing tests here:

• In base tests. HaystackHttpExtractorTest
• In instrumentation tests. ProxyRequestHelperTest

[Xyaren]

I see.
The Problem is around the Haystack Span,Trace and Parent Id, which is handled as baggage.

https://github.com/Xyaren/dd-trace-java/blob/c99bec69e0a7f72ecc427e0ecb19c877e8bacf81/dd-trace-core/src/main/java/datadog/trace/core/propagation/HaystackHttpCodec.java#L204-L214

This is only in the special case, when the haystack trace Id is zero, as this would normally result in no trace being created.
Due to the addition of haystack trace (and span) ids to the baggage map, this would result in a new context despite a 0 trace id.

I feel like this would still qualify for creating a new trace context including the extracted baggage of the zero-only haystack ids. Thats in fact what was correctly extracted, so why not keep it and propagate it ?

Alternatively one could invalidate the context similar to the case when the trace id is negative:

Another option is to remove the baggage after extraction, but this would require some additional mechanism for HttpCodec.Extractor Implementations to "cleanUp" the baggage after extraction in TagContextExtractor.extract.

I could also split the baggage into two separate maps i.e.: predefinedBaggage & baggage, so the check in the ContextInterpreter.build() can evaluate if the baggage is sourced from ot-*/mapped baggage or predefined like the haystack ids.

What do you prefer ?

[Xyaren]

ProxyRequestHelperTest#Test baggage headers to be correctly ignored through proxy
Is probably obsolete after this PR, as it tests exactly the opposite of what this PR is trying to achieve: Creating a new context when baggage is received.

My questions are:

• Does a received trace id of 0 represent a special case ? If so, what is it ?
  I've noticed we're still considering mapped tags (from headers), so the same could apply to baggage.
• Should ot-* baggage be propagated next to configured mapped baggage as well when received without a trace id?

[bantonsson]

@Xyaren If we receive a traceId of 0 we interpret that as no ingoing trace, so we will create a new Trace starting here. I think it's fine to always propagate the baggage, and you don't need to split up the different baggage types. The test makes the wrong assumptions and instead it should check that the baggage contains the expected items, something like this:

    ...
    baggage = [ ... ]

    when:
    final TagContext context = extractor.extract(headers, ContextVisitors.stringValuesMap())

    then:
    if (expectedTraceId) {
      ExtractedContext ec = context as ExtractedContext
      assert ec.traceId == expectedTraceId
      assert ec.spanId == expectedSpanId
    }
    context.baggageItems() == baggage
    ...

[Xyaren]

@bantonsson Thank you for the explanation.
I decided to split the test up and added one explicitly for the baggage propagation.

[bantonsson]

@Xyaren I think you need to update this PR since there is now a conflict with your changes from #4513

[Xyaren]

Rebased & Squashed 👍