Issue 7869 - Support the new W3C random flag

[PeterF778]

Adding a new bit definition to TraceFlags.
Adding a method to IdGenerator to declare randomness of the generated trace-ids. Passing the correct TraceFlags to the configured sampler for root spans. Modifying and adding unit tests.

[codecov]

Codecov Report

❌ Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.18%. Comparing base (8e332fc) to head (e1785c0).
⚠️ Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
...in/java/io/opentelemetry/api/trace/TraceFlags.java	80.00%	1 Missing ⚠️
...n/java/io/opentelemetry/sdk/trace/IdGenerator.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8012      +/-   ##
============================================
+ Coverage     90.15%   90.18%   +0.03%     
- Complexity     7476     7493      +17     
============================================
  Files           834      836       +2     
  Lines         22540    22590      +50     
  Branches       2236     2239       +3     
============================================
+ Hits          20320    20372      +52     
  Misses         1515     1515              
+ Partials        705      703       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jack-berg]

@carlosalberto if you're more in the loop on level 2 span context, any chance you could give this a look over as a sanity check?

[jmacd]

I see this is private. I'm aware of an underspecified region in the OTel specification. Should there be a specified way to create these "primordial" contexts with control over the root trace state?

In the Go SDK, asking for a new root gets you an empty context: https://github.com/open-telemetry/opentelemetry-go/blob/8d3b4cb2501dec9f1c5373123e425f109c43b8d2/sdk/trace/tracer.go#L92

It's not clear whether users are able to setup a context with control over TraceState at the root. We write:

Root Samplers MAY insert an explicit randomness value into the
OpenTelemetry TraceState value in cases where an explicit
randomness value is not already set.

It doesn't actually require SDKs provide a way to set explicit randomness for the root span, it just refers to the potential scenario.

[jack-berg]

Should there be a specified way to create these "primordial" contexts with control over the root trace state?

I was thinking something similar. Specifically, its weird that the primordial context always has the random trace bit set to true, even if paired with an IdGenerator which does not generate random trace ids.

[PeterF778]

In theory, the "primordial" context can have non-empty TraceFlags, and non-empty TraceState (which we do not use yet), while still remaining "invalid", forcing the "child" span to be root. Hence I opted for passing both TraceFlags and TraceState as arguments for this method, just to emphasize this point.

It is not clear to me, if the IdGenerator can be changed dynamically, but if it can, I believe this case will be handled properly (i.e. the primordial context to use will have the RandomTraceId on or off, depending on the IdGenerator). If a non-random IdGenerator is use, the RandomTraceId flad will not be set.

[carlosalberto]

@jack-berg Will review by EOD today. Thanks for the heads-up!

[jack-berg]

This reads as a near copy of the TraceFlags.withRandomTraceIdBit() and TraceFlags.withSampledBit() methods, and could go away if those methods accepted a boolean parameter:

  default TraceFlags withSampled(boolean isSampled) {
    byte newByte =
        isSampled
            ? (byte) (asByte() | ImmutableTraceFlags.SAMPLED_BIT)
            : (byte) (asByte() & ~ImmutableTraceFlags.SAMPLED_BIT);
    return ImmutableTraceFlags.fromByte(newByte);
  }

  default TraceFlags withRandomTraceId(boolean isRandomTraceId) {
    byte newByte =
        isRandomTraceId
            ? (byte) (asByte() | ImmutableTraceFlags.RANDOM_TRACE_ID_BIT)
            : (byte) (asByte() & ~ImmutableTraceFlags.RANDOM_TRACE_ID_BIT);
    return ImmutableTraceFlags.fromByte(newByte);
  }

[jack-berg]

Taking another look at this, I think these need to be static helpers with signatures of the form static TraceFlags with{Param}(TraceFlags traceFlags, boolean {param}) { ...}.

The instance levels make for nice UX, but are overridable, which I believe is never needed and would lead to bad / hard-to-debug behavior.

[PeterF778]

Yes, we do not want them to be overridden. I definitely considered the static methods, and ultimately opted out of them: the usage is less convenient, and even though TraceFlags is an interface, it is really tightly coupled with ImmutableTraceFlags (see fromHex(), and fromByte()), so in practice developing a different implementation would be very inconvenient, if not impossible.

Another thing is about being able to reset some bits. I consciously did not provide this functionality. W3C Trace Context specification effectively says that any set bit from the upstream TraceFlags has to be cleared if the current implementation does not recognize it. I read this as a requirement to always build TraceFlags from scratch. Providing the capability to take (potentially unknown) TraceFlags and clear selected bits could confuse the users into thinking that they can do this with incoming TraceFlags.

[jack-berg]

If I'm reading this right, rootContextWithRandomTraceIdBit can be private static final singleton, since there's no parameters which are dependent on this constructor's params, and the instance itself is immutable and thread safe.

I've sketched this (and my other suggestions) out in this commit to improve clarity: c77bad6bb

[PeterF778]

yes, it should static - thanks!