feat(attributes) bump semconv to 1.26.0 #6172
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6172 +/- ##
=======================================
+ Coverage 66.9% 67.0% +0.1%
=======================================
Files 193 194 +1
Lines 15652 15695 +43
=======================================
+ Hits 10482 10528 +46
+ Misses 4880 4878 -2
+ Partials 290 289 -1
|
…try-go-contrib into otelmongo#6171
prestonvasquez
changed the title
|
Updating semconv is definitely not just a chore, especially since, as you mention, there are field name changes (which can cause issues with users, since they need to change their queries and dashboards). We should either mark these breaking changes explicitely in a changelog entry, or follow a similar migration path as the otelhttp instrumentation is doing: #5132 |
@open-telemetry/specs-semconv-maintainers, @open-telemetry/semconv-db-approvers, do you have some recommendations or feedback regarding changes of database semantic conventions? Are we supposed to do some migrations in instrumentation libraries like for HTTP instrumentations? CC @XSAM as you are the maintainer of https://github.com/XSAM/otelsql which AFAIK is the most popular instrumentation library for |
prestonvasquez
changed the title
|
check out https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database/database-spans.md:
|
…try-go-contrib into otelmongo#6171
Co-authored-by: Damien Mathieu <[email protected]>
@pellared Could you clarify this? |
…try-go-contrib into otelmongo#6171
would this go against the semconv recommendation?
|
|
@prestonvasquez https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database/database-spans.md
|
@trask Would you mind reading the PR description and letting me know if the proposal is in line with the spirit of this requirement? |
You would need to address it in https://github.com/open-telemetry/semantic-conventions. This does not prevent bumping semconv to |
my recommendation would be to stick to what's defined in the semantic conventions if you're interested, there was a lot of discussion around the
I'm happy to discuss if you open an issue in https://github.com/open-telemetry/semantic-conventions describing the limitations you are running into with the existing |
What is the purpose of doing this? It seems like this will maintain the existing behavior. |
|
Yes, this would be a noop change. It would (a bit virtually) reduce the scope of the upgrade to 1.26 though, as a single version bump would then be required. |
@dmathieu In this case, should we just close this PR and leave the semconv dependency as-is for now? |
|
Sure, if you think there's still too much work for this PR. Maybe document the outcome from the discussion that happened here in #6171? |
|
@dmathieu I think we should maintain 1.17.0 since upgrading to 1.24.0 has no obvious benefit. However, I'm happy to open a separate PR for 1.24.0 if you want. I would suggest not using the pattern for a non-breaking upgrade, though. |
Like bumping a dependency - we are closer to the latest version. |
|
Also, folks using the package will have less surprises seeing a newer version in their dependency tree rather than something older that looks like it's not being cared for. |
|
@prestonvasquez, I changed it to draft as it looks like it is still WIP. Please change to ready for review once applicable. |
| attrs = appendNetworkPort(attrs, port) | ||
| attrs = appendNetworkHost(attrs, hostname) | ||
| attrs = appendNetworkAddress(attrs, net.JoinHostPort(hostname, strconv.Itoa(port))) | ||
| attrs = appendNetworkTransport(attrs) |
There was a problem hiding this comment.
Rather than creating one method for each attribute, how about one method which returns all attributes, and can decide their key name.
Kind of like what the otelhttp library does, where we define two structs, one for the old and another for the new version.
https://github.com/open-telemetry/opentelemetry-go-contrib/tree/main/instrumentation/net/http/otelhttp/internal/semconv
I would also move the logic into an internal package, for a better separation of concerns.
There was a problem hiding this comment.
That's an interesting idea. If I understand correctly, you're suggesting we define two separate command monitors specifically for the semantic conventions. Could you explain the benefits of this pattern over the more atomic append* solution proposed in the PR? The goal of the existing solution is to minimize code changes when advancing the library to a desired version, while also avoiding duplication in the monitoring logic.
There was a problem hiding this comment.
Your solution works if some values are renamed. But if a new value is added, or one is removed then, it becomes trickier to handle since the root method has to know about everything.
With one struct for each semconv version, that struct knows exactly what to do for that version, and that's it. There aren't two versions mixed.
It would also avoid having to define one method for each attribute.
Finally, doing the separation this way also makes it easier to add a third version if we someday need it (I really hope we don't).
There was a problem hiding this comment.
At last using the same patterns in multiple modules help in maintenance of the repository.
There was a problem hiding this comment.
@dmathieu Here are my arguments for the factory pattern over struct duplication:
if a new value is added, or one is removed then, it becomes trickier to handle since the root method has to know about everything.
In this case you would just add nil to the unused version, networkHost for example:
func appendNetworkHost(attrs []attribute.KeyValue, h string) []attribute.KeyValue {
return appendAttrs(attrs, semconv1210.NetPeerName, nil, h)
}It would also avoid having to define one method for each attribute.
This is pretty low complexity, exchanging 1 line of code for 3 to avoid refactoring the library.
Finally, doing the separation this way also makes it easier to add a third version if we someday need it (I really hope we don't).
By design, there can never be a third version. The clarification of this point can be found here: open-telemetry/semantic-conventions#1502. Additionally, this logic should be rolled back entirely once we release a stable version:
we only considered using OTEL_SEMCONV_STABILITY_OPT_IN for the initial unstable to stable migration, and not subsequent major version bumps from stable -> (breaking) stable
There was a problem hiding this comment.
@prestonvasquez, please first create a PR which refactors otelhttp. We need to try using similar patterns across the codebase to make it easier to maintain and more readable. Ideally the pattern should be described in https://github.com/open-telemetry/opentelemetry-go/blob/main/CONTRIBUTING.md#style-guide.
Resolves #6171, #2165
See issue for migration table.
The specifications for maintaining multiple semantic convention versions is documented here: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database/database-spans.md.
To use a non-default version:
To include both v1.26.0 and v1.21.0:
To include the default (v1.21.0) version: