-
Notifications
You must be signed in to change notification settings - Fork 69
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
Chore: Upgrade otel dependencies #967
Conversation
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.
LGTM
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.
I have only one comment but I have tested this and it LGTM! 🙌
span.SetAttributes( | ||
semconv.HTTPURL(req.URL.String()), | ||
semconv.HTTPMethod(req.Method), | ||
) |
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.
The following have all been deprecated:
http.url
http.method
http.status_code
and http.content_length
is not standard
https://opentelemetry.io/docs/specs/semconv/attributes-registry/http/#deprecated-http-attributes
This is the same in Grafana core:
https://github.com/grafana/grafana/blob/a4bb4c84002374f24cfc44efa42304e131353d89/pkg/infra/httpclient/httpclientprovider/tracing_middleware.go
but shall we maybe also add the new non-deprecated/standard ones alongside those (both in core and sdk)?
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.
Yes I noticed and was considering to change this and use the https://github.com/open-telemetry/opentelemetry-go-contrib/blob/c44d632df195200dee20d76b9b486d0349bfa9ea/instrumentation/net/http/otelhttp/transport.go#L51 to standardize on things. However, I think the first step is to make Grafana reuse the SDK tracing middleware why I suggest to keep using the deprecated fields for now.
What this PR does / why we need it:
Upgrades otel dependencies. Supersedes #963 #965 #966
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer: