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

Initial framework for Aggregators #2053

Merged
merged 16 commits into from
May 27, 2021

Conversation

victlu
Copy link
Contributor

@victlu victlu commented May 18, 2021

Introduce initial framework for plumbing in Aggregators into Metrics pipeline.

One goal is to avoid any allocations in the "Hot Path".

@victlu victlu requested a review from a team May 18, 2021 21:40
@codecov
Copy link

codecov bot commented May 18, 2021

Codecov Report

Merging #2053 (b2433b7) into metrics (4190b6b) will decrease coverage by 0.03%.
The diff coverage is 81.37%.

Impacted file tree graph

@@             Coverage Diff             @@
##           metrics    #2053      +/-   ##
===========================================
- Coverage    82.94%   82.90%   -0.04%     
===========================================
  Files          216      224       +8     
  Lines         6560     6804     +244     
===========================================
+ Hits          5441     5641     +200     
- Misses        1119     1163      +44     
Impacted Files Coverage Δ
src/OpenTelemetry/Metrics/Aggregator.cs 0.00% <0.00%> (ø)
src/System.Diagnostics.Metrics.Temp/Counter.cs 60.00% <0.00%> (ø)
src/OpenTelemetry/Metrics/DataPoint.cs 48.00% <45.83%> (-37.72%) ⬇️
src/System.Diagnostics.Metrics.Temp/InstrumentT.cs 72.22% <64.00%> (-4.71%) ⬇️
...enTelemetry/Metrics/ObjectArrayEqualityComparer.cs 73.33% <73.33%> (ø)
...enTelemetry/Metrics/StringArrayEqualityComparer.cs 73.33% <73.33%> (ø)
.../OpenTelemetry/Metrics/Aggregator/SumAggregator.cs 78.94% <78.94%> (ø)
src/OpenTelemetry/Metrics/DataPoint{T}.cs 85.71% <85.71%> (+42.85%) ⬆️
src/OpenTelemetry/Metrics/MeterProviderSdk.cs 94.44% <86.48%> (-4.31%) ⬇️
src/OpenTelemetry/Metrics/AggregatorStore.cs 88.13% <88.13%> (ø)
... and 20 more

@victlu victlu mentioned this pull request May 20, 2021
@tarekgh
Copy link
Contributor

tarekgh commented May 24, 2021

CC @noahfalk for awareness.

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

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

LGTM. Added just a few minor questions/comments that could easily be addressed later if needed.

src/OpenTelemetry/Metrics/IDataPoint.cs Outdated Show resolved Hide resolved
src/OpenTelemetry/Metrics/MeterProviderSdk.cs Outdated Show resolved Hide resolved
Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Thanks!

Left some comments, but we'll keep iterating and improve this as we move forward. I am sure we can do lot more optimizations in the future.

@cijothomas cijothomas merged commit b5f8873 into open-telemetry:metrics May 27, 2021
@victlu victlu deleted the aggregator-structure branch May 28, 2021 00:52
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.

7 participants