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

Add details for Aggregator in Metric spec #1804

Closed
wants to merge 11 commits into from

Conversation

victlu
Copy link
Contributor

@victlu victlu commented Jul 7, 2021

Add into Metric spec details for defining a Metric Aggregator.

@victlu victlu requested review from a team July 7, 2021 23:21
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
@reyang
Copy link
Member

reyang commented Jul 8, 2021

Do we intend to cover MIN and MAX?

@reyang
Copy link
Member

reyang commented Jul 8, 2021

Some high level questions:

  1. Is Aggregator intended to be exposed by the SDK as a type/class? (my answer would be NO, we don't need such type/class, we probably want some enum/flags that we can consume while defining the View).
  2. If the answer to 1) is yes, do we intend to provide extensibility (e.g. one could build their own Aggregator)? (my answer would be eventually YES, for the first stable release probably NO)
  3. If the answer to 1) is no, what's the purpose or it is just the SDK internal details?
  4. Do we allow multiple aggregators to co-exist for the same event(s)? For example, if I have temperature, could I have Histogram, Min/Max and Average at the same time? If the answer is yes, do we end up with one stream or multiple streams? (my answer would be - we'll have multiple streams)

specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
@jkwatson
Copy link
Contributor

jkwatson commented Jul 8, 2021

I was surprised not to see any mention of attributes/labels in here, and how an Aggregator needs to handle them. Is this something that is just so obvious that it doesn't need mentioning, or was this missed in writing up the details?

specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
@victlu
Copy link
Contributor Author

victlu commented Jul 9, 2021

Do we intend to cover MIN and MAX?

An Aggregator is intended to target a Metric Point in Metrics Data Model. Thus, an Aggregator internally (in-memory state) stores a composite of data required to generate a Metric Point (Sums, Gauge, Histogram).

Currently, our OTLP protocol and Metric Points do not require MIN and/or MAX. ("Legacy" Summary Metric Point has that concept but wrapped as Quantiles).

@victlu
Copy link
Contributor Author

victlu commented Jul 9, 2021

I was surprised not to see any mention of attributes/labels in here, and how an Aggregator needs to handle them.

Handling of Attributes is indeed a central concept. I did have a few mention and examples of how to address this. But, it likely needs it's own section to clarify. In the meantime, I did add a statement regarding SDK needing to route/map measurements to aggregators. It needs to be more flushed out in spec-level details.

In summary, A MeasurementProcessor ("Aggregator MeasurementProcessor") will route a measurement to zero or more pre-configured Aggregators. This Processor supports a mapping scheme which maps Attributes to Key/Value pair combinations. And for support of multiple exporters, mapping scheme which map per collection frequency.

At the last SIG meeting, I asked where this "routing" task should reside. As part of SDK (which I currently have spec) or part of a MeasurmentProcessor (call it the "Aggregator" measurement processor?). This is unfortunately confusing between a "Aggregator MeasurementProcessor" and an Aggregator.

Aggregator MeasurmentProcessor = concern with taking a measurement and breaking it up to zero or more instances.
Aggregator = concern with how to "aggregate" (sum/count/bucket/etc...) values.

@victlu
Copy link
Contributor Author

victlu commented Jul 9, 2021

  1. Is Aggregator intended to be exposed by the SDK as a type/class? (my answer would be NO, we don't need such type/class, we probably want some enum/flags that we can consume while defining the View).

I think this is more of a style question. In the end, we need to expose the concept of an aggregation/aggregator and potentially any configuration (e.g. Monotonic and/or Cumulative/Delta and/or Bucket scaling).

Enum

  .AddView(..., ENUM_AGGREGATOR_SUM);      // Use "default"
  .AddView(..., ENUM_AGGREGATOR_SUM_MONOTONIC_DELTA);

pro:

  • Only need to expose one ENUM type
  • If we support file based configs (i.e. json/xml), ENUMs are easier to encode.

con:

  • Need to enumerate all possible configurations upfront.
  • Need to update ENUM if we extend in future. And any mapping code for Enum to aggregator.
  • Need different method to pass in hints.

Class/Type

  .AddView(..., new SumAggregator());     // Use "default"
  .AddView(..., new SumAggregator(monotonic=true, temporality=DELTA));
  .AddView(..., new HistogramAggregator(hint=...));    // Take in hint

pro:

  • Allows for finer control (e.g. new Histogram(bucket_boundaries=new int[] { 1, 10, 100 } ;)
  • Potential for easy extensions in future.

con:

  • Need to expose several classes/types.

@victlu
Copy link
Contributor Author

victlu commented Jul 9, 2021

  1. Do we allow multiple aggregators to co-exist for the same event(s)? For example, if I have temperature, could I have Histogram, Min/Max and Average at the same time? If the answer is yes, do we end up with one stream or multiple streams? (my answer would be - we'll have multiple streams)

I also think it would be valuable to allow one measurement to generate multiple metrics. (i.e. a Temperature Instrument can produce a Gauge timeseries and a Histogram timeseries and a Summary timeseries.

An Aggregator as defined here, does not care, as each instance of an aggregator represent one timeseries. The SDK is poised to route a measurement to zero or more aggregators as configured by the View API.

specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
| [Asynchronous Gauge](./api.md#asynchronous-gauge) | [Last Value Aggregator](#LastValueAggregator) | Non-Monotonic | Cumulative | |
| [Histogram](./api.md#histogram) | [Explicit Bucket Histogram Aggregator](#ExplicitBucketHistogramAggregator) | Monotonic | Delta | Default Bucket Boundaries<sup>1</sup> |

\[1\]: Default Bucket Boundaries: (-&infin;, 0], (0, 5.0], (5.0, 10.0], (10.0, 25.0], (25.0, 50.0], (50.0, 75.0], (75.0, 100.0], (100.0, 250.0], (250.0, 500.0], (500.0, 1000.0], (1000.0, +&infin;)
Copy link

Choose a reason for hiding this comment

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

Do we want to specify a default unit of measure for this?

@victlu
Copy link
Contributor Author

victlu commented Aug 2, 2021

After discussion in SIG meeting, a new PR is created #1842 to continue with this spec.

@victlu victlu closed this Aug 2, 2021
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.

10 participants