opentelemetry: set _other for unknown http methods#45222
Conversation
|
Hi @jakebennert, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
|
@jakebennert Thanks for your contribution Please fix the DCO error /wait |
| static constexpr std::array<absl::string_view, 10> KnownMethods{ | ||
| "CONNECT", "DELETE", "GET", "HEAD", "OPTIONS", "PATCH", "POST", "PUT", "QUERY", "TRACE"}; |
There was a problem hiding this comment.
I would probably use a constant/static hashmap for this.
There was a problem hiding this comment.
I originally had this as a hashmap, but Gemini made the case to me that a) the Envoy style guide discourages the use of static non-PoD types due to potential destruction order issues, and b) "a linear search on 10 contiguous elements in memory often beats a hash set lookup because it avoids the overhead of computing a hash and has excellent CPU cache locality". What do you think?
…er in the case of an unknown HTTP method, as defined in the OpenTelemetry semantic conventions. Signed-off-by: Jake Bennert <jakebennert@google.com>
…ariables. Signed-off-by: Jake Bennert <jakebennert@google.com>
Signed-off-by: Jake Bennert <jakebennert@google.com>
Commit Message
The OpenTelemetry semantic conventions state, for both client and server spans:
HTTP request method value SHOULD be “known” to the instrumentation. By default, this convention defines “known” methods as the ones listed in RFC9110, the
PATCHmethod defined in RFC5789 and theQUERYmethod defined in httpbis-safe-method-w-body.If the HTTP request method is not known to instrumentation, it MUST set the
http.request.methodattribute to_OTHER.This PR updates Envoy's OpenTelemetry tracer to align with this standard.
Additional Description
AI was used to generate the code and tests in this PR. I have reviewed – and fully understand – the code.
It should be noted that Envoy's OTel tracer uses
http.methodinstead of the more up-to-datehttp.request.method. However, I believe there is ongoing work to update these names happening in #45184.Risk Level
Low
Testing
I've added unit tests that exercise the new code.
Docs Changes
None required
Release Notes
Added a changelog under minor_behavior_changes (area: opentelemetry).
Platform Specific Features
None