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

Add support for LowMemory metrics temporality #2234

Merged
merged 5 commits into from
Jul 13, 2023

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Jul 13, 2023

Fixes #2233

Changes

This PR adds a new AggregationTemporalitySelector for LowMemory with mapping between instrument types and aggregation temporality.

This is a BREAKING change for OTLP metrics exporter configuration, as OtlpHttpMetricExporterOptions and OtlpGrpcMetricExporterOptions class now use exporter::otlp::PreferredAggregationTemporality instead of sdk::metrics::AggregationTemporality as member. We could have instead kept both the fields to avoid this breaking change, but felt this is cleaner approach, unless there are other thoughts.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@lalitb lalitb requested a review from a team July 13, 2023 00:46
@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #2234 (f30e0dc) into main (bc23e6a) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2234   +/-   ##
=======================================
  Coverage   87.31%   87.31%           
=======================================
  Files         169      169           
  Lines        4900     4900           
=======================================
  Hits         4278     4278           
  Misses        622      622           

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix

@marcalff marcalff merged commit 038e65d into open-telemetry:main Jul 13, 2023
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.

[OTLP Metrics Exporter] Implement LowMemory metrics temporality
3 participants