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

otelgrpc: Remove high cardinality metric attributes #4322

Merged
merged 17 commits into from
Nov 6, 2023

Conversation

MadVikingGod
Copy link
Contributor

Related to #3536
Closes #3071

This removes Peer attributes from grpc.

Copy link
Member

@dmathieu dmathieu left a comment

Choose a reason for hiding this comment

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

Should the attributes being emitted be tested?

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Damien Mathieu <[email protected]>
pellared
pellared previously approved these changes Sep 21, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
@pellared pellared changed the title Removed high cardinality attributes from grpc metrics. otelgrpc: Remove high cardinality metric attributes Sep 21, 2023
@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

Merging #4322 (53ba6bd) into main (2a5fe23) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #4322   +/-   ##
=====================================
  Coverage   80.8%   80.8%           
=====================================
  Files        150     150           
  Lines      10366   10367    +1     
=====================================
+ Hits        8382    8383    +1     
  Misses      1840    1840           
  Partials     144     144           
Files Coverage Δ
...ion/google.golang.org/grpc/otelgrpc/interceptor.go 87.8% <100.0%> (+<0.1%) ⬆️

@pellared
Copy link
Member

pellared commented Sep 21, 2023

Should the attributes being emitted be tested?

IMO they should be tested. Example assertion for otelhttp:

func assertScopeMetrics(t *testing.T, sm metricdata.ScopeMetrics, attrs attribute.Set) {

PS. Now we also have metricdatatest.IgnoreValue 😉

@pellared pellared dismissed their stale review September 21, 2023 15:50

We should add assertion for the metric attributes.

@MadVikingGod
Copy link
Contributor Author

Should the attributes being emitted be tested?

I'm of two minds on that. There should be something that lets us know if we are emitting the attributes that are appropriate for the current semver, but not so coupled that updating to the next breaks everything.

e.g. metrics emits all of metrics attributes, and traces emits rpc.grpc, and rpc.server and maybe also rpc. But we also wouldn't want these tests to break just because we updated to the next semantic convention, that would be toil.

That being said there weren't tests of what was produced prior, and I would have updated them if there were.

@pellared
Copy link
Member

pellared commented Sep 21, 2023

But we also wouldn't want these tests to break just because we updated to the next semantic convention, that would be toil.

On the other hand, it is double-checking (validating) the changes.
EDIT: Bumping semver is not only about changing the package but e.g. also about adding/removing the set of attributes, sometimes changing their values (hopefully almost never).

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Approving as #4323 is created and added to the next release milestone.

@MadVikingGod MadVikingGod added this to the v0.45.0 milestone Sep 26, 2023
@MrAlias MrAlias modified the milestones: v1.20.0/v0.45.0, v0.46.0 Sep 28, 2023
@puckpuck
Copy link
Contributor

Any updates on when this is going to get merged and released?

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.

otelgrpc high metric cardinality from rpc.server.duration
7 participants