Skip to content

feat(sdk-metrics-base): shutdown and forceflush on MeterProvider - #2890

Merged
vmarchaud merged 6 commits into
open-telemetry:mainfrom
legendecas:metrics-ff/provider
Apr 15, 2022
Merged

feat(sdk-metrics-base): shutdown and forceflush on MeterProvider#2890
vmarchaud merged 6 commits into
open-telemetry:mainfrom
legendecas:metrics-ff/provider

Conversation

@legendecas

Copy link
Copy Markdown
Member

Which problem is this PR solving?

Refs: #2593

Short description of the changes

  • Passthrough ReaderOptions on MeterProvider.shutdown and MeterProvider.forceFlush.
  • MeterProvider.shutdown and MeterProvider.forceFlush short-circuit promise rejections by Promise.all
  • MetricReader throws on bound to a MeterProvider if it is already bound.

Type of change

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

How Has This Been Tested?

  • MeterProvider.shutdown
  • MeterProvider.forceFlush
  • MetricReader.setMetricProducer

Checklist:

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

@legendecas
legendecas requested a review from a team April 8, 2022 03:59
@codecov

codecov Bot commented Apr 8, 2022

Copy link
Copy Markdown

Codecov Report

Merging #2890 (97aa2ee) into main (322dabe) will increase coverage by 1.21%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2890      +/-   ##
==========================================
+ Coverage   92.42%   93.64%   +1.21%     
==========================================
  Files         163      165       +2     
  Lines        5520     5601      +81     
  Branches     1160     1177      +17     
==========================================
+ Hits         5102     5245     +143     
+ Misses        418      356      -62     
Impacted Files Coverage Δ
...emetry-sdk-metrics-base/src/export/MetricReader.ts 100.00% <0.00%> (ø)
...ry-context-zone-peer-dep/src/ZoneContextManager.ts 85.18% <0.00%> (ø)
...es/opentelemetry-context-zone-peer-dep/src/util.ts 100.00% <0.00%> (ø)
...lemetry-resources/src/detectors/ProcessDetector.ts 100.00% <0.00%> (+4.54%) ⬆️
...pentelemetry-sdk-metrics-base/src/MeterProvider.ts 100.00% <0.00%> (+24.19%) ⬆️
packages/opentelemetry-sdk-trace-web/src/utils.ts 95.00% <0.00%> (+29.37%) ⬆️
...etry-sdk-metrics-base/src/state/MetricCollector.ts 100.00% <0.00%> (+30.76%) ⬆️
...lemetry-resources/src/detectors/BrowserDetector.ts 100.00% <0.00%> (+46.66%) ⬆️

@pichlermarc pichlermarc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good. Just a few non-blocking comments. 🙂

Comment thread experimental/packages/opentelemetry-sdk-metrics-base/src/MeterProvider.ts Outdated

@seemk seemk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 👍

Comment thread experimental/packages/opentelemetry-sdk-metrics-base/src/MeterProvider.ts Outdated
@vmarchaud
vmarchaud merged commit 7086d5a into open-telemetry:main Apr 15, 2022
@legendecas
legendecas deleted the metrics-ff/provider branch April 16, 2022 03:19
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.

5 participants