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

LowMemory metrics temporality preference #3915

Merged
merged 14 commits into from
Jun 30, 2023

Conversation

martinkuba
Copy link
Contributor

Which problem is this PR solving?

This adds the LowMemory option for the OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE configuration. This is defined in the spec here.

Fixes #3818

Short description of the changes

This PR adds a new AggregationTemporalitySelector with mapping between instrument types and aggregation temporality. It also adds the new AggregationTemporalityPreference type to allow for the new value to be passed in configuration.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist:

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

@martinkuba martinkuba requested a review from a team June 15, 2023 00:09
@martinkuba martinkuba force-pushed the lowmemory-temporality branch from 2e5b58b to 814294e Compare June 20, 2023 22:48
@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Merging #3915 (23af818) into main (bb8a4f7) will increase coverage by 0.84%.
The diff coverage is 100.00%.

❗ Current head 23af818 differs from pull request most recent head 9f35fa3. Consider uploading reports for the commit 9f35fa3 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3915      +/-   ##
==========================================
+ Coverage   92.30%   93.14%   +0.84%     
==========================================
  Files         286      298      +12     
  Lines        8286     8869     +583     
  Branches     1713     1826     +113     
==========================================
+ Hits         7648     8261     +613     
+ Misses        638      608      -30     
Impacted Files Coverage Δ
...er-metrics-otlp-http/src/OTLPMetricExporterBase.ts 93.33% <100.00%> (+29.69%) ⬆️
...metrics-otlp-http/src/OTLPMetricExporterOptions.ts 100.00% <100.00%> (ø)

... and 12 files with indirect coverage changes

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Looks great; thanks for taking the time to add this 🙂
Implementation looks great, but I think we should highlight that this is a breaking change.

CHANGELOG.md Outdated Show resolved Hide resolved
martinkuba and others added 3 commits June 21, 2023 08:54
@hectorhdzg
Copy link
Member

I know these packages are still not version '1' so breaking changes are not a huge deal, but I believe we can just support both AggregationTemporalitySelector and AggregationTemporality without to much trouble here, being one a subset of the other.

@pichlermarc
Copy link
Member

I know these packages are still not version '1' so breaking changes are not a huge deal, but I believe we can just support both AggregationTemporalitySelector and AggregationTemporality without to much trouble here, being one a subset of the other.

I'd also be in favor of that - while we make no stability guarantees for experimental packages, supporting both would probably be helpful for users.

@martinkuba
Copy link
Contributor Author

@hectorhdzg @pichlermarc Can you please let me know what exactly you have in mind? Enums cannot be combined directly, so do you mean converting the two enums to something like const objects or types to allow making one subset of the other?

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Looks good, thanks 🙂

Copy link
Member

@hectorhdzg hectorhdzg left a comment

Choose a reason for hiding this comment

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

LGTM

@martinkuba martinkuba merged commit 070b685 into open-telemetry:main Jun 30, 2023
@martinkuba martinkuba deleted the lowmemory-temporality branch June 30, 2023 18:13
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.

Implement LowMemory metrics temporality
3 participants