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

Single-state Aggregator and test refactor #812

Merged
merged 25 commits into from
Jun 13, 2020

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Jun 11, 2020

Simplify export.Aggregator usage

The Aggregator.Checkpoint API is renamed SynchronizedCopy and adds an argument, a different Aggregator into which the copy is stored. This replaces part of #799.

The SDK now stores two aggregators per synchronous record, one per asynchronous record.

The simple Integrator stores one additional aggregator per stateful record.

The constructors of all the Aggregators are modified to return a variable-length slice of aggregators. The SDK will allocate one or two of these at a time. The AggregationSelector.AggregatorFor() API now accepts ...*export.Aggregator to initialize one or more Aggregators with a single allocation.

Test refactoring

The Aggregator tests allocate 1, 2, or 4 Aggregators. This pattern is assisted by new2() and new4() constructors in the individual Aggregator tests. External testing is assisted by Unslice2() in the sdk/metric/aggregator/test package.

Removes a number of test AggregatorFor methods in favor of a single test test.AggregationSelector() used everywhere, where the metric name suffix indicates which class of Aggregator to use.

Minor Aggregator cleanups

The export.Aggregator contract is that Update() and SynchronizedCopy() are synchronized with each other. All the aggregation interfaces (Sum, LastValue, ...) are not meant to be synchronized, as the caller is expected to synchronize aggregators at a higher level after the Accumulator. Some of the Aggregators used unnecessary locking and have been cleaned up.

Several uses of metric.Number were replaced by int64 now that we use sync.Mutex in the MinMaxSumCount and Histogram Aggregators.

metric.Number(0) expressions are replaced by 0 where possible.

Removed alignment tests that were unnecessary because we are using sync.Mutex.

@jmacd jmacd added the area:metrics Part of OpenTelemetry Metrics label Jun 11, 2020
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Nothing blocking, mostly just error checking and docstring updates.

sdk/metric/aggregator/array/array.go Outdated Show resolved Hide resolved
exporters/metric/test/test.go Outdated Show resolved Hide resolved
exporters/metric/test/test.go Outdated Show resolved Hide resolved
sdk/metric/aggregator/array/array_test.go Outdated Show resolved Hide resolved
sdk/metric/aggregator/array/array_test.go Outdated Show resolved Hide resolved
sdk/metric/aggregator/sum/sum_test.go Outdated Show resolved Hide resolved
sdk/metric/aggregator/test/test.go Outdated Show resolved Hide resolved
sdk/metric/histogram_stress_test.go Outdated Show resolved Hide resolved
sdk/export/metric/metric.go Show resolved Hide resolved
sdk/export/metric/metric.go Outdated Show resolved Hide resolved
jmacd and others added 4 commits June 12, 2020 23:51
Thank you for helping improve the tests. ⭐

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@jmacd jmacd merged commit 9925ebe into open-telemetry:master Jun 13, 2020
@jmacd jmacd deleted the jmacd/singleagg branch June 13, 2020 07:55
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants