Batch metrics: compact captures#19265
Conversation
By default, HdrHistogram's `Recorder` uses ConcurrentHistogram internally to ensure concurrency-safety with regard to updates, but that means that the values produced by `Recorder#getIntervalHistogram` carry all of the concurrency-safety overhead of that implementation. We already make copies of these histograms for storage in captures, and we do NOT update these objects once they are created; we can safely use the lower overhead basic `Histogram` implementation.
🤖 GitHub commentsJust comment with:
|
|
This pull request does not have a backport label. Could you fix it @yaauie? 🙏
|
yaauie
added
the
backport-active-9
TL;DRJava unit tests are failing because Remediation
Investigation detailsRoot CauseThe PR changes
But Histogram sample = new Recorder(3).getIntervalHistogram();
int sampleOccupation = sample.getEstimatedFootprintInBytes();
...
int expectedOccupation = sampleOccupation * totalDatapoints;
assertEquals(expectedOccupation, sut.estimateBatchMetricsFootprintInBytes());That expectation no longer matches the implementation, because Evidence
Verification
Follow-upThe prefetched Buildkite log for this job only contained the artifact-upload tail, so the assertion excerpt above is from a targeted local reproduction of the PR patch. What is this? | From workflow: PR Buildkite Detective Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
|
Both test failures are around calculating the effective total storage size. I'll follow up with fixes to both:
|
|
I think the production formula itself is now wrong: 3 * estimateSingleHistogramFootprintInBytes was written to approximate the recorder's 2 live histograms + 1 snapshot and that only worked because all three used to be the same (unpacked) size class. Now the snapshot shrank ~100x while the other two stayed full-size. This will likely lead to incorrect sizes, which is what the failing tests are signaling. |
2 participants
Release notes
What does this PR do?
Uses
PackedHistogramto store histogram captures in a significantly more compact manner, which is safe because the captured histograms are NOT modified once captured.ConcurrentHistogramcarries double overhead so that it can isolate "active" and "inactive" sets; we don't need this since our captures are never updated.PackedHistogramis very compact and recommended for sparse values, with a note that the resize and re-pack operations can be slower; this doesn't affect us because we never update the values once created.Why is it important/What is the impact to the user?
Significantly reduces the memory overhead of storing batch metrics.
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files (and/or docker env variables)Author's Checklist
How to test this PR locally
mainwith a generator:org.logstash.instrument.metrics.HistogramCaptureIn my local testing where batches were consistently full with 10,000 events (truly optimal), the size per capture was 848b when packed vs 82,500b without this patch. Since we need to keep hundreds of captures for each metric on each pipeline to satisfy the retention windows, this space savings is multiplied significantly in the real world.
When the values are less sparse, the savings will still be at least 50% since we're not carrying the concurrent update overhead.
Related issues
Relates: #18243
Use cases
On a host with many pipelines, batch metrics can occupy a significant amount of memory. By using a more compact representation for unmodified captures, we can reduce the overall memory footprint.