-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Remove Span.SetAttribute #302
Comments
In OpenTelemetry, we have stated a desire to avoid performance penalties especially when there is a no-op implementation in use. The specification language that justifies this decision is "It is also important that minimal implementation incurs as little performance penalty as possible". |
I've made a simple benchmark comparing using Basically, It sets 4 attributes (one by one) using each function. Here is the benchmark running on my machine:
Each call adds an additional alloc and approximately 75~225ns. IMO, that is a small penalty that I think is worth taking off to have a clear and smaller API. I'm in favor of removing this singular sets all over the API, as @rakyll and @freeformz has already spoken on this and other issues. Although, I'm okay the SDK using this kinda of optimization in its internals. ps: After we have a decision here, we should probably extend this for the rest of the API. For example, the Trace API have |
100-200ns is what you see in a microbenchmark. What you'll see in real applications is additional garbage collection cost, which has a non-local, non-linear impact. I return to "It is also important that minimal implementation incurs as little performance penalty as possible". Even a no-op implementation bears this cost. I agree that AddLink doesn't have a corresponding plural version, but I also suspect a call to support adding multiple links will have ~0 users. Adding a single attribute should be a common operation. |
Agreed that the impact is higher than what the benchmark shows and I understand your position with the library guidelines. But there is not doubt that this "duplicated" method pollutes the At the very minimum we should better document these methods and their tradeoffs. About the |
I will not object any further to removing SetAttribute. If there is a serious performance complaint, perhaps it can be re-introduced. (For me, somewhat of a grammar nerd, using a plural "attributes" when there is only one is a bit off-putting. That's a separate matter.) |
I think we both made our points about this, would be great to have input from others as well and get to a consensus. |
IIRC we reached consensus about removing this in last Thursday's SIG meeting. To @jmacd's point, we can always add the singular version back in later! |
Is this up for grabs? I'll like to try my hands at it |
…/google.golang.org/grpc (#302) * Bump google.golang.org/grpc in /instrumentation/google.golang.org/grpc Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.31.0 to 1.31.1. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.31.0...v1.31.1) Signed-off-by: dependabot[bot] <support@github.com> * Auto-fix go.sum changes in dependent modules Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Span.SetAttributes(...core.KeyValue) already does this if you pass a single argument. In Go, we don't provide too many options for the same functionality and variadic args are used exactly to avoid necessity to provide two methods.
The text was updated successfully, but these errors were encountered: