Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add support for OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS #5197
Add support for OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS #5197
Changes from 12 commits
81790f5
22aad50
2cf08b2
5e4635e
7bf90e5
047b4b7
3bc8c2d
283f818
5e6d36c
688602f
92c4df1
40dec15
f8e1d6d
058e5ba
9d62ca6
f89c183
e50836b
8e2928b
1d08a17
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is this being set from
OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS
anywhere? I don't see a ctor acceptingIConfiguration
on this class so it is possible some prep works needs to be done to hook that up. LMK if you would like some help with that.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.
Correct, that was still a to do.
I now made some changes to read from env variables as well and added a unit test for it.
Can you review?
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.
A question I had:
When
OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS
is set to an empty string or something illogical likefooBar
,http.request.method
is still set toGET
for example and not to_OTHER
.Do you think this is correct behaviour?
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 pushed some changes to the branch. Sorry about that I needed to see how the code was working and then I just started messing around with it 😓
Refactored everything into
RequestMethodHelper
(instead of options classes). Reason for that isOTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS
should also apply to metrics. I didn't want to introduce metrics options classes where we don't have them and also didn't want the same code in 4 options classes 🤣 I think now it has better separation of concerns.Your question above about
fooBar
. I took a look at the spec and it seems likeOTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS
should fully override the default list. The way it was coded you could only pair down the list of known things. I switched it up so we respect whatever is specified byOTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS
. I would like @vishweshbankwar to sound off on this. He has more expertise in the area. But he is out until 1/15 so we'll have to wait a bit to get his review.