Skip to content

interop/xds: Avoid setting unit suffixes #8479

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

Closed

Conversation

arjan-bal
Copy link
Contributor

@arjan-bal arjan-bal commented Jul 30, 2025

The prometheus exporter changed it's behaviour to add not standard unit suffixes to metrics: open-telemetry/opentelemetry-go#6839. As a result grpc_server_call_started_total becomes grpc_server_call_started_call_total.

From the release notes:

Changed handling of go.opentelemetry.io/otel/exporters/prometheus metric renaming to add unit suffixes when it doesn't match one of the pre-defined values in the unit suffix map.

Similar fix in another project: https://github.com/pomerium/pomerium/pull/5749/files

Tested

Verified by running the otel examples that the units are no longer part of the metric names.

RELEASE NOTES: N/A

@arjan-bal arjan-bal force-pushed the interop-avoid-setting-unit-suffixes branch from b5dccae to f63369e Compare July 30, 2025 16:27
@arjan-bal arjan-bal added the Area: Testing Includes tests and testing utilities that we have for unit and e2e tests within our repo. label Jul 30, 2025
@arjan-bal arjan-bal added this to the 1.75 Release milestone Jul 30, 2025
@arjan-bal arjan-bal added Type: Testing and removed Area: Testing Includes tests and testing utilities that we have for unit and e2e tests within our repo. labels Jul 30, 2025
@arjan-bal arjan-bal requested a review from dfawley July 30, 2025 16:31
Copy link

codecov bot commented Jul 30, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.41%. Comparing base (e1f69d8) to head (f63369e).
⚠️ Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8479      +/-   ##
==========================================
- Coverage   82.47%   82.41%   -0.06%     
==========================================
  Files         414      413       -1     
  Lines       40531    40518      -13     
==========================================
- Hits        33429    33394      -35     
- Misses       5743     5762      +19     
- Partials     1359     1362       +3     

see 20 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dfawley dfawley assigned arjan-bal and unassigned dfawley Jul 31, 2025
Copy link
Contributor Author

@arjan-bal arjan-bal left a comment

Choose a reason for hiding this comment

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

I went through the otel PRs that caused the behaviour change again and I think that there is a bug in gRPCs otel plugin:

h.serverMetrics.callStarted = createInt64Counter(metrics.Metrics(), "grpc.server.call.started", meter, otelmetric.WithUnit("call"), otelmetric.WithDescription("Number of server calls started."))

We're specifying the unit as call instead of {call}. Based on the metric naming conventions anything enclosed in curly braces is an annotation and in the absence of a prefix, the unit is 1. The gRPC docs also mention that non-standard units need to be enclosed in curly braces: https://grpc.io/docs/guides/opentelemetry-metrics/#server-instruments.

I believe the correct fix would be to follow the gRPC docs and enclose the required units in {}, however I'm not sure how this would impact existing users of the gRPC otel plugin.

@dfawley @easwars can you comment on this?

@arjan-bal arjan-bal marked this pull request as draft July 31, 2025 18:53
@dfawley
Copy link
Member

dfawley commented Jul 31, 2025

Oh good catch. I would classify that as a bug fix. We should include it in the release notes, but I don't think we have much other option here....

@dfawley dfawley self-requested a review July 31, 2025 19:00
@arjan-bal
Copy link
Contributor Author

Closing this PR, will open another PR with the correct fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants