Add a set_trace_context method to LogRecord trait

[KodrAus]

Fixes #2106

Changes

This is a breaking change.

This PR moves the opentelemetry_sdk::logs::TraceContext into opentelemetry::logs::TraceContext so we can add a LogRecord::set_trace_context(&mut self, trace_context: TraceContext) method.

To make this work, I needed to make the TraceContext type depend on trace APIs, which are only conditionally available. I followed the pattern set by SpanContext by hiding the fields and exposing them via methods. These methods and the TraceContext constructor are only available when the trace feature is enabled. I thought this was better than making the set_trace_context method conditional, because conditional trait methods are a bit of a hazard for consumers to deal with.

Merge requirement checklist

• CONTRIBUTING guidelines followed
• Unit tests added/updated (if applicable)
• Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
• Changes in public API reviewed (if applicable)

[codecov]

Codecov Report

Attention: Patch coverage is 89.24731% with 10 lines in your changes missing coverage. Please review.

Project coverage is 79.0%. Comparing base (9cbf693) to head (ab1de99).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
opentelemetry/src/logs/record.rs	0.0%	10 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #2129   +/-   ##
=====================================
  Coverage   79.0%   79.0%           
=====================================
  Files        121     121           
  Lines      20856   20945   +89     
=====================================
+ Hits       16482   16561   +79     
- Misses      4374    4384   +10     

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

[lalitb]

Will providing a method (say):

fn set_trace_context(&mut self, trace_id: TraceId, span_id: SpanId, trace_flags: Option<TraceFlags>);

prevent moving the TraceContext to the API, and keep API surface minimal?

[KodrAus]

We wouldn’t be able to do this without making that method conditional on #[feature = “trace”], since that’s where TraceId etc are defined.

[lalitb]

Do you foresee any concerns with making it conditional, as the method serves no purpose without the trace feature enabled? Alternatively, should we pass Context as a parameter, allowing the user to embed TraceContext within it?

[KodrAus]

Conditional trait methods without a default body are a hazard, but ones with a default body are generally ok.

The issue with non-default conditional trait methods is that the crate you implement it in is not the only crate that can enable features, so you can end up with impossible builds where an unrelated dependency causes you to need to implement a method you didn't have.

Default conditional trait methods are ok, because you only opt-in to implementing them when you have something meaningful to implement. Otherwise you don't have to worry about them. The drawback of this approach compared to non-conditional trait methods with arguments that may be conditionally constructible is that the method itself becomes less reliable. If, for example, you're writing an implementation that forwards to another one there may be some methods that don't forward, because they're using the default that throws the argument away.

If you'd prefer, I can make the method itself conditional and give it an empty default body, then move TraceContext back into the SDK. That sets out a bit of a precedent for evolving the trait too once it's stable and anything we add to it needs to have a default body.

[lalitb]

Thanks for explanation. I agree conditional trait is not good to have.

If you'd prefer, I can make the method itself conditional and give it an empty default body, then move TraceContext back into the SDK. That sets out a bit of a precedent for evolving the trait too once it's stable and anything we add to it needs to have a default body.

Yes, this would be the good approach.

[KodrAus]

It’s been a full week but I should be able to pick this up again. Are we happy to:

1. Make this method conditional on the trace feature with an empty default body.
2. Make it take TraceId, SpanId, and TraceFlags instead of TraceContext so it doesn’t have to be pulled into the core crate.

If so, I’ll make that change.

[lalitb]

It’s been a full week but I should be able to pick this up again. Are we happy to:

1. Make this method conditional on the trace feature with an empty default body.
2. Make it take TraceId, SpanId, and TraceFlags instead of TraceContext so it doesn’t have to be pulled into the core crate.

If so, I’ll make that change.

Thanks @KodrAus . That looks good to me. Just to clear this would also mean - Make TraceId, SpanId, TraceFlags (and possibly few other) not conditional on the trace feature.

[KodrAus]

Just to clear this would also mean - Make TraceId, SpanId, TraceFlags (and possibly few other) not conditional on the trace feature.

Ah, I thought making the method conditional on trace would allow us not to make TraceId et al. non-conditional. If you'd prefer, I can go down that route and make those types non-conditional, and make the method non-conditional too, but still with a default empty body.

[lalitb]

Ah, I thought making the method conditional on trace would allow us not to make TraceId et al. non-conditional. If you'd prefer, I can go down that route and make those types non-conditional, and make the method non-conditional too, but still with a default empty body.

Sorry for the confusion. I think this will do as you suggested earlier:

• Make this method conditional on the trace feature with an empty default body.
• Make it take TraceId, SpanId, and TraceFlags instead of TraceContext so it doesn’t have to be pulled into the core crate.

[KodrAus]

Ok, I've updated the PR. It's now a much smaller change than it was before.

[cijothomas]

LGTM, with a changelog entry.

[lalitb]

LGTM. Thanks