-
Notifications
You must be signed in to change notification settings - Fork 452
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
Directly implement ComputeAggregation #2425
Directly implement ComputeAggregation #2425
Conversation
Oops just merged the startTime PR! Could you resolve conflicts |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2425 +/- ##
=====================================
Coverage 76.8% 76.8%
=====================================
Files 122 122
Lines 21806 21823 +17
=====================================
+ Hits 16758 16772 +14
- Misses 5048 5051 +3 ☔ View full report in Codecov by Sentry. |
c2d13a1
to
65877a6
Compare
opentelemetry-sdk/src/lib.rs
Outdated
@@ -118,7 +118,7 @@ | |||
#![doc( | |||
html_logo_url = "https://raw.githubusercontent.com/open-telemetry/opentelemetry-rust/main/assets/logo.svg" | |||
)] | |||
#![cfg_attr(test, deny(warnings))] | |||
// #![cfg_attr(test, deny(warnings))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good move!
Prerequisite for #2328
Currently part of aggregate measuring and collecting logic is scattered in several places:
AggregateBuilder
.Instead we should implement all logic in one place (aggregate implementation e.g.
Sum
), andAggregateBuilder
would be responsible only for initializing it.This is especially important for #2328, because we want to be able to select specific implementation (depending on temporality) at compile time. The goal is to be able to initialize/build aggregation like this:
As a side effect this approach will also has less indirection which simplifies code, and might improve performance.
Changes
Temporality
is now passed in to aggregation during creationMerge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes