open-telemetry
SpanBuilder should not allow with_status
Similar to https://github.com/open-telemetry/opentelemetry-rust/issues/2743
I am not sure what is the need of with_status in the SpanBuilder. SpanBuilder is used to create a new Span, at which point the status should be unknown. It totally is valid to have this API on the Span itself as that'll be called only after a Span has been created.
Used by tracing-opentelemtry here
(not exactly the method but I assume you are referring to the abilty to set the status field in SpanBuilder)
Yes, it is similar to https://github.com/open-telemetry/opentelemetry-rust/issues/2753
We cannot stabilize Otel Tracing API with these APIs exposed. (A lot of special casing to support tracing-opentelemetry actually).
Hopefully @bantonsson's work will allow us to trim public APIs.
Let's keep this issue in a blocked state until then? IIRC we removed some API tracing-opentelemetry used and had to add it back. Want to avoid that until we have a good replacement in place
Yes okay to keep them now to not break tracing-opentelemetry. But I don't think we can delay 1.0 indefinitely either. Need to see if tracing-opentelemetry maintainers can commit a timeframe before tracing-opentelemetry can be modified to not depend on these APIs.
The function @TommyCpp highlighted in SpanBuilderUpdates::update that uses with_status is used in a couple flows (below).
The underlying pattern is that OTel's SpanBuilder is used as a scratch space as telemetry is pushed through tracing-opentelemetry - this extends not just to the status, but to a bunch of other fields, all visible in SpanBuilderUpdates::update - name, kind, status, attributes. Then the builder is started and dropped immediately to trigger submission when the telemetry is closed (@bantonsson 's favourite bit!)
Usages
1. When a new span is created (OpenTelemetryLayer::on_new_span)
https://github.com/tokio-rs/tracing-opentelemetry/blob/b6701c1f971e1401d91ef7116fd4cf92c9c35989/src/layer.rs#L934
2. When span values are recorded on an existing span via OpenTelemetryLayer::on_record
https://github.com/tokio-rs/tracing-opentelemetry/blob/b6701c1f971e1401d91ef7116fd4cf92c9c35989/src/layer.rs#L984
3. When event data is added to an existing span via OpenTelemetryLayer::on_event
https://github.com/tokio-rs/tracing-opentelemetry/blob/b6701c1f971e1401d91ef7116fd4cf92c9c35989/src/layer.rs#L1098
Alternatives
Doesn't look too difficult to change the tokio-tracing side to avoid the need to mutate the SpanBuilder repeatedly. We could add a status to OtelData:
https://github.com/tokio-rs/tracing-opentelemetry/blob/b6701c1f971e1401d91ef7116fd4cf92c9c35989/src/lib.rs#L145
... modify OpenTelemetryLayer::update to mutate this, which implies changing the callers above so they pass through this instead of just the SpanBuilder, and then simply modify OpenTelemetryLayer::on_close to set the status with Span::set_status.
I've hacked this up and its pretty straightforward if kind of a noisy change. Not sure there's much value in doing this at this point, though, as @bantonsson 's changes to make the context activation work properly will necessarily change this anyway!
Actually it's a bit more nuanced whether we should change this, or not - is @bantonsson 's "new bridge work" going to necessarily land in tracing-opentelemetry? I don't think that's obvious yet. If it doesn't we'd want to see if we can do something like what i've described above there to move off of this pattern of SpanBuilder and in particular status mutation.