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-base): per metric-reader aggregation #3153

Merged

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Aug 9, 2022

Which problem is this PR solving?

Add support for per-metric-reader aggregation support

Short description of the changes

sdk-metrics-base
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#metricreader

The spec requires the SDK to allow constructing a MetricReader with an optional aggregation selector and an optional aggregation temporality selector. These selectors are only applicable to the specific metric reader, and should not cause side effects on other metric readers.

Type of change

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

How Has This Been Tested?

  • Create instruments with per-metric-reader aggregations.

Checklist:

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

@legendecas legendecas force-pushed the metrics-ff/reader-aggregation branch 4 times, most recently from ccc296f to a9a058d Compare August 9, 2022 08:48
@codecov
Copy link

codecov bot commented Aug 9, 2022

Codecov Report

Merging #3153 (ac40e73) into main (bd9159a) will decrease coverage by 0.01%.
The diff coverage is 96.47%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3153      +/-   ##
==========================================
- Coverage   93.21%   93.20%   -0.02%     
==========================================
  Files         196      196              
  Lines        6443     6491      +48     
  Branches     1363     1370       +7     
==========================================
+ Hits         6006     6050      +44     
- Misses        437      441       +4     
Impacted Files Coverage Δ
...y-sdk-metrics/src/export/AggregationTemporality.ts 100.00% <ø> (ø)
...entelemetry-sdk-metrics/src/export/MetricReader.ts 100.00% <ø> (ø)
...opentelemetry-sdk-metrics/src/view/ViewRegistry.ts 100.00% <ø> (ø)
...elemetry-sdk-metrics/src/state/MeterSharedState.ts 96.15% <93.33%> (-3.85%) ⬇️
...try-sdk-metrics/src/state/MetricStorageRegistry.ts 94.73% <97.43%> (+0.98%) ⬆️
...etry-exporter-prometheus/src/PrometheusExporter.ts 92.59% <100.00%> (+0.18%) ⬆️
...etrics/src/export/PeriodicExportingMetricReader.ts 96.07% <100.00%> (+0.42%) ⬆️
...-sdk-metrics/src/state/MeterProviderSharedState.ts 100.00% <100.00%> (ø)
...telemetry-sdk-metrics/src/state/MetricCollector.ts 100.00% <100.00%> (ø)
...entelemetry-sdk-metrics/src/state/MetricStorage.ts 100.00% <100.00%> (ø)
... and 2 more

@legendecas legendecas force-pushed the metrics-ff/reader-aggregation branch 5 times, most recently from df29111 to 2c5d368 Compare August 15, 2022 06:50
@legendecas legendecas marked this pull request as ready for review August 15, 2022 07:16
@legendecas legendecas requested a review from a team August 15, 2022 07:16
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.

Thanks for working on this! 🙂
Just a partial review for now, I'll try to continue tomorrow. 🙂

@dyladan dyladan modified the milestones: Configs, Metrics GA Aug 22, 2022
@dyladan
Copy link
Member

dyladan commented Aug 22, 2022

Adding this to the milestone since it doesn't have a tracking issue

@legendecas legendecas force-pushed the metrics-ff/reader-aggregation branch from fd1830c to fabdaf4 Compare August 24, 2022 03:05
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 for working on this! 🙂

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

This looks good thanks

…ggregation

# Conflicts:
#	experimental/packages/opentelemetry-sdk-metrics/src/index.ts
#	experimental/packages/opentelemetry-sdk-metrics/test/state/MeterSharedState.test.ts
@legendecas
Copy link
Member Author

Conflicts resolved.

@legendecas legendecas merged commit dd22372 into open-telemetry:main Sep 1, 2022
@legendecas legendecas deleted the metrics-ff/reader-aggregation branch September 1, 2022 05:57
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.

3 participants