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

Metrics aggregate collector generic over temporality #2506

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fraillt
Copy link
Contributor

@fraillt fraillt commented Jan 11, 2025

Prerequisite for #2328

Changes

Provided several interfaces that allow to provide "ValueMap" implementation for specific temporality.
The key element is Collector (which implements AggregateCollector). It provides common functionality for all aggregations (Sum, LastValue, Histogram, etc...):

  • provide attribute-set filtering functionality (for measurement).
  • for specific temporality:
    • time initialization
    • aggregate data creation/initialization
    • collection of aggregation data
      This allow to have very clean concrete implementations. I have refactored ExponentialHistogram to use these new interfaces as an example.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@fraillt fraillt requested a review from a team as a code owner January 11, 2025 18:45
Copy link

codecov bot commented Jan 11, 2025

Codecov Report

Attention: Patch coverage is 95.97701% with 7 lines in your changes missing coverage. Please review.

Project coverage is 77.9%. Comparing base (68af3bb) to head (ea151e4).

Files with missing lines Patch % Lines
...-sdk/src/metrics/internal/exponential_histogram.rs 91.1% 6 Missing ⚠️
...pentelemetry-sdk/src/metrics/internal/collector.rs 98.7% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2506   +/-   ##
=====================================
  Coverage   77.9%   77.9%           
=====================================
  Files        123     124    +1     
  Lines      22944   22974   +30     
=====================================
+ Hits       17880   17915   +35     
+ Misses      5064    5059    -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fraillt fraillt force-pushed the generic-metrics-collector branch from f89f06b to 09ebfdc Compare January 11, 2025 19:47
const TEMPORALITY: Temporality;
type Aggr: Aggregator;

fn measure(&self, value: <Self::Aggr as Aggregator>::PreComputedValue, attributes: &[KeyValue]);
Copy link
Contributor

@utpilla utpilla Jan 17, 2025

Choose a reason for hiding this comment

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

It's getting a bit too confusing with these new coupled traits. Could we separate the concerns here? Could we update Collector trait to only have collect related methods and AggregateMap trait to only have update related methods? You could then keep an impl of both the traits as fields of ExpoHistogram struct.

Copy link
Contributor Author

@fraillt fraillt Jan 21, 2025

Choose a reason for hiding this comment

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

Not sure if it's possible to have this separation, basically this whole things is like an onion.
Starting from the center:

  1. trait AggregateMap - implement the core data structure optimized for updating AND collecting aggregates for specific temporality (e.g. DeltaValueMap, and CumulativeValueMap)
  2. trait AggregateCollector - provides filtering attributes AND collecting impl specific DataPoints into dyn Aggregation (including time initialization for specific temporality). There's only one implementation, - Collector. The reason we have this trait, is because Collector itself is also generic (currently only over trait AggregateMap, but it might also be generic over "aggregate-filter"), which makes trait bounds for implementations to be very verbose). Important property of this implementation is that it has all common code for aggregations.
  3. specific aggregation e.g. (Sum, LastValue, Histogram, etc..) - implements Measure and ComputeAggregation traits, which are used by SDK. Also implements InitAggregationData which is used by AggregateCollector to make whole collection phase reusable.

The thing that I like about this is that 1) and 2) (e.g. impls for trait AggregateMap and trait AggregateCollector ) is common code for absolutely all aggregates. The only aggregate specific logic is in three traits: Measure, ComputeAggregation, InitAggregationData.

Regarding splitting...:

  • trait AggregateMap cannot be split, because it's implementation has optimized internal structure that only it knows how to update and collect
  • since trait AggregateCollector depends on trait AggregateMap, it also cannot be split into few instances.
  • same fields within specific implementations can be used in measure and collect phases, as well. (e.g. Histogram::bounds field), so splitting concrete implementation not optimal as well.
  • so the AggregateFns implementation is probably most efficient way to do it. (e.g. Arc and clone, so one instance implements Measure,- another ComputeAggregation).

BTW, I agree that this gets a bit confusing, but I think better naming would solve the problem here... but that's the hardest part :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mm... maybe I have one more idea.
Instead of having specific aggregation "the final thing" (which implements Measure and ComputeAggregation). I could make that Collector is the final thing, and specific aggregations would have a trait which will be used to provide specific functionality...
I will probably create separate revision for that, (so we could compare both at the same time).
Ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2530 Same thing,- new design.
IMO it turned out very good, I like this new design much more.
If you have same opinion, then we can close this PR.

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.

2 participants