Skip to content
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.

Sign up for GitHub

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

Jump to bottom

undici instrumentation does not set http.url attribute #2417

Closed
timfish opened this issue Sep 2, 2024 · 4 comments
Closed

undici instrumentation does not set http.url attribute #2417

timfish opened this issue Sep 2, 2024 · 4 comments
Labels
bug Something isn't working pkg:instrumentation-undici triage

Comments

@timfish
Copy link
Contributor

timfish commented Sep 2, 2024 •
edited
Loading

I've noticed that the undici instrumentation sets the url.full attribute but does not set the http.url attribute which I would expect for HTTP client spans.

It only sets these attributes:

opentelemetry-js-contrib/plugins/node/instrumentation-undici/src/undici.ts

Lines 178 to 185 in 9dc58af

const attributes: Attributes = {
[SemanticAttributes.HTTP_REQUEST_METHOD]: requestMethod,
[SemanticAttributes.HTTP_REQUEST_METHOD_ORIGINAL]: request.method,
[SemanticAttributes.URL_FULL]: requestUrl.toString(),
[SemanticAttributes.URL_PATH]: requestUrl.pathname,
[SemanticAttributes.URL_QUERY]: requestUrl.search,
[SemanticAttributes.URL_SCHEME]: urlScheme,
};

@timfish timfish added the bug Something isn't working label Sep 2, 2024
@timfish
Copy link
Contributor Author

timfish commented Sep 4, 2024

Ah, looking at this discussion, this is intensional?

@pichlermarc
Copy link
Member

Ah yes, that's intentional. We made it use the latest (stable) semconv, and @opentelemetry/instrumentation-http is still using the old one. @dyladan is currently working on updating @opentelemetry/instrumentation-http, and the spans attributes will then also eventually resemble what the @opentelemetry/instrumentation-undici package produces.

@pichlermarc
Copy link
Member

Core PR updating @opentelemetry/instrumentation-http for future reference: open-telemetry/opentelemetry-js#4940

@timfish
Copy link
Contributor Author

timfish commented Sep 5, 2024

Ok, closing this since it's not a bug!

@timfish timfish closed this as completed Sep 5, 2024
AbhiPrasad pushed a commit to getsentry/sentry-javascript that referenced this issue Sep 9, 2024
@timfish
feat(node): Use @opentelemetry/instrumentation-undici for fetch tra…
facaae4 
…cing (#13485)

This PR migrates the `nativeNodeFetchIntegration` to use
`@opentelemetry/instrumentation-undici` instead of
`opentelemetry-instrumentation-fetch-node`.

The instrumentation is still exported as `nativeNodeFetchIntegration`
and is named `NodeFetch` to ensure backwards compatibility and the tests
pass ~~without changes~~.

Note: One `nextjs-14` e2e test did need a change due to the
new/differing attribute names.

It's worth noting that `@opentelemetry/instrumentation-undici` [uses
different
attributes](open-telemetry/opentelemetry-js-contrib#2417 (comment))
from the latest semantic convention version vs what we are using and
what's used by `opentelemetry-instrumentation-fetch-node`. It looks like
the [http instrumentation is migrating to these
too](open-telemetry/opentelemetry-js#4940) so
some of the changes in this PR will ensure that the http instrumentation
continues to work after these updates.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working pkg:instrumentation-undici triage
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@timfish @pichlermarc