feat: add support for Otlp exponential histograms #38
Conversation
tylerburdsall
force-pushed
the
exponential-histograms
branch
from
31d58ed
to
568b9ad
Compare
There was a problem hiding this comment.
This change looks great - I think it's a good port of the rust feature. I have but one concern:
Metrics are emit()ted on multiple threads. Today, normal histograms account for that with ConcurrentHashMap and LongAdder. But the exponential histogram strategy involves changing multiple things in one shot atomically. If multiple threads emit metrics at the same time this will result in unsound metrics. I think you need to lock the critical section of the ExponentialHistogram implementation to make this thread safe - i.e., LongAdders will not get you there.
|
|
||
| // Verify multi-threaded access is indeed safe | ||
| @Test | ||
| fun testExponentialHistogramMultiThreadedAccess() = runBlocking { |
There was a problem hiding this comment.
This runs the test on 1 thread with 2 coroutines. This does not exercise multi-threaded access. I'd recommend using simple thread spawn apis and have each of 8 threads accumulate 10000 times each with no delays or sleeps. Join the 8 threads then assert that the accumulation is as you expect it to be.
There was a problem hiding this comment.
Ah that sounds good, I'll try it with that, thank you
Adds support for Otlp exponential histograms. This is more or less the JVM implementation
of the goodmetrics_rs feature.
Unit tests were grabbed from the latest
mainof the Rust implementation's unit tests,so this helped ensure development was correct. That being said, it is entirely
possible I have missed something in the actual conversion to Otlp protocol.
I also added a new
sealed interfacecalledDistributionModeso consumers can explicitly set which aggregation mode they wouldlike to use. Considering exponential histograms are a "newer" technology, the default is set to
Histogramso this new versionis not a breaking change.
Unit tests all pass and indicate that this behavior is correct.
The key difference here is that I opted not to use a
LongAdderfor the buckets out of simplicity.If that needs to change, I am more than happy to change that.