feat(opentelemetry-sdk): add a constructor for SpanEvents and SpanLinks #2934
Conversation
# Changes Add public constructors on SpanEvents and SpanLinks structs. # Motivation Even though all fields are public, because of the `#[non_exhaustive]` annotation on `SpanEvents`and `SpanLinks`, it's not possible to construct instances of these outside of the `opentelemetry-sdk` crate. Since they are used in `export::SpanData`, it is problematic because it means there is no way to create span data containing any events or span links. So there is no way to write tests on how to these fields are processed in external crate that implement the SpanExporter or SpanProcessor traits.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2934 +/- ##
=======================================
- Coverage 81.3% 81.3% -0.1%
=======================================
Files 126 126
Lines 24254 24266 +12
=======================================
Hits 19736 19736
- Misses 4518 4530 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| impl SpanLinks { | ||
| /// Create a new `SpanLinks` from a list of events and a dropped count |
There was a problem hiding this comment.
is it only required for tests?
There was a problem hiding this comment.
No, I need it for tests, but there are definitely wider use cases.
For instance you could imagine span processors or span exporters trying to create "synthetic spans" and put span links and span events on these synthetic spans.
This currently doesn't work, and I can't think of a good rational as to why creating SpanData without span events should not be possible, and not with them.
There was a problem hiding this comment.
This currently doesn't work, and I can't think of a good rational as to why creating SpanData without span events should not be possible, and not with them.
It feels like there is one too many negations here. I assume you're saying that if it's possible to create SpanData without SpanLinks and SpanEvents, then it should be possible to create SpanData with them.
There was a problem hiding this comment.
I wonder if they should not be builders. The reason being that I believe we had intentionally made them non_exhaustive to prevent api breakage but if we go with simply a new that would then potentially require creating a new with breaking changes in the future.
What do you think @cijothomas ?
There was a problem hiding this comment.
Well actually I was thinking of closing this PR because I found a way to get around this.
The main reason I was proposing this is I have a function like fn transform_span(s: SpanData) -> InternalSpan with complex business logic which I'd like to write unit tests for.
What I ended up doing is creating a struct mirroring SpanData in my crate, which I can construct. so now I do
pub(crate) struct ExportSpan {
pub span_context: opentelemetry::trace::SpanContext,
pub parent_span_id: opentelemetry::trace::SpanId,
pub span_kind: opentelemetry::trace::SpanKind,
pub name: Cow<'static, str>,
pub start_time: SystemTime,
pub end_time: SystemTime,
pub attributes: Vec<opentelemetry::KeyValue>,
pub dropped_attributes_count: u32,
pub events: Vec<Event>,
pub dropped_event_count: u32,
pub links: Vec<opentelemetry::trace::Link>,
pub dropped_links_count: u32,
pub status: opentelemetry::trace::Status,
pub instrumentation_scope: opentelemetry::InstrumentationScope,
}
fn export_span(s: SpanData) -> ExportSpan; // 1:1 mapping, no tests needed
fn transform_span(s: ExportSpan) -> InternalSpan; // complex logic, but I can create ExportSpan structs for tests
5 participants
Even though all fields are public, because of the
#[non_exhaustive]annotation onSpanEventsandSpanLinks, it's not possible to construct instances of these outside of theopentelemetry-sdkcrate.Since they are used in
export::SpanData, it is problematic because it means there is no way to create span data containing any events or span links. So there is no way to write tests on how to these fields are processed in external crate that implement the SpanExporter or SpanProcessor traits.Changes
Add public constructors on SpanEvents and SpanLinks structs.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes