pkg/tracing: HTTPStatusCodeAttributes: remove use of deprecated SemConv const #12605
Conversation
1cc15fe to
b6fef4b
Compare
| // HTTPStatusCodeAttributes generates HTTP response status code attributes | ||
| // as specified by the current OpenTelemetry semantic conventions. | ||
| func HTTPStatusCodeAttributes(code int) []attribute.KeyValue { |
There was a problem hiding this comment.
Looks like the only use of this function is in the nydus snapshotter; https://github.com/containerd/nydus-snapshotter/blob/eda72f05cd118853c2ea7a2fc3ce206ce97e31b2/pkg/remote/remotes/docker/resolver.go#L598-L600
Maybe we should consider deprecating it 🤔
There was a problem hiding this comment.
opened a PR to remove uses there;
b6fef4b to
0d01cd3
Compare
| return []attribute.KeyValue{ | ||
| attribute.Int("http.response.status_code", code), | ||
| attribute.Int("http.status_code", code), // Deprecated: SemConv <= v1.21 | ||
| } |
There was a problem hiding this comment.
Not sure if we must keep both, or if we should just swap it for the non-deprecated one.
0d01cd3 to
c125598
Compare
c125598 to
ad65531
Compare
There was a problem hiding this comment.
Pull request overview
This PR removes the dependency on the deprecated semconv.HTTPStatusCodeKey constant and updates HTTPStatusCodeAttributes to use both the current and deprecated HTTP status code attribute names for backward compatibility.
Key Changes:
- Removes the deprecated
semconvpackage import - Updates
HTTPStatusCodeAttributesto return both the new "http.response.status_code" and deprecated "http.status_code" attributes
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return []attribute.KeyValue{semconv.HTTPStatusCodeKey.Int(code)} | ||
| return []attribute.KeyValue{ | ||
| attribute.Int("http.response.status_code", code), | ||
| attribute.Int("http.status_code", code), // Deprecated: SemConv <= v1.21 |
There was a problem hiding this comment.
The deprecation comment is imprecise. According to the PR description, 'http.status_code' was deprecated in SemConv v1.21, not in versions up to and including v1.21. The comment should read '// Deprecated: SemConv v1.21+' or '// Deprecated: since SemConv v1.21' to accurately reflect when the deprecation occurred.
| attribute.Int("http.status_code", code), // Deprecated: SemConv <= v1.21 | |
| attribute.Int("http.status_code", code), // Deprecated: since SemConv v1.21 |
There was a problem hiding this comment.
The intent, or at least my understanding, was "http.status_code" was semconv <= v1.21.
There was a problem hiding this comment.
Yeah, the deprecation is all a bit ambiguous; semconv 1.21 mentions "deprecated", but not in the correct format;
// HTTPStatusCodeKey is the attribute Key conforming to the
// "http.status_code" semantic conventions. It represents the deprecated,
// use `http.response.status_code` instead.
I tried to find the actual commit that deprecated these, but there wasn't a clear one; the v1.21.0 SemConv files were added with the "deprecated" change already present; idealy v1.21.0 would've been added as a copy of v1.20.0, then changes applied to more easily see what changed, but alas; open-telemetry/opentelemetry-go@830fae8
There was a problem hiding this comment.
That said, with containerd/nydus-snapshotter#694 merged, I don't think there's any remaining users of this function at all, so perhaps we could even deprecate it and remove in a future release.
austinvazquez
left a comment
There was a problem hiding this comment.
LGTM, I'm not versed in open telemetry but from a general perspective it seems low cost to carry both in this fashion. The code reduction is a nice touch. Yet another example where a little copy is better than a little dependency.
|
Looks like the merge queue failed; maybe someone can kick it again |
…nv const The "http.status_code" attribute was deprecated in [SemConv v1.21] in favor of "http.response.status_code", and the `HTTPStatusCodeKey` const was deprecated in [SemConv v1.22]. The `HTTPStatusCodeAttributes` utility doesn't appear to be used currently, but let's update it to use both the old and new variants in case someone still uses it. [SemConv v1.21]: https://github.com/open-telemetry/opentelemetry-go/blob/v1.38.0/semconv/v1.21.0/attribute_group.go#L106-L114 [SemConv v1.22]: https://github.com/open-telemetry/opentelemetry-go/blob/v1.38.0/semconv/v1.22.0/attribute_group.go#L1444-L1452 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
ad65531 to
0d27fce
Compare
|
Did a quick rebase to have a fresh run of CI, but if someone could add it back to the merge queue; GitHub has been bad last few days. |
pkg/tracing: HTTPStatusCodeAttributes: remove use of deprecated SemConv const
The "http.status_code" attribute was deprecated in SemConv v1.21 in favor
of "http.response.status_code", and the
HTTPStatusCodeKeyconst was deprecatedin SemConv v1.22.
The
HTTPStatusCodeAttributesutility doesn't appear to be used currently, butlet's update it to use both the old and new variants in case someone still uses
it.