Add sdk/metric/aggreation structure to the new_sdk/main branch

[MrAlias]

Blocked by #2799

• Build the unimplemented structure of the sdk/metric/aggregator package.
• Build the unimplemented structure of the sdk/metric/aggregator/aggregation package.
• Build the unimplemented structure of the sdk/metric/aggregator/gauge package.
• Build the unimplemented structure of the sdk/metric/aggregator/histogram packbage.
• Build the unimplemented structure of the sdk/metric/aggregator/sum package.
• Ensure all implementation TODOs have an issue tracking them.

[cuichenli]

Hi @MrAlias I would like to work on this, but I am not sure if my understanding on the ticket is correct. So we just need one file similar to /~https://github.com/open-telemetry/opentelemetry-go/blob/new_sdk/main/sdk/metric/view/view.go for each of those packages? Thanks!

[MrAlias]

Hi @MrAlias I would like to work on this, but I am not sure if my understanding on the ticket is correct. So we just need one file similar to /~https://github.com/open-telemetry/opentelemetry-go/blob/new_sdk/main/sdk/metric/view/view.go for each of those packages? Thanks!

Hey @cuichenli it is going to be more complex than just a duplication of view.go for the aggregators. It requires designing the aggregation pipeline and building out that structure.

I've actually already started working on this, but forgot to assign it to myself. I'm going to assign it to myself now, but feel free to comment here if you have design ideas or structure that you would like to present.

[MadVikingGod]

Additionally when aggregations are defined views must provide a way to select them.

[MrAlias]

The specification states that aggregations are used in conjunction with a view to tell the SDK how to aggregate. This is the only place they are used and it seems like the best place for them would be in the same package that they will be used (the view package).

I would propose this addition to the view package (taking into account the proposed changes in #2926):

package view

import "go.opentelemetry.io/otel/sdk/metric/internal"

func WithAggregation(agg Aggregation) Option {
	return optionFunc(func(v View) View {
		v.aggregation = agg
		return v
	})

}

type Aggregation internal.Aggregation

func Drop() Aggregation {
	return Aggregation(internal.DropAggregation())
}

func Sum() Aggregation {
	return Aggregation(internal.SumAggregation())
}

func LastValue() Aggregation {
	return Aggregation(internal.LastValueAggregation())
}

type explicitBucketHistogramConfig struct {
	Boundaries   []float64
	RecordMinMax bool
}

func newExplicitBucketHistogramConfig(opts []ExplicitBucketHistogramOption) explicitBucketHistogramConfig {
	c := explicitBucketHistogramConfig{
		Boundaries:   []float64{0, 5, 10, 25, 50, 75, 100, 250, 500, 1000},
		RecordMinMax: true,
	}
	for _, o := range opts {
		c = o.apply(c)
	}
	return c
}

type ExplicitBucketHistogramOption interface {
	apply(explicitBucketHistogramConfig) explicitBucketHistogramConfig
}

func ExplicitBucketHistogram(opts ...ExplicitBucketHistogramOption) Aggregation {
	c := newExplicitBucketHistogramConfig(opts)
	impl := internal.ExplicitBucketHistogramAggregation(c.Boundaries, c.RecordMinMax)
	return Aggregation(impl)
}

The implementation of the internal.Aggreation, it's related types, and its ultimate internal package location would be left for a separate change. This would be focused on adding the user facing API.

• Does this seem like a good approach?
• Should the function names include a suffix of Aggreation?

[MadVikingGod]

Is the internal.Aggregation intended to be the object that is responsible for ordering updates and reporting to the reader? If that's the case there are a few more options that are needed for some of the aggregations, specifically temporality and monotonicity.

I think what we need here is some form of selectability, like a stringified int, and a histogram bucket. The problem is we want to expose a way for the user to change an instrument's aggregation, but the aggregations have many more dimensions then just the kind (sum, drop, lastvalue, histogram) they also have temporality (delta sum, cumulative sum, delta histogram, cumulative histogram), and if they are monotonic (all 4 sum's could be monotonic or not). Some of these decisions are written into the spec, like a Asynchronous Gauge will make a non-monotonic Sum, and some are specified by the MetricExporter/MetricReader

[MrAlias]

Is the internal.Aggregation intended to be the object that is responsible for ordering updates and reporting to the reader?

Yes

If that's the case there are a few more options that are needed for some of the aggregations, specifically temporality and monotonicity.

Agreed, I envision they will be added in that internal package. The temporality is configured at the Reader level (we will need to add a WithTemporality option to the NewManualReader and NewPeriodicReader functions), but not at the view aggregation level. As for the monotonicity, it looks to be statically defined by the instrument type. Both of these things do not need to be configured by the user with this API.

I think what we need here is some form of selectability, like a stringified int, and a histogram bucket.

I considered the enum (or even the specified string naming) approach, but that fails to encapsulate the parameters associated with a distinct aggregation type. Using a function or a struct enables users to select an aggregation type and provided relevant
configuration for that type, it removes the question of how to handle parameters for one aggregation being passed when another aggregation is provided.

[MadVikingGod]

I wasn't expecting create an internal.Aggregation here, because the instruments that might be created matched by a view could use all the different flavors of a particular aggregation. What we would need from a view is: given an instrument+kind what aggregation should be used. This information plus the temporality from the Reader and the int vs float from the instrument it self will tell us what exact flavor of aggregation we are using.

I say this because while it might seem simple enough to group all sum Aggregations together there is actual different behavior of a Cumulative Sum vs a Delta Sum (the monotonic is a flag that needs transporting to the output)

[MrAlias]

I've split off the "configure a view with an override aggregation" task to its own issue: #2950. My implementation suggestion above applied to that. I plan to continue the discussion there for that part of the task and will updated this issue with aggregation calculation structure.

[MrAlias]

@MadVikingGod shared this in today's SIG meeting. It is an overview of information flow that is likely relevant to the design of this aggregation pipeline.

[MrAlias]

Closed by #2954