-
Notifications
You must be signed in to change notification settings - Fork 224
fix: HTTP unknown methods #1781
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
base: main
Are you sure you want to change the base?
Conversation
Ensures that all HTTP client gems support unknown methods as described in semantic conventions: https://opentelemetry.io/docs/specs/semconv/http/http-spans/#http-client-span See open-telemetry#1779
065f55f to
f59066e
Compare
| def self.span_attrs_for(method, semconv: :stable) | ||
| normalized = METHOD_CACHE[method] | ||
| if normalized | ||
| span_name = semconv == :old ? OLD_SPAN_NAMES[normalized] : normalized |
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.
@arielvalentin - I'm concerned about changing the span names and attributes for the "old" instrumentation. It seems to break the contract for the OTEL_SEMCONV_STABILITY_OPT_IN var:
The default behavior (in the absence of one of these values) is to continue emitting whatever version of the old experimental HTTP and networking conventions the instrumentation was emitting previously.
(source)
What do you think about reverting the changes for the old modules and leaving the changes for dup and new?
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.
Ok that makes sense. I can leave the span name alone in follow up PRs.
Please consider that the existing old implementation does not follow the correct conventions for span names and should not have a prefix of HTTP:
HTTP spans MUST follow the overall guidelines for span names. HTTP server span names SHOULD be {http.method} {http.route} if there is a (low-cardinality) http.route available. HTTP server span names SHOULD be {http.method} if there is no (low-cardinality) http.route available. HTTP client spans have no http.route attribute since client-side instrumentation is not generally aware of the "route", and therefore HTTP client spans SHOULD use {http.method}. Instrumentation MUST NOT default to using URI path as span name, but MAY provide hooks to allow custom logic to override the default span name.
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.
may provide hooks to allow custom logic to override the default span name is also something new to me 🤔
Ensures that all HTTP client gems support unknown methods as described in semantic conventions: https://opentelemetry.io/docs/specs/semconv/http/http-spans/#http-client-span
Some concessions:
OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODSenvironment variable to a future PRSee #1779