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): bootstrap metric streams #2636

Merged
merged 8 commits into from
Dec 17, 2021

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Nov 22, 2021

Which problem is this PR solving?

This is a prototype of the synchronous metric streams implementation described in https://docs.google.com/document/d/1EluDhj5UanNATS3hR8xx30sv55Se7uYjsGYslvNWHtc/edit#.

Bootstrap metric streams:

  1. Instruments delegate metric events to WritableMetricStorage
  2. SyncMetricStorage (a WritableMetricStorage) stashes delta accumulations in DeltaMetricStorage
  3. DeltaMetricProcessor flushes accumulations when the data is being collected so it won't persist in memory
  4. SyncMetricStorage builds metric data with TemporalMetricStorage for collector's preferred aggregation temporality
  5. TemporalMetricProcessor stashes unreported accumulations for each collector
  6. TemporalMetricProcessor keeps a reported history for each collector for temporality merging.

Related: #2574
Depends on: #2634

Type of change

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

How Has This Been Tested?

  • Basic interface testing.
  • Multiple collectors initiating collections should not have side effects on each other.
  • Integrated testing with MeterProvider/Meter interface.

Checklist:

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

@codecov
Copy link

codecov bot commented Nov 22, 2021

Codecov Report

Merging #2636 (f64ef47) into main (10cd916) will decrease coverage by 0.43%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2636      +/-   ##
==========================================
- Coverage   93.29%   92.86%   -0.44%     
==========================================
  Files         131      143      +12     
  Lines        4923     5158     +235     
  Branches     1078     1102      +24     
==========================================
+ Hits         4593     4790     +197     
- Misses        330      368      +38     
Impacted Files Coverage Δ
...etry-sdk-metrics-base/src/export/MetricExporter.ts 23.52% <0.00%> (ø)
...sdk-metrics-base/src/state/DeltaMetricProcessor.ts 100.00% <0.00%> (ø)
...ckages/opentelemetry-sdk-metrics-base/src/utils.ts 90.00% <0.00%> (ø)
...metrics-base/src/state/MeterProviderSharedState.ts 100.00% <0.00%> (ø)
...pentelemetry-sdk-metrics-base/src/state/HashMap.ts 100.00% <0.00%> (ø)
...ry-sdk-metrics-base/src/state/SyncMetricStorage.ts 100.00% <0.00%> (ø)
...-metrics-base/src/state/TemporalMetricProcessor.ts 97.87% <0.00%> (ø)
...metry-sdk-metrics-base/src/InstrumentDescriptor.ts 100.00% <0.00%> (ø)
...trics-base/src/state/MultiWritableMetricStorage.ts 100.00% <0.00%> (ø)
...emetry-sdk-metrics-base/src/export/MetricReader.ts 23.80% <0.00%> (ø)
... and 3 more

@legendecas legendecas marked this pull request as ready for review December 13, 2021 06:43
@legendecas legendecas requested a review from a team December 13, 2021 06:43
@dyladan
Copy link
Member

dyladan commented Dec 13, 2021

to those @open-telemetry/javascript-approvers who didn't see, this is now ready for review.

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

did a quick pass and overall looks fine from my comprehension of the spec, i will try to review it a second time though before approving

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, just a few nits from my side.

@dyladan dyladan merged commit 27bbb11 into open-telemetry:main Dec 17, 2021
@legendecas legendecas deleted the metrics-ff/streams branch December 17, 2021 13:24
@dyladan dyladan added the enhancement New feature or request label Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants