Directly implement Measure trait for metric aggregates #2371
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
At the moment aggregation implementations (Sum, LastValue, Histogram, etc...) doesn't implement any traits. But there's a
Measure<T>
trait is used to implementSyncInstrument
.At the moment there's a
filter
functionAggregateBuilder
, that makes implementsMeasure<T>
for aggregations. The way it works is like this:Measure<T>
, which captures Arc for measurementArc<dyn Measure<T>
.In this revision aggregations directly implement
Measure<T>
, so things get a bit simpler:Arc<SpecificAggregation>
(no need to wrap in closure, and then in another closure and then create Arc again)Arc<FilteredMeasureInstrument>
which wrapsArc<SpecificAggregation>
to do attribute filtering.Technically this should be more performant (because if there's no filtering, then there's no need to have two closures and 1 extra Arc), but compiler is smart enough, so I didn't observed any increase in performance.
Regardless, in theory there's less work to be done, and also less code to understand :)
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes