Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9010 +/- ##
==========================================
+ Coverage 64.52% 64.60% +0.08%
==========================================
Files 289 290 +1
Lines 37366 37411 +45
==========================================
+ Hits 24111 24170 +59
+ Misses 11157 11150 -7
+ Partials 2098 2091 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Adds `zstd` alongside the existing `snappy`/`none` choices for --receive.grpc-compression, mirroring the existing pkg/extgrpc/snappy compressor implementation with github.com/klauspost/compress/zstd (already a direct dependency). No server-side changes are needed since grpc's encoding registry auto-selects the decompressor from the grpc-encoding header once a compressor is registered. Closes thanos-io#9009 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013KaQWsirNZChXNYuo8su1b Signed-off-by: heonjinjeong <hj.jeong@okestro.com>
17120b6 to
f087f71
Compare
|
The failing e2e shard (Thanos end-to-end tests (8, 4)) looks like an unrelated CI infra flake, not something caused by this change:
I don't have permission to re-run the job myself ( |
The e2e shard failure on the previous push (Thanos end-to-end tests (8, 4)) looks like an unrelated CI infra flake in TestReceive/multitenant_active_series_limiting (Docker embedded-DNS resolution failure), not something caused by this change. Empty commit to get a fresh run on a clean runner. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013KaQWsirNZChXNYuo8su1b Signed-off-by: heonjinjeong <hj.jeong@okestro.com>
|
Update: re-ran CI (empty commit This is a Docker Hub pull failure for the pinned e2e test-fixture image I also reproduced a (differently-shaped, but same class of) container-network flake running the originally-failing subtest locally via Docker, for what it's worth — logged in case it's useful context, though the minio pull failure above is the much stronger and more specific signal here. Given this is Docker Hub availability, not code, happy to keep retrying, but wanted to flag the root cause in case there's a mirror/cache that should be pointed at for |
Summary
zstdas a new option for--receive.grpc-compression, alongside the existingsnappyandnone. Default stayssnappy(no behavior change for existing deployments).pkg/extgrpc/zstdpackage implements agrpc/encoding.Compressorusinggithub.com/klauspost/compress/zstd(already a direct dependency viapkg/extgrpc/snappy's sibling package, no new dependency added), mirroring the existingpkg/extgrpc/snappyimplementation (sync.Pool-backed encoders/decoders, registered viaencoding.RegisterCompressor).grpc-encodingheader, sopkg/receive/handler.go's peer dial-option plumbing (Options.DialOpts→peerGroup.dialOpts) picks it up unchanged.Closes #9009.
Benchmark (real data, not synthetic)
Measured on real
WriteRequestpayloads built from live avalanche-generated series (50 metrics × 100 series, real label cardinality), compressed with the actualpkg/extgrpc/snappy/pkg/extgrpc/zstdcode, and sent over a real loopback gRPCRemoteWritecall withgrpc.UseCompressorset exactly ascmd/thanos/receive.godoes. Wire bytes measured via grpc'sstats.OutPayload.WireLength(not estimated).Compression ratio (compressed / raw), by batch size:
At realistic replication batch sizes,
zstdproduces roughly 70% smaller wire payloads thansnappyon this dataset.Pure compress/decompress CPU cost (no network, 5,000-series batch, 200 iterations):
zstdcosts ~2.3x more CPU to compress and ~1.7x more to decompress thansnappy, but this adds well under a millisecond to the end-to-endRemoteWriteRPC latency in this test (2.88ms → 3.29ms for the 5,000-series batch).Caveat: the ratio numbers above are likely an upper bound — the label patterns in this synthetic dataset repeat heavily across series, which favors compressors with larger windows (
zstd) more than real-world cardinality/entropy would. A related discussion on the Prometheus side (prometheus/prometheus#13366) cites a more conservative real-world number (~+4% CPU / -30% bandwidth for RW2). Directionally consistent, but the absolute percentages here shouldn't be taken as universal.Conclusion:
zstdis opt-in rather than a default change, since it's a straightforward win only when replication bandwidth (not CPU) is the actual constraint — e.g. hashrings replicating across AZs/regions, or high replication-factor deployments. Same-rack/low-latency-network deployments have little to gain and pay a real CPU cost, hence keepingsnappyas the default.Test plan
go build ./pkg/extgrpc/... ./cmd/thanos/...go test ./pkg/extgrpc/zstd/... ./pkg/extgrpc/snappy/...— newTestZstdround-trip test passes (mirrors existingTestSnappy)go vet ./pkg/extgrpc/... ./cmd/thanos/...thanosbinary and confirmed--receive.grpc-compression=zstdis accepted by the flag parser and an invalid value is still rejected--helpoutput anddocs/components/receive.mdregenerated/updated to listzstdin the allowed values🤖 Generated with Claude Code
https://claude.ai/code/session_013KaQWsirNZChXNYuo8su1b