-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(otel): Use HTTP_URL
attribute for client requests
#8539
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
Conversation
Turns out, `HTTP_TARGET` is always the relative path, even for outgoing requests, which omits the host. So we handle this specifically now here.
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.
Name changes lgtm!
} | ||
|
||
// Remove trailing ? and # from the target | ||
function normalizeTarget(httpTarget: string): string { |
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.
We have a helper for this, stripUrlQueryAndFragment
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.
ahh nice, I'll use this then 👍
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, just out of curiosity (no action required): Do we store the query or fragment somewhere on the span as we do in our own instrumentation?
It's actually a very good point! I have started aligning this a bit in a different place, but actually may as well do it here already. |
Turns out,
HTTP_TARGET
is always the relative path, even for outgoing requests, which omits the host. So we handle this specifically now here.While at it (and as I'm working a bit in this codebase), I also renamed the files from kebap-case to camelCase, to align with the rest of the codebase better - unless there was a specific reason to use that here @AbhiPrasad ?
Closes #8535