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 _total from duplicate untyped Sums before export #678

Merged
merged 2 commits into from Jul 18, 2023

Conversation

damemi
Copy link
Member

@damemi damemi commented Jul 18, 2023

Fixes #677

updates GMP exporter's custom GetMetricName() to remove _total from Sums which have the double-export untyped-metric attribute, if total was not present in the original metric name (before being normalized by OTel's prometheus.BuildPromCompliantName() function).

We need this because the extra untyped Sum metric is added at the ExtraMetrics extension point, which is before name normalization happens for the individual metric types.

Alternatively, the extra metric could be added after name normalization (ie, copy the normalized Gauge metric name to a new Sum after this line in gaugePointToTimeSeries). But that would require refactoring to add a new extension point, which is not worth the effort with a long term upstream plan in the works. The current method takes advantage of existing metric handling for Gauges+Sums as well, such as cumulative normalization.

@damemi damemi requested a review from a team as a code owner July 18, 2023 16:32
@dashpole
Copy link
Contributor

unit test would be useful here

@damemi
Copy link
Member Author

damemi commented Jul 18, 2023

unit test would be useful here

added a unit test

@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Merging #678 (9dd6a16) into main (191af02) will decrease coverage by 0.23%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##             main     #678      +/-   ##
==========================================
- Coverage   69.01%   68.79%   -0.23%     
==========================================
  Files          36       36              
  Lines        4651     4679      +28     
==========================================
+ Hits         3210     3219       +9     
- Misses       1288     1308      +20     
+ Partials      153      152       -1     
Impacted Files Coverage Δ
...tor/integrationtest/testcases/testcases_metrics.go 85.52% <0.00%> (-14.48%) ⬇️
...porter/collector/googlemanagedprometheus/naming.go 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@dashpole
Copy link
Contributor

One thing we will have to think about going forward is that we won't have access to the "unknown" flag from a counter. The "unknown" flag will be gauge-specific. That will make our current approach of looking at labels in the naming function no longer work. We don't need to solve that here, but worth noting

@damemi damemi merged commit cb9c62a into GoogleCloudPlatform:main Jul 18, 2023
23 of 25 checks passed
@damemi
Copy link
Member Author

damemi commented Jul 18, 2023

fyi @ridwanmsharif: if you see double-exported metrics with _total added to the counter, this will fix that

@dashpole
Copy link
Contributor

@damemi it looks like this removes _total from all counters, not just those that came from untyped. We should only remove _total from counters that come from unknown metrics.

@damemi
Copy link
Member Author

damemi commented Jul 18, 2023

@damemi it looks like this removes _total from all counters, not just those that came from untyped. We should only remove _total from counters that come from unknown metrics.

ugh yup, missed that... #680

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.

Untyped metrics are incorrectly named when metric name normalization is enabled.
2 participants