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

Move aggregation to sdk/metric #4432

Closed
pellared opened this issue Aug 10, 2023 · 7 comments · Fixed by #4435
Closed

Move aggregation to sdk/metric #4432

pellared opened this issue Aug 10, 2023 · 7 comments · Fixed by #4435
Assignees
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package

Comments

@pellared
Copy link
Member

pellared commented Aug 10, 2023

This is an output after reviewing #4396

I propose to move the aggregation code to sdk/metric and define the aggregation type like this:

// Aggregation is the aggregation used to summarize recorded measurements.
type Aggregation interface {
	// copy returns a deep copy of the Aggregation.
	copy() Aggregation

	// err returns an error for any misconfigured Aggregation.
	err() error
}

Reasons:

  1. This will disallow creating any customer user-defined aggregation types. Right now someone could "hack" be using type-embedding + creating own implementation of Copy/Err functions. I think that having all methods unexported like in most Option interfaces is cleaner. This argument will be invalid if we decide to allow user-defined Aggregations.
  2. We will flatten the module. The SDK would only expose metric, metricdata and metricdatatests packages. Usually the users would only need metric package in production code. metricdata and metricdatatests packages would usually be used in test code.
  3. Similar to what we have decided to do for instruments under [proposal] Move instrument creation configuration to otel/metric #3995

My only doubt is if we would eventually support user-defined Aggregations. Would it be any problem if allow it up-front? But I find this as a separate issue. Even if we decide to remove the private() method I would still be in favor of this proposal.

@pellared pellared added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Aug 10, 2023
@MrAlias
Copy link
Contributor

MrAlias commented Aug 10, 2023

Do we plan to prefix all the other aggregation structures with Aggregation? E.g. AggregationDrop.

@MrAlias
Copy link
Contributor

MrAlias commented Aug 10, 2023

FWIW, I think when the spec is going to allow custom aggregators it plans to call them Aggregator not aggregation so there may never be a need to extend Aggregation.

@pellared
Copy link
Member Author

During the SIG meeting we agreed to try implementing the proposal.

@pellared pellared changed the title [Proposal] Move aggregation to sdk/metric Move aggregation to sdk/metric Aug 10, 2023
@MrAlias MrAlias added this to the v1.17.0/v0.40.0 milestone Aug 10, 2023
@MrAlias MrAlias self-assigned this Aug 10, 2023
@MrAlias
Copy link
Contributor

MrAlias commented Aug 10, 2023

Flattening this into sdk/metric will mean that sdk/metric/internal/aggregate will need import sdk/metric as it receives these Aggregation as configuration for the values they create. This will create an import cycle as sdk/metric imports sdk/metric/internal/aggregate to use its aggregators.

@MrAlias
Copy link
Contributor

MrAlias commented Aug 10, 2023

Flattening this into sdk/metric will mean that sdk/metric/internal/aggregate will need import sdk/metric as it receives these Aggregation as configuration for the values they create. This will create an import cycle as sdk/metric imports sdk/metric/internal/aggregate to use its aggregators.

I in-lined the fields into parameters in #4435 to resolve this and decouple sdk/metric/internal/aggregate from sdk/metric.

@MrAlias
Copy link
Contributor

MrAlias commented Aug 10, 2023

exporters/stdout/stdoutmetric uses the exported Copy and Err:

cpA := a.Copy()
if err := cpA.Err(); err != nil {

It uses these methods to validate the aggregation selector a user passes. It doesn't seem like this is a necessary things for the exporter to be doing. I'm going to remove it in #4435.

@MrAlias
Copy link
Contributor

MrAlias commented Aug 10, 2023

exporters/stdout/stdoutmetric uses the exported Copy and Err:

cpA := a.Copy()
if err := cpA.Err(); err != nil {

It uses these methods to validate the aggregation selector a user passes. It doesn't seem like this is a necessary things for the exporter to be doing. I'm going to remove it in #4435.

This exists for all the metric exporters as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants