Let LogDataBuilder take a SpanContext directly.

[breedx-splk]

The underlying type is SpanContext, so it is more straightforward to allow the user to just pass one in instead of jumping through Context hoops (there's no type relationship between those two classes).

This change still supports passing in the Context, but it is a breaking change because the name was changed to make it more explicit. I assume this is ok because logs are alpha, but if we want to provide a non-breaking deprecation path I can add that too...

I didn't propagate this up to SdkLogBuilder or the LogBuilder interface, but it might make sense there too?

[codecov]

Codecov Report

Merging #3942 (546c730) into main (ec45e94) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #3942      +/-   ##
============================================
+ Coverage     89.72%   89.85%   +0.13%     
- Complexity     4163     4201      +38     
============================================
  Files           498      503       +5     
  Lines         12646    12739      +93     
  Branches       1221     1230       +9     
============================================
+ Hits          11346    11447     +101     
+ Misses          906      900       -6     
+ Partials        394      392       -2     

Impacted Files	Coverage Δ
...io/opentelemetry/sdk/logs/data/LogDataBuilder.java	100.00% <100.00%> (ø)
.../api/baggage/propagation/W3CBaggagePropagator.java	81.63% <0.00%> (-8.37%)	⬇️
...io/opentelemetry/sdk/metrics/SdkMeterProvider.java	79.66% <0.00%> (-3.39%)	⬇️
...xporter/otlp/internal/grpc/OkHttpGrpcExporter.java	82.47% <0.00%> (-1.40%)	⬇️
...xporter/otlp/internal/grpc/ManagedChannelUtil.java	86.27% <0.00%> (-1.00%)	⬇️
...toconfigure/OpenTelemetrySdkAutoConfiguration.java	61.90% <0.00%> (-0.89%)	⬇️
...io/opentelemetry/api/baggage/ImmutableBaggage.java	100.00% <0.00%> (ø)
...opentelemetry/api/metrics/GlobalMeterProvider.java	100.00% <0.00%> (ø)
...telemetry/sdk/metrics/SdkMeterProviderBuilder.java	96.66% <0.00%> (ø)
...lemetry/sdk/metrics/internal/state/DebugUtils.java	100.00% <0.00%> (ø)
... and 28 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec45e94...546c730. Read the comment docs.

[jkwatson]

I don't think that's the purpose of this method. There could be other things in the context to be extracted into the logs in the future...baggage, for instance. This method should still be called setContext().

[anuraaga]

Yeah - Span is similar, but we only have setParent(Context) since we expect Context to be what's propagated and don't want to miss baggage. Currenty we're hoping that the OTel appender can always be added on the app thread (not under an async one) so it has access to the full Context automatically - so for now we don't even call this in the appender

open-telemetry/opentelemetry-java-instrumentation#4375 (comment)

Are you running into code where you need to create a SpanContext?

[breedx-splk]

Totally makes sense that we'd want the ability to propagate things other than just span context...but I'm guessing that in the short term span context would be the primary use case. Generically supporting context propagation (and baggage) in the future makes sense...

The implementation (which I didn't modify) literally creates a Span in order to get a SpanContext because that's the type contained in this builder. I guess that would also need to change down the road.

I will change the comment and rename to just setContext().

[breedx-splk]

Are you running into code where you need to create a SpanContext?

Yeah, in my specific case we already have a span context that we need to propagate into logs, but prior to this PR we have to jump through the clumsy Context.root().with(Span.wrap(spanContext)) incantation to make it happen.

[anuraaga]

Yeah, in my specific case we already have a span context that we need to propagate into logs

Is there a reason it's not already in a Context? Mainly I'm trying to understand if there's a logs-specific reason we need a setSpanContext on logbuilder, despite not having it on spanbuilder. In apps using OTel, we usually expect SpanContext to be short lived, it gets filled into a Context and made current basically right away.

[anuraaga]

/cc @tigrannajaryan In case you're interested in this discussion of setContext vs setSpanContext in logs SDK

[breedx-splk]

Is there a reason it's not already in a Context?

(Some handwaving here) We have a mechanism that is effectively stitching together span context (traceId, spanId, flags) from an "out of band" source. That is, as we're exporting logs, we stitch in the span was in scope at the time the log was created, but we kinda don't know until export time.

We use an instance of SpanContext to hold these details, and we don't bother wrapping it up in a Context because it's just an unnecessary layer (and creating a Context from just the raw span context n-tuple {traceId,spanId,flags} with Context.root().with(Span.wrap(spanContext)) feels overcomplicated to me). We could have used Context for this, but it seems unnecessary when LogData has a SpanContext directly in it.

I don't think there necessarily needs to be an equivalent method on SpanBuilder because spans and span context are conceptually intertwined, but logs are a different beast. Seems like the current implementation of logs is missing the more generic Context+Baggage support that we think we'll want one day (the spec only barely mentions it).

[anuraaga]

It sounds like there is a logs-specific use case for creating span context out of band so this seems fine for me and can always check back later