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.

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(plugin-http): remove path from span name #1466

Merged
merged 5 commits into from
Sep 3, 2020

Conversation

morigs
Copy link
Contributor

@morigs morigs commented Aug 27, 2020

Resolves #897.

According to otel spec:

Many REST APIs encode parameters into URI path, e.g. /api/users/123 where 123 is a user id, which creates high cardinality value space not suitable for span names. ... The approach does not work ... when instrumentation is provided by a lower-level middleware that is not aware of the specifics of how the URIs are formed. ... spans SHOULD be using conservative, low cardinality names formed from the available parameters of an HTTP request, such as "HTTP {METHOD_NAME}". Instrumentation MUST NOT default to using URI path as span name...

This PR removes pathname from span name. So span names will be like in spec: HTTP GET.
I assume that framework instrumentations will add parameterized routes to the span names (like express-plugin does).

@codecov
Copy link

codecov bot commented Aug 27, 2020

Codecov Report

Merging #1466 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1466   +/-   ##
=======================================
  Coverage   93.82%   93.82%           
=======================================
  Files         154      154           
  Lines        4762     4762           
  Branches      951      951           
=======================================
  Hits         4468     4468           
  Misses        294      294           
Impacted Files Coverage Δ
packages/opentelemetry-plugin-http/src/http.ts 97.89% <100.00%> (ø)

@obecny
Copy link
Member

obecny commented Sep 1, 2020

I think you have to fix the tests, thx

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving because the changes look good, but the tests must be fixed before merge.

@morigs
Copy link
Contributor Author

morigs commented Sep 3, 2020

I can't figure out how my change could affect coverage. Can anyone help me find what needs to be fixed?

@dyladan
Copy link
Member

dyladan commented Sep 3, 2020

I can't figure out how my change could affect coverage. Can anyone help me find what needs to be fixed?

I would guess that all of the failures are because the span names are being asserted in the tests like here https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-plugin-http/test/integrations/http-enable.test.ts#L292

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

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.

High-cardinality HTTP span names
4 participants