-
Notifications
You must be signed in to change notification settings - Fork 972
Refine guidance on providing attributes at span creation time #4879
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -400,9 +400,20 @@ The API MUST accept the following parameters: | |
| description](sdk.md#sampling). An empty collection will be assumed if | ||
| not specified. | ||
|
|
||
| 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 | ||
| to [`Sampler`s](sdk.md#sampler) when making sampling decisions. | ||
| The API documentation MUST state that: | ||
|
|
||
| - Providing readily-available, low-cost attributes at span creation time | ||
| enables head-based samplers that rely on attributes (e.g., custom or | ||
| rule-based samplers) to make informed sampling decisions. | ||
| - 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Depends! 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).
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agree. The ones recommended by SemConv to be provided at startup is quite reasonable and readily available ones only. |
||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just to rope @jmacd into the discussion 😄
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! |
||
|
|
||
| - `Link`s - an ordered sequence of Links, see [API definition](#link). | ||
| - `Start timestamp`, default to current time. This argument SHOULD only be set | ||
|
|
@@ -515,7 +526,9 @@ attributes"](https://github.com/open-telemetry/semantic-conventions/blob/main/do | |
|
|
||
| Note that [Samplers](sdk.md#sampler) can only consider information already | ||
| present during span creation. Any changes done later, including new or changed | ||
| attributes, cannot change their decisions. | ||
| attributes, cannot change their decisions. See | ||
| [Span Creation](#span-creation) for guidance on when to provide attributes at | ||
| creation time versus setting them later. | ||
|
|
||
| #### Add Events | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.