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

Remove high cardanility metrics from otelhttp #4277

Merged
merged 9 commits into from
Sep 7, 2023

Conversation

MadVikingGod
Copy link
Contributor

@MadVikingGod MadVikingGod commented Sep 5, 2023

This does not upgrade to the latest semantic convention, that will be a different PR.

This removes the following attributes from the metrics that go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp generates:

  • net.sock.peer.addr
  • net.sock.peer.port
  • http.user_agent
  • enduser.id
  • http.client_ip

This also only allows http.request.method to be known HTTP methods, or _OTHER.

Resolve open-telemetry/opentelemetry-go#3765

@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

Merging #4277 (3541453) into main (b6fc62f) will increase coverage by 0.0%.
The diff coverage is 82.6%.

Impacted file tree graph

@@          Coverage Diff           @@
##            main   #4277    +/-   ##
======================================
  Coverage   79.4%   79.5%            
======================================
  Files        166     166            
  Lines      10376   10677   +301     
======================================
+ Hits        8246    8491   +245     
- Misses      1996    2045    +49     
- Partials     134     141     +7     
Files Changed Coverage
...stful/otelrestful/internal/semconvutil/httpconv.go 82.6%
...gonic/gin/otelgin/internal/semconvutil/httpconv.go 82.6%
...rilla/mux/otelmux/internal/semconvutil/httpconv.go 82.6%
...ack/echo/otelecho/internal/semconvutil/httpconv.go 82.6%
...on.v1/otelmacaron/internal/semconvutil/httpconv.go 82.6%
...ace/otelhttptrace/internal/semconvutil/httpconv.go 82.6%
...net/http/otelhttp/internal/semconvutil/httpconv.go 82.6%
instrumentation/net/http/otelhttp/handler.go 100.0%

internal/shared/semconvutil/httpconv.go.tmpl Outdated Show resolved Hide resolved
internal/shared/semconvutil/httpconv.go.tmpl Outdated Show resolved Hide resolved
internal/shared/semconvutil/httpconv.go.tmpl Outdated Show resolved Hide resolved
@davendu
Copy link

davendu commented Sep 12, 2023

Weird. Why @dmathieu 's previous suggestion is not applied to this PR:

I don't think this is the proper approach. It's adding some weird complexity, and whether an attribute has too much cardinality or not is very subjective.

The solution being discussed in open-telemetry/opentelemetry-specification#3061 seems much better.

And what's the community's suggestion towards similar attributes? My own instrumentation library also needs some high cardanility attributes, and not sure if I should remove them or filter through the view.

@dmathieu
Copy link
Member

This PR solves from a potential memory leak issue, since the values from those attributes are coming from untrusted sources.
We should be able to bring back those attributes once something like open-telemetry/opentelemetry-go#4457 is stable.

@FaranIdo
Copy link
Contributor

@dmathieu @pellared @MadVikingGod do you plan to add a similar solution to otelgrpc to avoid high cardinality there as well? (I think reported here: #3071)

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.

Metrics memory leak v1.12.0/v0.35.0 and up
7 participants