Implement support for NonAbsolute Measurement MaxSumCount #335

evantorrie · 2019-11-20T01:41:18Z

Previously, the MaxSumCount aggregator failed to work correctly with
negative numbers (e.g. MeasureKind Alternate()==true).

This adds various tests for MaxSumCount which vary the alternate param together with the sign of the random numbers generated in the test.

Previously, the MaxSumCount aggregator failed to work correctly with negative numbers (e.g. MeasureKind Alternate()==true).

jmacd

Thanks @evantorrie. I'm glad you took this up. I had filed #319 which is related. It would be great if you'd like to take that up as well (in a second PR)! The task involves

renaming this aggregator to MinMaxSumCount
entering a Min interface in sdk/export/metric/aggregator, implement it here (and ideally in the others--array and ddsketch also support Min).
in the stdout exporter, add the min output (and test for the "no data" condition discussed here).

sdk/metric/aggregator/maxsumcount/msc.go

jmacd · 2019-11-20T19:15:04Z

sdk/metric/aggregator/maxsumcount/msc.go

@@ -96,6 +98,9 @@ func (c *Aggregator) Update(_ context.Context, number core.Number, desc *export.
 	c.current.count.AddUint64Atomic(1)
 	c.current.sum.AddNumberAtomic(kind, number)

+	c.first.Do(func() {


I think with the above suggestion, we can avoid the sync.Once here--instead we'll just treat min and max the same way.

The Max() and Min() functions will have to recognize the minimum/maximum values as special cases. This means they'll now be able to detect a "no data" condition, which will be possible in certain race conditions within the SDK.

They can return an error to indicate that no data was found for Min or Max, using the existing declared errors in the export aggregator package.

Thanks @evantorrie. I'm glad you took this up. I had filed #319 which is related. It would be great if you'd like to take that up as well (in a second PR)! The task involves

Yes, this PR started off as an implementation for #319, but I then found the issue with negative values and the existing Max() implementation, so decided to try to fix that first.

The Max() and Min() functions will have to recognize the minimum/maximum values as special cases. This means they'll now be able to detect a "no data" condition, which will be possible in certain race conditions within the SDK.

@jmacd The issue with this is that when an Aggregator is first created, it does not have any notion of what type of core.Number it will be aggregating. As I understand it, it does not know until the first time that agg.Update() is called.

If we do not set min/max via the sync.Once() and instead leave them as null-initialized values per the existing New() method, then the first period during which numbers are being aggregated may not give the correct answer. That's because number.CompareNumber(kind, c.current.max.AsNumberAtomic()) doesn't work correctly for negative values when c.current.max == 0 (and more obviously, a null-initialized c.current.min will never compare greater than an uint64 metric).

As I see it, possible solutions are:

Pass the type of number being aggregated down to each Aggregator's New() function. The only place an Aggregator is created in the SDK is when the AggregationSelector is called by the batcher, and the AggregationSelector knows which type of metric is going to be aggregated. Passing this to the Aggregator's New() would allow it to set the initialized "current.min/max" correctly based on the number kind.

Avoid the issue by stating that Min()/Max() are only valid after the first Checkpoint() has been taken for an aggregator.

I prefer approach 1., but I don't know whether there are existing constraints or other intended purposes where an Aggregator may need to be created without knowing the kind of metric that it will later aggregate.

jmacd · 2019-11-20T23:58:11Z

I also prefer option #1. As you can probably tell by now, this is a case I hadn’t completely thought through. Thanks again!

…

On Wed, Nov 20, 2019 at 2:55 PM ET ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In sdk/metric/aggregator/maxsumcount/msc.go <#335 (comment)> : > @@ -96,6 +98,9 @@ func (c *Aggregator) Update(_ context.Context, number core.Number, desc *export. c.current.count.AddUint64Atomic(1) c.current.sum.AddNumberAtomic(kind, number) + c.first.Do(func() { The Max() and Min() functions will have to recognize the minimum/maximum values as special cases. This means they'll now be able to detect a "no data" condition, which will be possible in certain race conditions within the SDK. @jmacd <https://github.com/jmacd> The issue with this is that when an Aggregator is first created, it does not have any notion of what type of core.Number it will be aggregating. As I understand it, it does not know until the first time that agg.Update() is called. If we do not set min/max via the sync.Once() and instead leave them as null-initialized values per the existing New() method, then the first period during which numbers are being aggregated may not give the correct answer. That's because number.CompareNumber(kind, c.current.max.AsNumberAtomic()) doesn't work correctly for negative values when c.current.max == 0 (and more obviously, a null-initialized c.current.min will never compare greater than an uint64 metric). As I see it, possible solutions are: 1. Pass the type of number being aggregated down to each Aggregator's New() function. The only place an Aggregator is created in the SDK is when the AggregationSelector is called by the batcher, and the AggregationSelector knows which type of metric is going to be aggregated. Passing this to the Aggregator's New() would allow it to set the initialized "current.min/max" correctly based on the number kind. 2. Avoid the issue by stating that Min()/Max() are only valid after the first Checkpoint() has been taken for an aggregator. I prefer approach 1., but I don't know whether there are existing constraints or other intended purposes where an Aggregator may need to be created without knowing the kind of metric that it will later aggregate. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#335?email_source=notifications&email_token=AA3WFCMKORPYF5JM7TXYQPTQUW56XA5CNFSM4JPL3R22YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCMM2EGA#discussion_r348787788>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA3WFCOICD4WPWBNNYMQJ7DQUW56XANCNFSM4JPL3R2Q> .

Allows it to set the initial state (current.max) to the correct value based on the NumberKind.

This is analagous to the DDSketch New() constructor

evantorrie · 2019-11-21T01:55:50Z

Should be ready to review now.

An empty checkpoint should have Sum() == 0, Count() == 0 and Max() still equal to the numberKind.Minimum()

Remove TODO from stdout exporter to ensure that if a maxsumcount or ddsketch aggregator returns ErrEmptyDataSet from Max(), then the entire record will be skipped by the exporter. Added tests to ensure the exporter doesn't send any updates for EmptyDataSet checkpoints - for both ddsketch and maxsumcount.

evantorrie · 2019-11-21T03:31:07Z

@jmacd This seems to experience intermittent crashes during the x86-32 (386) CI build.. backtrace shows a panic in

runtime/internal/atomic.Xadd64(0x56cb2304, 0x1, 0x0, 0x8147fb0, 0x31)
	/usr/local/go/src/runtime/internal/atomic/asm_386.s:105 +0xc
go.opentelemetry.io/otel/api/core.(*Number).AddUint64Atomic(...)
	/home/circleci/project/api/core/number.go:442
go.opentelemetry.io/otel/sdk/metric/aggregator/maxsumcount.(*Aggregator).Update(0x56cb2300, 0x836a880, 0x56cb0000, 0x31, 0x0, 0x56ca2390, 0x1, 0x0)
	/home/circleci/project/sdk/metric/aggregator/maxsumcount/msc.go:110 +0x47

Looking at the sync.atomic docs, there is this warning from rsc: "On ARM, x86-32, and 32-bit MIPS, it is the caller's responsibility to arrange for 64-bit alignment of 64-bit words accessed atomically. The first word in a variable or in an allocated struct, array, or slice can be relied upon to be 64-bit aligned."

Have you seen/run into this before?

jmacd · 2019-11-21T22:30:58Z

I glanced at the test failures, and it looks like the root cause is a nil pointer exception.

On 32-bit architectures, Go only guarantees that primitive values are aligned to a 4 byte boundary. Atomic operations on 32-bit machines require 8-byte alignment. See golang/go#599

evantorrie · 2019-11-22T00:59:03Z

It was a 32-bit vs 64-bit layout issue - now fixed.

See https://go101.org/article/memory-layout.html for more gory details

jmacd

I'm glad you figured out the problem about alignment! (I learned some things.)

I'm inclined to (in another PR) add some tests based on unsafe.Alignof() to quickly avoid such confusion in the future. (I will file an issue about this.)

sdk/metric/aggregator/maxsumcount/msc.go

jmacd · 2019-11-22T04:35:19Z

sdk/metric/aggregator/maxsumcount/msc.go

 func (c *Aggregator) Max() (core.Number, error) {
+	if c.checkpoint == unsetMaxSumCount(c.kind) {


Because the MaxSumCount aggregator does not take a lock during Update, each field of the state will be independently modified, and a race with Update could happen such that the count and sum are non-zero but the max field hasn't been set. So, I think you should test c.checkpoint.max == c.kind.Minimum() here.

(Note: this means, now, that the minimum and maximum values are considered invalid. This might be worth adding comments about, i.e., that if you record a measure value which is the Minimum possible value, that the Max() function will return a no-data error. I don't think this will be a problem, but it would probably become a problem if this library supported unsigned integer instruments. Then a value of Record(0) would lead to a no-data error in Max(). If we did ever support unsigned instruments, I we'd probably want to offset by one or something. Nevermind.)

Related note: https://gitter.im/open-telemetry/opentelemetry-go?at=5dd7871489fce12f25a5a76c

The use of Minimum() for the default uninitialized Maximum value means that in the unlikely condition that every recorded value for a measure is equal to the same NumberKind.Minimum(), then the aggregator's Max() will return ErrEmptyDataSet

jmacd · 2019-11-22T20:09:38Z

@paivagustavo will you please review?

evantorrie added 2 commits November 19, 2019 17:36

Add tests for nonabsolute and varying sign values

e6da370

Implement support for NonAbsolute Measurement MaxSumCount

9df7ee5

Previously, the MaxSumCount aggregator failed to work correctly with negative numbers (e.g. MeasureKind Alternate()==true).

evantorrie requested review from jmacd, lizthegrey, pjanotti, rghetia, sjkaris and tedsuo as code owners November 20, 2019 01:41

evantorrie changed the title ~~Implement support for NonAbsolute Measurement MaxSumCount~~ WIP: Implement support for NonAbsolute Measurement MaxSumCount Nov 20, 2019

jmacd reviewed Nov 20, 2019

View reviewed changes

evantorrie added 4 commits November 20, 2019 17:28

Pass NumberKind to MaxSumCount New() function

3d8676f

Allows it to set the initial state (current.max) to the correct value based on the NumberKind.

Revert extraneous local change

a902c06

Pass full descriptor to msc New()

5614d50

This is analagous to the DDSketch New() constructor

Remember to run make precommit first

8ac72bc

evantorrie changed the title ~~WIP: Implement support for NonAbsolute Measurement MaxSumCount~~ Implement support for NonAbsolute Measurement MaxSumCount Nov 21, 2019

evantorrie added 2 commits November 20, 2019 18:39

Add tests for empty checkpoint of MaxSumCount aggregator

6c3bff3

An empty checkpoint should have Sum() == 0, Count() == 0 and Max() still equal to the numberKind.Minimum()

Relayout Aggreggator struct to ensure int64s are 8-byte aligned

697dff2

On 32-bit architectures, Go only guarantees that primitive values are aligned to a 4 byte boundary. Atomic operations on 32-bit machines require 8-byte alignment. See golang/go#599

jmacd reviewed Nov 22, 2019

View reviewed changes

jmacd mentioned this pull request Nov 22, 2019

Add unsafe.Alignof() tests for code that depends on 64-bit atomics #341

Closed

Addressing PR comments

7dc27fa

The use of Minimum() for the default uninitialized Maximum value means that in the unlikely condition that every recorded value for a measure is equal to the same NumberKind.Minimum(), then the aggregator's Max() will return ErrEmptyDataSet

evantorrie requested a review from iredelmeier as a code owner November 22, 2019 18:29

jmacd approved these changes Nov 22, 2019

View reviewed changes

evantorrie added 2 commits November 22, 2019 11:35

Merge remote-tracking branch 'upstream/master' into nonabsolutemsc

2403cfb

Fix PR merge issue

990cf2e

paivagustavo approved these changes Nov 23, 2019

View reviewed changes

Merge branch 'master' into nonabsolutemsc

0ea7340

rghetia approved these changes Nov 25, 2019

View reviewed changes

rghetia merged commit 9a2484c into open-telemetry:master Nov 25, 2019

evantorrie deleted the nonabsolutemsc branch June 23, 2020 19:07

hstan referenced this pull request in hstan/opentelemetry-go Oct 15, 2020

mongo: use db.system instead of db.type (#335)

5997cc3

pellared added this to the untracked milestone Nov 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for NonAbsolute Measurement MaxSumCount #335

Implement support for NonAbsolute Measurement MaxSumCount #335

evantorrie commented Nov 20, 2019

jmacd left a comment

jmacd Nov 20, 2019

evantorrie Nov 20, 2019

evantorrie Nov 20, 2019

jmacd commented Nov 20, 2019 via email

evantorrie commented Nov 21, 2019

evantorrie commented Nov 21, 2019

jmacd commented Nov 21, 2019

evantorrie commented Nov 22, 2019

jmacd left a comment

jmacd Nov 22, 2019

jmacd Nov 22, 2019

jmacd commented Nov 22, 2019

		func (c *Aggregator) Max() (core.Number, error) {
		if c.checkpoint == unsetMaxSumCount(c.kind) {

Implement support for NonAbsolute Measurement MaxSumCount #335

Implement support for NonAbsolute Measurement MaxSumCount #335

Conversation

evantorrie commented Nov 20, 2019

jmacd left a comment

Choose a reason for hiding this comment

jmacd Nov 20, 2019

Choose a reason for hiding this comment

evantorrie Nov 20, 2019

Choose a reason for hiding this comment

evantorrie Nov 20, 2019

Choose a reason for hiding this comment

jmacd commented Nov 20, 2019 via email

evantorrie commented Nov 21, 2019

evantorrie commented Nov 21, 2019

jmacd commented Nov 21, 2019

evantorrie commented Nov 22, 2019

jmacd left a comment

Choose a reason for hiding this comment

jmacd Nov 22, 2019

Choose a reason for hiding this comment

jmacd Nov 22, 2019

Choose a reason for hiding this comment

jmacd commented Nov 22, 2019