Update Go guidance for attributes #3927
Conversation
Co-authored-by: Cijo Thomas <[email protected]>
There was a problem hiding this comment.
This is the way for for users would like to reduce the amount of heap allocations: https://cloud-native.slack.com/archives/CJFCJHG4Q/p1706821596412959?thread_ts=1706816394.957449&cid=CJFCJHG4Q
CC @MrAlias
| are useful for aggregating, filtering, and grouping traces. Attributes can be | ||
| added at span creation, or at any other time during the lifecycle of a span | ||
| before it has completed. | ||
|
|
||
| Generally speaking, you should try to set attributes during span creation. This | ||
| will incur less overhead compared to setting attributes after span creation, and |
There was a problem hiding this comment.
This
will incur less overhead compared to setting attributes after span creation
^ Is this actually true? I think the answer is - it depends.!
If the sampler is ParentBased and the parent's decision is to Drop, then providing all attributes at startup time is wasted as the span is going to be dropped anyway...
If user choses to add it after span creation, he/she can avoid the cost of representing attributes completes, by short-circuiting if span is not recording.
The only reason spec recommended providing attributes at creation time is for the trace-sampler to access it, not perf. If Golang implementation makes it more performant, then this section makes sense - but I wanted to double check if that is indeed true.
There was a problem hiding this comment.
This is the justification: https://cloud-native.slack.com/archives/CJFCJHG4Q/p1706816394957449 (SetAttributes also takes a variadic argument). My question would be if the Go compiler optimizes the single value case though.
Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
|
While I have nothing against the changes I just want to point out that in my opinion the description is misleading
See: #3927 (review) |
|
@cartermp @open-telemetry/go-approvers is this PR something we want to continue? |
|
The content and title LGTM. However, I think that the description is not true. CC @MrAlias |
@cartermp does this help you to finalize this PR? |
|
gonna close this one out, it's been rather long |
In service towards https://cloud-native.slack.com/archives/CJFCJHG4Q/p1706816394957449