Add observer metric

[krnowak]

Here is the initial stab at observers.

This adds integral and floating point observer instruments. The meter got Register{Int,Float}64Observer(name, callback, options...) functions. The observers are interfaces like as follows:

type {Int,Float}64Observer interface {
	SetCallback(callback)
	Unregister()
}

The callback is func({Int,Float}64ObserverResult) and the result type is:

type {Int,Float}64ObserverResult interface {
	Observe({int,float}64 value, LabelSet)
}

Both observer and observer result interfaces are implemented by SDK and mocks, noop and global only implement observer.

On the SDK side, each labelset passed to the result's observe function gets its own recorder/aggregator. So, every observer may have several aggregators if the callback called observe several times, each time with a different labelset. That more-or-less mimics what happens when you bind an instrument several times, each time with a different labelset.

There are also three new aggregators for observers, which are basically compositions of two other aggregators: (gauge, minmaxsumcount), (gauge, ddsketch) and (gauge, array). These are used by the simple selector for observer instruments.

SDK calls the observer callbacks in the collection routine, after checkpointing records. Currently there is no protection against a callback that does sleep(time.Eternity).

TODO:

• docs
  • some are done, need more package docs (especially api/metric and sdk/metric)
• tests
• monotonicity option for observers
• the observe implementation in sdk could likely be split into two parts:
  • first part would remain in observe and could call Update on recorder every time observer callback calls Observe on result.
  • second part would be moved to the observer loop in Collect and could call Process on batcher after observer callback is finished
• the checkpointed variable should rather be incremented once per observer callback, not once per observation in the observer callback, I think
• add observer aggregators and update aggregator selectors to handle the observer metric kind

Fixes #423

[krnowak]

I must have some misunderstanding of the observers. During collection period we are going to call the callback of the observer. The callback receives a result object on which the callback can call the observe function many times. Also, observer is like a gauge, so by default we remember the value from the last event. So if we call the observe function several times within a single callback, we basically overwrite the last value all the time. Is this some corner case where calling observe multiple times doesn't make sense?

Also, I tried splitting the observe function as I described in the PR description and it looks to me that the value is not connected to the passed label set. We seem to care about label sets during checkpoint, for the batcher.

I wonder if it would make sense for the observer result object to have two functions:

type Int64ObserverResult interface {
    Observe(value ...int64)
    Labels(labels ...LabelSet)
}

Anyway, I need to read and understand the standard implementation for observers first. This seems to invalidate a lot I wrote above.

[krnowak]

Scratch the above post. Now each observer has a separate recorder for every labelset passed to the observe function and it makes more sense now.

[krnowak]

I'd like to get some initial opinion on the PR before I embark on a dull task of writing the tests for observers. ;)

[rghetia]

Overall looks good.

[rghetia]

Are two attempts necessary?
Wouldn't this work?

obs.observer.setCallback(callback)
obs.trySetCallbackInDelegate(callback)

[krnowak]

That would work too, but it is an (unprofiled ;) ) optimization - obs.observer.setCallback(callback) involves locking the mutex. And I expect the first call to trySetCallbackInDelegate to usually succeed anyway.

[rghetia]

nit:s/NewInt64Observer/RegisterInt64Observer/

[krnowak]

Done.

[rghetia]

same.

[krnowak]

Done.

[jmacd]

I couldn't finish looking this over. It's looking good but I've found a few ways we can limit scope. I'll keep reviewing this again tomorrow.

[krnowak]

Updated.

[krnowak]

TestDirect seems to fail for some reason. I have run it on my computer in 100 iterations loop and all passed. :/

I suppose that we should be checking the core.Number equality in a different way, especially when dealing with floats.

[krnowak]

Rebased. And fixed the TestDirect failure. Basically global meter was registering the observers at the delegate meter in random order. And mock meter was calling observer callbacks in random order too, so the order of taken measurements was also random. TestDirect was expecting some fixed order. My earlier runs of 100 iterations were badly done, because go test cached the test results. Go me.

Added some tests for the internal global meter alignment stuff - test 386 was failing on those.

[jmacd]

Nice work!

[jmacd]

One thing I wonder about is why we should have a SetCallback interface, as opposed to a constructor argument that would bind the callback once for the lifetime of the Observer?

@bogdandrutu It occurs to me that we haven't specified this detail, what do you think?

[rghetia]

One thing I wonder about is why we should have a SetCallback interface, as opposed to a constructor argument that would bind the callback once for the lifetime of the Observer?

I agree. Observer without a Callback is incomplete and most likely it would not require changing it. If we do find a use case later to update the callback then we can always add the SetCallback or UpdateCallback api.

[krnowak]

Rebased to master. Dropped the SetCallback function.

[jmacd]

👏