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(sdk-metrics)!: drop View and Aggregation for options #4931

Merged

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Aug 20, 2024

Which problem is this PR solving?

Drops Aggregation and View for options (AggregationOption and ViewOptions, respectively). This means we will have two less classes exported from the @opentelemetry/sdk-metrics package. This has a few benefits:

  • Aggregation is not user-implementable, so having only options makes this clearer.
  • Aggregation was supposed to be used in exporters, however its type does reference SDK internal types as well. This means that if these supposedly internal types change, the exporter may break too if it pins an older version of @opentelemetry/sdk-metrics (which is a valid approach for writing an exporter).
  • View is really only used internally and not directly by the user, so writing new View({...}) for each view is just additional work vs {...}.
  • making these internal opens up the possibility of non-breaking bundle size improvements in the future

Fixes #4654

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • Updated tests

@pichlermarc pichlermarc marked this pull request as ready for review August 20, 2024 15:04
@pichlermarc pichlermarc requested a review from a team August 20, 2024 15:04
@pichlermarc pichlermarc added the target:next-major-release This PR targets the next major release (`next` branch) label Aug 20, 2024
@pichlermarc pichlermarc requested a review from a team as a code owner September 24, 2024 13:21
Copy link

codecov bot commented Sep 24, 2024

Codecov Report

Attention: Patch coverage is 93.05556% with 5 lines in your changes missing coverage. Please review.

Project coverage is 93.90%. Comparing base (32564ad) to head (e15b379).
Report is 5 commits behind head on next.

Files with missing lines Patch % Lines
packages/sdk-metrics/src/view/AggregationOption.ts 87.50% 4 Missing ⚠️
...er-metrics-otlp-http/src/OTLPMetricExporterBase.ts 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next    #4931      +/-   ##
==========================================
- Coverage   93.96%   93.90%   -0.06%     
==========================================
  Files         310      311       +1     
  Lines        8147     8172      +25     
  Branches     1630     1643      +13     
==========================================
+ Hits         7655     7674      +19     
- Misses        492      498       +6     
Files with missing lines Coverage Δ
...etry-exporter-prometheus/src/PrometheusExporter.ts 96.34% <100.00%> (+0.04%) ⬆️
...imental/packages/opentelemetry-sdk-node/src/sdk.ts 95.48% <100.00%> (ø)
packages/sdk-metrics/src/MeterProvider.ts 100.00% <100.00%> (ø)
...ages/sdk-metrics/src/export/AggregationSelector.ts 100.00% <100.00%> (ø)
packages/sdk-metrics/src/export/MetricReader.ts 100.00% <100.00%> (ø)
.../sdk-metrics/src/state/MeterProviderSharedState.ts 100.00% <100.00%> (ø)
packages/sdk-metrics/src/view/Aggregation.ts 100.00% <100.00%> (ø)
packages/sdk-metrics/src/view/View.ts 100.00% <100.00%> (ø)
...er-metrics-otlp-http/src/OTLPMetricExporterBase.ts 91.42% <66.66%> (-1.33%) ⬇️
packages/sdk-metrics/src/view/AggregationOption.ts 87.50% <87.50%> (ø)

... and 1 file with indirect coverage changes

@pichlermarc pichlermarc merged commit 8fed1b1 into open-telemetry:next Oct 8, 2024
18 checks passed
@pichlermarc pichlermarc deleted the feat/remove-aggregation branch October 8, 2024 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
target:next-major-release This PR targets the next major release (`next` branch)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants