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(monitoring-exporter): convert to GCM format based on metric data type, not instrument type #453

Merged

Conversation

aabmass
Copy link
Contributor

@aabmass aabmass commented Nov 29, 2022

This makes the exporter work with views (see included snapshot test), where the data point type will not match the instrument type. Otherwise the behavior of the exporter is unchanged.

Note the InstrumentDescriptor.name is still the only place to get the metric's name and it actually will match the name configured for output in the view. This is confusing, discussion going on in open-telemetry/opentelemetry-js#3439

@aabmass aabmass force-pushed the do-not-use-instrument-descriptor branch from b28d706 to 1ba4939 Compare November 29, 2022 22:20
@codecov
Copy link

codecov bot commented Nov 29, 2022

Codecov Report

Merging #453 (b347659) into main (375e1be) will decrease coverage by 0.06%.
The diff coverage is 72.22%.

❗ Current head b347659 differs from pull request most recent head c96c019. Consider uploading reports for the commit c96c019 to get more accurate results

@@            Coverage Diff             @@
##             main     #453      +/-   ##
==========================================
- Coverage   92.74%   92.68%   -0.07%     
==========================================
  Files          13       13              
  Lines         441      451      +10     
  Branches       85       86       +1     
==========================================
+ Hits          409      418       +9     
- Misses         32       33       +1     
Impacted Files Coverage Δ
...emetry-cloud-monitoring-exporter/src/monitoring.ts 88.04% <70.58%> (+2.65%) ⬆️
...lemetry-cloud-monitoring-exporter/src/transform.ts 80.00% <73.68%> (-2.06%) ⬇️

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

@aabmass aabmass force-pushed the do-not-use-instrument-descriptor branch 6 times, most recently from 277677b to 7b35b19 Compare November 30, 2022 05:08
@aabmass aabmass marked this pull request as ready for review November 30, 2022 05:11
@aabmass aabmass requested a review from a team November 30, 2022 05:11
@aabmass aabmass force-pushed the do-not-use-instrument-descriptor branch from 7b35b19 to c96c019 Compare November 30, 2022 15:53
@aabmass aabmass merged commit f5df44e into GoogleCloudPlatform:main Nov 30, 2022
@aabmass aabmass deleted the do-not-use-instrument-descriptor branch November 30, 2022 16:24
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.

2 participants