Refine guidance on providing attributes at span creation time#4879
Refine guidance on providing attributes at span creation time#4879cijothomas wants to merge 1 commit intoopen-telemetry:mainfrom
Conversation
| them for spans that are ultimately dropped is wasted. Instrumentations | ||
| MAY defer expensive attribute computation until after span creation, | ||
| guarded by [`IsRecording`](#isrecording), to avoid unnecessary overhead | ||
| when spans are not being recorded. |
There was a problem hiding this comment.
+1 on encouraging instrumentation providers to use judgement on when span attributes are provided rather than assuming that eagerly provided attributes are always preferable. In core .NET libraries we often concluded that eagerly provided attributes are not preferred. .NET tries to encourage "pay for play" performance characteristics - we want only the users who enable a feature to pay its performance costs whenever possible.
There was a problem hiding this comment.
That "lazy" thing is something I am exploring in OTel Rust. The instrumentations in Rust are extremely bad in terms of performance and a lot of it is due to eagerly passing attributes upfront!
| The API documentation MUST state that adding attributes at span creation is preferred | ||
| to calling `SetAttribute` later, as samplers can only consider information | ||
| already present during span creation. | ||
| Attributes provided at span creation time are the only attributes available |
There was a problem hiding this comment.
TODO: see if https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-spans.md needs tweaking as well, as it has SHOULD on providing a set of attributes at Span creation time.
| - Collecting attribute values can be expensive (e.g., URL parsing, string | ||
| formatting). When the configured `Sampler` does not consider attributes — | ||
| as is the case for all built-in samplers — the cost of eagerly computing | ||
| them for spans that are ultimately dropped is wasted. Instrumentations |
There was a problem hiding this comment.
I believe it's common in semconv that sampling-relevant attributes are the same that you'd use on metrics, so not collecting them only helps when metrics are disabled
There was a problem hiding this comment.
Depends!
For duration metric, which is the most common one - that is reported when request is ending only. SpanCreation attribute are at request start time.
Also, in languages like Rust/,NET, metric attributes can be provided via stack allocated arrays, but Span/Trace requires ownership (i.e Heap allocated vectors/arrays/Tags).
Something we are trying to improve in parallel.
There was a problem hiding this comment.
My goal is not to drastically change the guidance, but add some note like "Use judgement when providing all attributes eagerly vs after-sampling-decision-is-known"
So that .NET (and upcoming Rust) implementations are not considered non-compliant with the OTel semantic conventions)
There was a problem hiding this comment.
they would not be strictly compliant because conventions define which attributes SHOULD be provided at start time https://github.com/open-telemetry/semantic-conventions/blob/99f629fec1a9e14a43a33ba8cc5ca7987e7c13f8/docs/http/http-spans.md?plain=1#L300-L306
I don't mind relaxing guidance here for manual, non-semconv instrumentations, but we already applied judgement in semconv for common ones.
There was a problem hiding this comment.
agree. The ones recommended by SemConv to be provided at startup is quite reasonable and readily available ones only.
|
This PR was marked stale. It will be closed in 14 days without additional activity. |
4 participants
Changes
The current spec gives a blanket preference for providing attributes at span creation for samplers. However, no built-in sampler actually examines attributes — AlwaysOn, AlwaysOff, TraceIdRatioBased, ParentBased, and ProbabilitySampler all ignore them. Eagerly collecting attributes (URL parsing, string ops, Vec/array allocation) for spans that get dropped is wasted work.
This PR replaces the blanket preference with balanced guidance: provide cheap/readily-available attributes at creation (to support custom head-based samplers), but defer expensive attribute computation until after creation, guarded by IsRecording().
.NET instrumentations already follow this guidance - they don't provide attributes at startup time unless they are readily available. I am now adding Rust instrumentations where the costs of providing attributes before sampling is quite high. Hence trying to see if we can balance the wording in the spec.
For non-trivial changes, follow the change proposal process.
CHANGELOG.mdfile updated for non-trivial changes[chore]in the PR title to skip the changelog check