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

feat(metrics-exporters): configure temporality via env var #3305

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Oct 10, 2022

Which problem is this PR solving?

Previously, the aggregation temporality could only be changed by explicitly setting it in the exporters constructor. This PR changes this by implementing the spec around the OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE variable.

Setting this variable will allow users to configure temporality preference. Invalid values will default to cumulative and will be logged as a warning.

Related to (but does not close) #3193

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Existing unit tests
  • Added unit tests

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@codecov
Copy link

codecov bot commented Oct 10, 2022

Codecov Report

Merging #3305 (d044862) into main (30a81bd) will increase coverage by 0.37%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3305      +/-   ##
==========================================
+ Coverage   93.46%   93.84%   +0.37%     
==========================================
  Files         244      222      -22     
  Lines        7325     6526     -799     
  Branches     1515     1351     -164     
==========================================
- Hits         6846     6124     -722     
+ Misses        479      402      -77     
Impacted Files Coverage Δ
...ckages/opentelemetry-core/src/utils/environment.ts 90.90% <ø> (ø)
...porter-metrics-otlp-grpc/src/OTLPMetricExporter.ts 100.00% <100.00%> (ø)
...er-metrics-otlp-http/src/OTLPMetricExporterBase.ts 63.41% <100.00%> (+18.58%) ⬆️
...-otlp-http/src/platform/node/OTLPMetricExporter.ts 100.00% <100.00%> (ø)
...orter-metrics-otlp-proto/src/OTLPMetricExporter.ts 100.00% <100.00%> (ø)
...s/opentelemetry-core/src/platform/node/sdk-info.ts 0.00% <0.00%> (-100.00%) ⬇️
...opentelemetry-core/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...pentelemetry-core/src/platform/node/performance.ts 0.00% <0.00%> (-100.00%) ⬇️
...perimental/packages/otlp-exporter-base/src/util.ts 78.94% <0.00%> (-15.79%) ⬇️
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts 84.48% <0.00%> (-15.52%) ⬇️
... and 24 more

@pichlermarc pichlermarc changed the title feat(metrics-exporters): configure temporality via environment feat(metrics-exporters): configure temporality via environment variables Oct 10, 2022
@pichlermarc pichlermarc changed the title feat(metrics-exporters): configure temporality via environment variables feat(metrics-exporters): configure temporality via environment variable Oct 10, 2022
@pichlermarc pichlermarc marked this pull request as ready for review October 10, 2022 11:11
@pichlermarc pichlermarc requested a review from a team October 10, 2022 11:11
@svetlanabrennan
Copy link
Contributor

Thanks for working on this. I think we should add some documentation in the readme around using these environment variables similar to this.

I know the spec mentions all these details but I think it's helpful for new otel users to see some quick documentation around these env variable options when using these exporters.

Maybe this documentation work can be done in a separate PR since I know we have an issue open to document latest metrics SDK.

@pichlermarc
Copy link
Member Author

Thanks for working on this. I think we should add some documentation in the readme around using these environment variables similar to this.

I know the spec mentions all these details but I think it's helpful for new otel users to see some quick documentation around these env variable options when using these exporters.

Maybe this documentation work can be done in a separate PR since I know we have an issue open to document latest metrics SDK.

I added the env var to the docs and adapted the table for use in the metrics exporters. It bloats up the diff a bit but I think it fits within the scope of this PR. 🙂

However, I also found that several environment variables that were listed in the GRPC metrics exporter's readme are not respected. I have kept those in for now.

@pichlermarc
Copy link
Member Author

Failing build will be fixed by #3328 🙂

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

@legendecas
Copy link
Member

@pichlermarc I can not update this PR's branch so please update it with the latest main branch and feel free to merge this PR! :D

@pichlermarc pichlermarc changed the title feat(metrics-exporters): configure temporality via environment variable feat(metrics-exporters): configure temporality via env var Oct 24, 2022
@pichlermarc pichlermarc merged commit 4420402 into open-telemetry:main Oct 24, 2022
@pichlermarc pichlermarc deleted the feature/env-temporality-config branch October 24, 2022 13:10
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.

4 participants