-
Notifications
You must be signed in to change notification settings - Fork 813
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
fix(instrumentation-http): add http.host attribute before sending the request #3054
fix(instrumentation-http): add http.host attribute before sending the request #3054
Conversation
Signed-off-by: Cuichen Li <cuichli@cisco.com>
Signed-off-by: Cuichen Li <cuichli@cisco.com>
Signed-off-by: Cuichen Li <cuichli@cisco.com>
Codecov Report
@@ Coverage Diff @@
## main #3054 +/- ##
==========================================
+ Coverage 92.69% 92.70% +0.01%
==========================================
Files 181 181
Lines 5655 5664 +9
Branches 1201 1202 +1
==========================================
+ Hits 5242 5251 +9
Misses 413 413
|
experimental/packages/opentelemetry-instrumentation-http/src/utils.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-instrumentation-http/test/functionals/utils.test.ts
Show resolved
Hide resolved
…o add-http-host-attributes-before-sending-request
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.
could you rebase your PR ? otherwise the small nit, lgtm
experimental/packages/opentelemetry-instrumentation-http/src/utils.ts
Outdated
Show resolved
Hide resolved
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 attribute changes LGTM. However, I find that the new changes on the timeout event are not convincing. Would you mind splitting that into another PR?
experimental/packages/opentelemetry-instrumentation-http/src/http.ts
Outdated
Show resolved
Hide resolved
…o add-http-host-attributes-before-sending-request
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.
Thank you for your contribution!
Which problem is this PR solving?
Instead of add the
http.host
attribute after received the response, this PR changed it to add the attribute before the request was sent.Fixes #3031
Short description of the changes
Infer the hostname and port from the
host
fieldType of change
Please delete options that are not relevant.
How Has This Been Tested?
Unit test