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(metrics-sdk): bootstrap aggregation support #2634

Merged
merged 13 commits into from
Dec 9, 2021

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Nov 22, 2021

Which problem is this PR solving?

Bootstraps aggregation and aggregator support. Use cases can be reviewed on #2636.

  • Aggregation is responsible for selecting concrete Aggregator type for an InstrumentDescriptor.
  • Aggregator is stateless, and is responsible for merging, diffing stateful accumulations.
  • Accumulation is aggregated state for a metric stream in a period of time.

Fixes #2599

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

Short description of the changes

Type of change

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

How Has This Been Tested?

  • Basic interface testing

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 #2634 (42bba7d) into main (f51f853) will increase coverage by 0.26%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2634      +/-   ##
==========================================
+ Coverage   93.10%   93.36%   +0.26%     
==========================================
  Files         123      131       +8     
  Lines        4757     4932     +175     
  Branches     1059     1078      +19     
==========================================
+ Hits         4429     4605     +176     
+ Misses        328      327       -1     
Impacted Files Coverage Δ
...es/opentelemetry-sdk-metrics-base/src/view/View.ts 100.00% <0.00%> (ø)
...-metrics-base/src/export/AggregationTemporality.ts 100.00% <0.00%> (ø)
...metry-sdk-metrics-base/src/aggregator/Histogram.ts 98.00% <0.00%> (ø)
...telemetry-sdk-metrics-base/src/aggregator/types.ts 100.00% <0.00%> (ø)
...metry-sdk-metrics-base/src/aggregator/LastValue.ts 100.00% <0.00%> (ø)
...elemetry-sdk-metrics-base/src/export/MetricData.ts 100.00% <0.00%> (ø)
...ntelemetry-sdk-metrics-base/src/aggregator/Drop.ts 100.00% <0.00%> (ø)
...entelemetry-sdk-metrics-base/src/aggregator/Sum.ts 100.00% <0.00%> (ø)
...telemetry-sdk-metrics-base/src/aggregator/index.ts 100.00% <0.00%> (ø)
...telemetry-sdk-metrics-base/src/view/Aggregation.ts 100.00% <0.00%> (+28.57%) ⬆️

@legendecas legendecas force-pushed the metrics-ff/aggregation branch 4 times, most recently from b87269b to 5282e9f Compare November 22, 2021 08:06
@legendecas

This comment has been minimized.

@legendecas legendecas changed the title feat: aggregation support feat: bootstrap aggregation support Nov 22, 2021
@legendecas legendecas changed the title feat: bootstrap aggregation support feat(metrics-sdk): bootstrap aggregation support Nov 22, 2021
@legendecas legendecas marked this pull request as ready for review November 22, 2021 15:32
@legendecas legendecas requested a review from a team November 22, 2021 15:32
Copy link
Member

@pirgeo pirgeo left a comment

Choose a reason for hiding this comment

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

I like this, but I have a few questions about the aggregation and temporarily (see comments).

Copy link
Member

@pirgeo pirgeo left a comment

Choose a reason for hiding this comment

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

I think this is great work! 💯

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.

There are a few small nits still open and a conflict. Once they are resolved I think we can merge this and move forward with other PRs

@dyladan dyladan merged commit a35f093 into open-telemetry:main Dec 9, 2021
@legendecas legendecas deleted the metrics-ff/aggregation branch December 9, 2021 14:50
@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.

Metric Aggregations
6 participants