Skip to content

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

Merged
merged 3 commits into from
Jul 17, 2023
Merged

Conversation

mydea
Copy link
Member

@mydea mydea commented Jul 14, 2023

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

Turns out, `HTTP_TARGET` is always the relative path, even for outgoing requests, which omits the host. So we handle this specifically now here.
@mydea mydea requested review from Lms24 and AbhiPrasad July 14, 2023 12:37
@mydea mydea self-assigned this Jul 14, 2023
Copy link
Member

@AbhiPrasad AbhiPrasad left a 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 {
Copy link
Member

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

Copy link
Member Author

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 👍

Copy link
Member

@Lms24 Lms24 left a 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?

@mydea
Copy link
Member Author

mydea commented Jul 17, 2023

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.
I've updated this to also set url, http.query and http.fragment data on the spans. Note the difference between url (which we use in our own instrumentation already) and http.url which comes from otel itself is that http.url contains the query & hash, wheras our url has these stripped off.

@mydea mydea requested a review from AbhiPrasad July 17, 2023 13:25
@mydea mydea merged commit 2919511 into develop Jul 17, 2023
@mydea mydea deleted the fn/otel-http-urls branch July 17, 2023 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Otel span mapping uses HTTP_TARGET, which may be wrong
3 participants