HdrSummary based on Summary and HdrHistogram

[ghost]

Idea:

We could implement HdrSummary based on Summary and HdrHistogram.

Pros:

• avoids suspending threads for highly concurrent workloads (get, insert)
• reduces constant overhead (get, insert)

Cons:

• introduces busy waiting and retry-loop (rotate)
• increases memory usage (maxAgeBuckets, numberOfSignificantValueDigits)
• reduces accuracy / precision (High Dynamic Range)

Neutral:

• error is configured globally (numberOfSignificantValueDigits)
• negative measurements are not supported

See http://hdrhistogram.org and https://github.com/HdrHistogram/HdrHistogram for more info on HdrHistogram.

Related to #481 , #482 , #483
Resolves #480

cc @t3hnar

[ghost]

Please note that this is work in progress and requires careful review and testing. ⚠️

[brian-brazil]

I'll be interested to see the benchmarks. I'd not have two Summary variants though, replacing what's there is what'd be considered.

[ghost]

I ran multiple benchmarks with 24 threads and picked the result with the smallest error, but of course I can’t know how it behaves under different workloads.

It looks like this improves observe performance significantly. 🙂
I haven't checked collect performance, memory usage, GC overhead, etc. yet.

Benchmark:

# JMH version: 1.21
# VM version: JDK 11.0.2, OpenJDK 64-Bit Server VM, 11.0.2+9
# VM invoker: /Library/Java/JavaVirtualMachines/openjdk-11.0.2.jdk/Contents/Home/bin/java
# VM options: -javaagent:/Applications/IntelliJ IDEA CE.app/Contents/lib/idea_rt.jar=49590:/Applications/IntelliJ IDEA CE.app/Contents/bin -Dfile.encoding=UTF-8
# Warmup: 5 iterations, 10 s each
# Measurement: 4 iterations, 10 s each
# Timeout: 10 min per iteration
# Threads: 24 threads, will synchronize iterations

Summary - 4e0e752:

Benchmark                                                           Mode  Cnt       Score        Error  Units
SummaryBenchmark.prometheusSimpleSummaryBenchmark                   avgt    4     186.796 ±     15.029  ns/op
SummaryBenchmark.prometheusSimpleSummaryChildBenchmark              avgt    4      61.782 ±      5.032  ns/op
SummaryBenchmark.prometheusSimpleSummaryNoLabelsBenchmark           avgt    4      59.388 ±     12.457  ns/op
SummaryBenchmark.prometheusSimpleSummaryQuantilesBenchmark          avgt    4  530692.958 ± 627614.926  ns/op
SummaryBenchmark.prometheusSimpleSummaryQuantilesChildBenchmark     avgt    4  563807.323 ± 389413.917  ns/op
SummaryBenchmark.prometheusSimpleSummaryQuantilesNoLabelsBenchmark  avgt    4  559719.005 ± 631676.260  ns/op

HdrSummary - c8e56140:

Benchmark                                                                 Mode  Cnt     Score     Error  Units
HdrSummaryBenchmark.prometheusSimpleHdrSummaryBenchmark                   avgt    4   243.063 ±  47.352  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryChildBenchmark              avgt    4    64.539 ±   4.000  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryNoLabelsBenchmark           avgt    4    68.874 ±   5.327  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesBenchmark          avgt    4  4725.957 ± 299.873  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesChildBenchmark     avgt    4  5180.548 ± 599.410  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesNoLabelsBenchmark  avgt    4  5505.174 ±  21.149  ns/op

[ghost]

I'll be interested to see the benchmarks. I'd not have two Summary variants though, replacing what's there is what'd be considered.

I created a separate simpleclient_hdrhistogram module to avoid adding the org.hdrhistogram: HdrHistogram dependency to simpleclient.

[ghost]

I'm wondering if there's any kind of integration / system test suite which I could use to ensure that HdrSummary behaves the same and could be used as a drop-in replacement for Summary?

[brian-brazil]

The unittest for Summary should cover that.

We can always copy in the Hdr code if needed.

[ghost]

0dbe690a:

Benchmark                                                                 Mode  Cnt     Score    Error  Units
HdrSummaryBenchmark.prometheusSimpleHdrSummaryBenchmark                   avgt    4   177.918 ± 23.296  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryChildBenchmark              avgt    4    58.518 ±  9.194  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryNoLabelsBenchmark           avgt    4    67.881 ±  8.436  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesBenchmark          avgt    4  4664.426 ± 71.544  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesChildBenchmark     avgt    4  4605.121 ± 21.612  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesNoLabelsBenchmark  avgt    4  4451.485 ± 38.236  ns/op

[ghost]

f21fbf0c:

Benchmark                                                                 Mode  Cnt     Score     Error  Units
HdrSummaryBenchmark.prometheusSimpleHdrSummaryBenchmark                   avgt    4   209.753 ±  44.021  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryChildBenchmark              avgt    4    61.513 ±   1.254  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryNoLabelsBenchmark           avgt    4    67.730 ±   1.329  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesBenchmark          avgt    4  5014.461 ±  17.032  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesChildBenchmark     avgt    4  6177.009 ± 489.784  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesNoLabelsBenchmark  avgt    4  5186.091 ± 383.779  ns/op

[ghost]

We can always copy in the Hdr code if needed.

I'll check if we could use https://maven.apache.org/plugins/maven-shade-plugin/ .

[ghost]

Benchmarks with unrealistically frequent rotations by hardcoding HdrTimeWindowQuantiles. durationBetweenRotates.

The results were consistent across many runs over the last 2 days.

Before - 4e0e752 - durationBetweenRotates=1ms:

Benchmark                                                           Mode  Cnt      Score      Error  Units
SummaryBenchmark.prometheusSimpleSummaryBenchmark                   avgt    4    180.660 ±   31.025  ns/op
SummaryBenchmark.prometheusSimpleSummaryChildBenchmark              avgt    4     59.794 ±    2.183  ns/op
SummaryBenchmark.prometheusSimpleSummaryNoLabelsBenchmark           avgt    4     62.603 ±    2.560  ns/op
SummaryBenchmark.prometheusSimpleSummaryQuantilesBenchmark          avgt    4  16668.278 ±  899.351  ns/op
SummaryBenchmark.prometheusSimpleSummaryQuantilesChildBenchmark     avgt    4  16473.560 ± 1271.724  ns/op
SummaryBenchmark.prometheusSimpleSummaryQuantilesNoLabelsBenchmark  avgt    4  16279.621 ± 1513.338  ns/op

After - b2489083 - durationBetweenRotates=1ms:

Benchmark                                                                 Mode  Cnt     Score     Error  Units
HdrSummaryBenchmark.prometheusSimpleHdrSummaryBenchmark                   avgt    4   183.298 ±   5.568  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryChildBenchmark              avgt    4    60.277 ±   2.809  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryNoLabelsBenchmark           avgt    4    63.545 ±  31.810  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesBenchmark          avgt    4  4737.209 ± 202.428  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesChildBenchmark     avgt    4  4494.128 ± 101.534  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesNoLabelsBenchmark  avgt    4  5125.099 ± 649.673  ns/op

Before - 4e0e752 - durationBetweenRotates=100us:

Benchmark                                                           Mode  Cnt      Score     Error  Units
SummaryBenchmark.prometheusSimpleSummaryBenchmark                   avgt    4    190.128 ±  37.522  ns/op
SummaryBenchmark.prometheusSimpleSummaryChildBenchmark              avgt    4     59.393 ±   1.586  ns/op
SummaryBenchmark.prometheusSimpleSummaryNoLabelsBenchmark           avgt    4     62.860 ±   7.727  ns/op
SummaryBenchmark.prometheusSimpleSummaryQuantilesBenchmark          avgt    4  12261.290 ± 206.572  ns/op
SummaryBenchmark.prometheusSimpleSummaryQuantilesChildBenchmark     avgt    4  12279.661 ± 117.961  ns/op
SummaryBenchmark.prometheusSimpleSummaryQuantilesNoLabelsBenchmark  avgt    4  12317.630 ± 270.580  ns/op

After - b2489083 - durationBetweenRotates=100us:

Benchmark                                                                 Mode  Cnt      Score      Error  Units
HdrSummaryBenchmark.prometheusSimpleHdrSummaryBenchmark                   avgt    4    187.572 ±   12.372  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryChildBenchmark              avgt    4     57.810 ±    2.903  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryNoLabelsBenchmark           avgt    4     60.689 ±    8.472  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesBenchmark          avgt    4  37362.127 ±  839.878  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesChildBenchmark     avgt    4  29928.902 ±  300.892  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesNoLabelsBenchmark  avgt    4  37429.125 ± 2312.796  ns/op

[ghost]

580d689e:

Benchmark                                                                 Mode  Cnt     Score    Error  Units
HdrSummaryBenchmark.prometheusSimpleHdrSummaryBenchmark                   avgt    4   208.215 ± 35.954  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryChildBenchmark              avgt    4    63.688 ±  5.062  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryNoLabelsBenchmark           avgt    4    68.558 ±  3.316  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesBenchmark          avgt    4  5573.098 ± 14.488  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesChildBenchmark     avgt    4  4503.708 ± 13.022  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesNoLabelsBenchmark  avgt    4  6543.911 ± 42.473  ns/op

580d689e - durationBetweenRotates=1ms:

Benchmark                                                                 Mode  Cnt     Score     Error  Units
HdrSummaryBenchmark.prometheusSimpleHdrSummaryBenchmark                   avgt    4   186.682 ±   6.647  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryChildBenchmark              avgt    4    58.485 ±   2.135  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryNoLabelsBenchmark           avgt    4    61.128 ±   1.780  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesBenchmark          avgt    4  5908.102 ± 321.887  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesChildBenchmark     avgt    4  5614.081 ± 158.416  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesNoLabelsBenchmark  avgt    4  5615.604 ± 185.716  ns/op

580d689e - durationBetweenRotates=100us:

Benchmark                                                                 Mode  Cnt      Score     Error  Units
HdrSummaryBenchmark.prometheusSimpleHdrSummaryBenchmark                   avgt    4    186.267 ±  23.955  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryChildBenchmark              avgt    4     59.950 ±   4.620  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryNoLabelsBenchmark           avgt    4     65.844 ±   0.638  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesBenchmark          avgt    4  40217.638 ± 434.766  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesChildBenchmark     avgt    4  38471.865 ± 615.677  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesNoLabelsBenchmark  avgt    4  40003.732 ± 655.800  ns/op

[ghost]

1ae65868:

Benchmark                                                                 Mode  Cnt     Score     Error  Units
HdrSummaryBenchmark.prometheusSimpleHdrSummaryBenchmark                   avgt    4   209.209 ±  30.386  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryChildBenchmark              avgt    4    68.149 ±  21.612  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryNoLabelsBenchmark           avgt    4    66.847 ±   5.324  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesBenchmark          avgt    4  5275.542 ±  85.372  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesChildBenchmark     avgt    4  5429.180 ±  24.373  ns/op
HdrSummaryBenchmark.prometheusSimpleHdrSummaryQuantilesNoLabelsBenchmark  avgt    4  6861.641 ± 578.346  ns/op

[ghost]

I rebased the changes on master.
I'll share load testing and profiling results soon™️.

[ghost]

[ERROR] Failed to execute goal on project benchmarks: Could not resolve dependencies for project io.prometheus:benchmarks:jar:0.6.1-SNAPSHOT: Could not find artifact io.prometheus:simpleclient_hdrhistogram:jar:0.6.1-SNAPSHOT in sonatype-nexus-snapshots (https://oss.sonatype.org/content/repositories/snapshots)

I ran into the same issue with IntelliJ IDEA locally, it was trying to resolve nonexistent artifacts.
I fixed it by using ${project.version} instead of 0.6.1-SNAPSHOT for dependencies between modules.
It looks like the same fix doesn't work for CicleCI, but maybe there's a better way to handle this.

[brian-brazil]

The CI changed recently, so it's possible it doesn't handle it well. Let's worry about it at merge time.

[ghost]

I'll follow up regarding defaults in HdrHistogram/HdrHistogram#156.

[ghost]

I set the default initial dynamic range to 1000 and the default number of significant value digits to 2 based on previous suggestions.

[ghost]

I'd not have two Summary variants though, replacing what's there is what'd be considered.

We used the following binary compatible in-place upgrade version for testing:
Branch: https://github.com/rrakos-evo/client_java/commits/summary-hdr-in-place
Diff: master...rrakos-evo:summary-hdr-in-place

[ghost]

I found Summary in YourKit snapshots and screenshots from July. 🙁
I'll need to profile the changes again, so I'll have more recent results.

[ghost]

YourKit screenshots about blocked theads.

Before - 4e0e752:

After - 01108f1:

After - aaa9565:

[ghost]

YourKit screenshots about CPU time.

Before - 4e0e752:

After - 01108f1:

After - aaa9565:

[brian-brazil]

I'm not familiar with this tool, I think it's saying things are a fair bit better?

[ghost]

It's YourKit Java Profiler, similar to JProfiler, slightly smarter than VisualVM or JConsole.
https://www.yourkit.com/java/profiler/features/

I collected data for a couple of minutes from both versions under similar workload (hopefully).
I usually compare the relative amount of CPU time (or real time or memory) used by different classes and methods.

The Monitor Usage tab shows time spent waiting on synchronized, java.util.concurrent.locks, and probably I/O.
The CPU / Method List tab shows time spent by different methods (measured with sampling).

I didn't make proper notes, and I'd still like to test our most intensive code path, but I think it improved enough that it doesn't affect our latency and throughput anymore.

[ghost]

I'd like to get your view on how to proceed with this to get it merged, as the performance looks good now. We probably should wait for this fix to be merged in HdrHistogram HdrHistogram/HdrHistogram#156, so we have a reliable version to depend on. We should decide if it should be done as a drop in replacement or in a separate module. What do you think?

[brian-brazil]

It looks like at least an order of magnitude improvement, so I think we should go with replacement.

[ghost]

I can confirm that the performance overhead essentially disappeared allowing us to gather metrics in our hot code path.

I'll rebase the changes from #484 (comment) on master next week.

Example (the only change is metrics):

[t3hnar]

Hello @brian-brazil,
Do you expect some more work to be done within this issue? or you going to merge this in?

[ghost]

I was asked to double check if it's acceptable that negative measurements are not supported and how do you recommend to handle them (ignore, throw exception, etc.)?

Edit: I was told that it's a breaking API change that HdrHistogram throws ArrayIndexOutOfBoundsException.

[ghost]

We can always copy in the Hdr code if needed.

I'll check if we could use https://maven.apache.org/plugins/maven-shade-plugin/ .

I added the maven-shade-plugin to the in-place upgrade version.
Commit: https://github.com/rrakos-evo/client_java/commit/efed6d79e040f03308c5f55ffba18535e6048371

[ghost]

I'll update this Pull Request to the in-place upgrade version when HdrHistogram is released.

HdrHistogram/HdrHistogram#156 (comment)

[ghost]

I'm planning to finish this around the end of the year. I'm still waiting for the HdrHistogram release.

[mindw]

@rrakos-evo it seems HDRHistorgram has released a version with the required fix. Any chance the above great work can be complete?

[ghost]

I'm sorry for the delay, I'm planning to finish this around the end of next week, unless something urgent pops up.

I can confirm that this has been working fine in production without any issues in the last ~3 months. 🙂🐎🚀
https://github.com/rrakos-evo/client_java/commit/b192b69c4489d546e5c46f4423fa7ff8513f5d33

[ghost]

I might be able to pick this up in 2 weeks... 😞

[ghost]

I rebased the changes on 0.8.1 / a814b67.

I'll update this Pull Request to the in-place upgrade version after a couple of days of testing.
Branch: https://github.com/rrakos-evo/client_java/commits/summary-hdr-in-place
Diff: master...rrakos-evo:summary-hdr-in-place

[nikitapecasa]

@brian-brazil Hello, I hope you're doing well :)
Looks like it's ready to be merged, so if you agree, could you please merge the change and release new version?

Please note that this PR brings kinda breaking change in summary behaviour, before it was fine to pass negative measurements, and now it would result in runtime exception, also Histogram collector is not throwing exception on negative measurements, such behaviour might result in pretty unpleasant surprise for users, as most probably they're not handling exceptions from Summary collector (as it's currently is not throwing them).

[thestarmaker]

@brian-brazil @ghost sorry to be the Shrek Donkey, but are we there yet?
Jokes aside, how are you guys getting on with this? I am quite new to Prometheus but had been burned with broken histograms in the past on Dropwizard, now need to introduce metrics into a new project, would love to be able to expose HdrHistogram via Prometheus.

[brian-brazil]

The PR needs work still.

If you want histograms with Prometheus then the Histogram type is recommended, as client-side quantiles are not aggregatable.

[thestarmaker]

Thanks you Brian, going through the documentation just now.

[brian-brazil]

I'll soon be making various breaking changes to this client library to switch to the OpenMetrics data model, so if we're going to try to get this in now is a good time so that all the potential breakages could be in one release.

[lavrov]

I'm happy to help with this MR. Do you have something off the top of your head about what has to be fixed?

[brian-brazil]

Reading the above discussion, this would need to be made a drop-in replacement with no new dependencies.

[brian-brazil]

Superseded by #605