open-telemetry / opentelemetry-specification

Specifications for OpenTelemetry
https://opentelemetry.io
Apache License 2.0
3.69k stars 884 forks source link

Clarify the terms "aggregation" and "aggregator" #2149

Open legendecas opened 2 years ago

legendecas commented 2 years ago

What are you trying to achieve?

When implementing the feature-freeze metrics SDK, we found that the terms "aggregation" and "aggregator" are used in the spec in an interchangeable way confusingly and the spec didn't explain the detailed concept of how aggregation works.

Additional context.

In Java's implementation, there are four concepts for the "aggregation":

  1. Aggregation for selecting concrete Aggregator type for an InstrumentDescriptor, e.g. DefaultAggregation
  2. Aggregator is a stateless interface and is responsible for repetitively creating new stateful Accumulations, also it implements the concrete algorithm for merging and diffing two accumulations, e.g. DoubleSumAggregator
  3. Accumulation is a stateful interface, e.g. DoubleAccumulation
reyang commented 2 years ago

Both "aggregation" and "aggregator" are used in https://github.com/open-telemetry/opentelemetry-proto/blob/b43e9b18b76abf3ee040164b55b9c355217151f3/opentelemetry/proto/metrics/v1/metrics.proto.

I think in the SDK spec we probably should use "aggregation" (and avoid "aggregator") at this moment. "Aggregator" could be a reserved name that we add after the Stable release, to allow custom implementations of "aggregations".

reyang commented 2 years ago

Some of the names from Java were carried from the previous implementation (according to @jsuereth).

Doesn't think it's a spec issue, more like Java implementation issue?

@bogdandrutu mentioned that most of these are internal, so it's not an issue for Java.

@reyang proposed that the SDK should use Aggregation and reserve "Aggregator" term (for the future, when we allow custom aggregators). @aabmass +1. @dyladan +1, this was coming from the JavaScript SIG.

The proto file should also be updated to use "aggregation" only.

The only Stable spec blocking issue is to reserve the "aggregator" term in the SDK spec.

legendecas commented 2 years ago

In the spec, the term Aggregation is declared to "informs the SDK on the ways and means to compute Aggregated Metrics from incoming Instrument Measurements."

Also, there is a pre-defined aggregation DefaultAggregation which acts like the previously defined AggregatorSelector, and SumAggregation and HistogramAggregation etc. that act like previously defined concrete type SumAggregator and HistogramAggregator. With https://github.com/open-telemetry/opentelemetry-specification/pull/2153 the term Aggregator will be reserved in the spec, it is still unclear what the Aggregation is capable of: just selecting the aggregation means, or does it also handle the concrete aggregation jobs?

Since views are configured with aggregations, I found that it is confusing on the term Aggregation as it is defined like the selector but used the term for concrete means to compute aggregated metrics.

reyang commented 2 years ago

The SDK part is covered by #2153, the data model spec needs to be updated before we can resolve/close this issue.

dyladan commented 2 years ago

from @legendecas comment it doesn't seem like he feels the sdk part was resolved by #2153