[Feature]: Move SpanLimits to opentelemetry and use the limits in SpanBuilder

[mladedav]

Related Problems?

The tracing-opentelemetry library uses SpanBuilder to create spans and puts events in there, but if there are more than the configured limit, these are kept around, ignored by Tracer when a span is actually built and then discarded.

This can be an issue especially for long-running spans with much larger number of events than the limit as they are effectively just leaking memory.

Describe the solution you'd like:

SpanBuilder can already take the limits into account. It is built by a Tracer which holds the limits internally so it can configured the builder.

Considered Alternatives

A provided method could also be added to the Tracer trait to expose the limits, such as fn span_limits(&self) -> Option<SpanLimits> { None } where implementations can override that with their actual limits, if they implement them. Then a library such as tracing-opentelemetry can discard the events as they come.

I think it would be better for to keep the filtering inside opentelemetry-rust itself. But if there is a desire to keep SpanBuilder as simple as possible, or breaking changes to it would be undesirable, this could also be an option.

Additional Context

Breaking Changes

Part of this would be moving SpanLimits into opentelemetry, which could be done in non-breaking way with a reexport from opentelemetry-sdk.

Changing the internals of SpanBuilder would be a breaking change since all its fields are publicly available.

Adding a method with a default implementation to the trait should be non-breaking, if the name does not clash with other methods on implementing types or traits they implement.

Runtime Changes to SpanLimits

According to the spec, the span limits configuration must be owned by the provider and the sdk may proivde a way to change them during runtime, where the change is visible in all tracers including those created before the change.

However, I have not found any information about when the limits should be applied, i.e. if the change should also be visible in already created spans or if limiting in helper structures such as the SpanBuilder is even permitted. This proposal would use the limits present at the time of SpanBuilder creation and also at the time of building the span, effectively taking the lower of the two. Currently, only the latter is applied.

Note that opentelemetry-rust does not allow chaning the limits after a TracerProvider is built, so this section is here only so that this proposed change does not prevent future improvements.

API vs SDK SpanLimits

SpanLimits are defined in OpenTelemetry trace SDK, not API, so exposing it might be a no-go based on that and considerations of #1042

[cijothomas]

The general rule is - API (i.e OpenTelemetry crate) should not do any enforcement of limits, and it should be the responsibility of the SDK (opentelemetry-sdk crate). Unfortunately, in many places we have violated that policy - example in logs #1189
example in metrics - #1508 (that got reverted)
another example : #1269

Related discussion : #1043 (comment)

Despite some special handling done in the sdk in this repo to accomodate tracing,tracing-opentelemetry scenarios, that part is not fully sorted out. We need to fix them (i.e tracing and otel should interop smoothly) surely, and we will fix them. I don't have a firm ETA as we are lacking manpower :(....

[mladedav]

Ok, I would like to help in any way I can.

If API cannot enforce the limits, can the limits at least be exposed then or is the configuration also something that should not be part of the API?

If so I think there is no way for open telemetry to accommodate tracing's strategy of creating otel spans only after the span has been closed and I'll try to see if I can make some effort there for better interoperability.

[mladedav]

I've read more through the discussions in the links you\ve provided (thanks for that by the way!) and now I think this won't be necessary to make the limits work after all.

tracing-opentelemetry already depends on opentelemetry_sdk so it might be cleanest for now to just use that dependency more instead of forcing this opentelemetry implementation to deviate more from the standard.