Skip to content

fix(otlp): complete post-#262 DDSketch encoding-value + proto wire-type absorption - #269

Merged
zzylol merged 1 commit into
mainfrom
fix/sweep-blocker-1-ddsketch-encoding-gap
May 5, 2026
Merged

zzylol merged 1 commit into
mainfrom
fix/sweep-blocker-1-ddsketch-encoding-gap

Conversation

@zzylol

@zzylol zzylol commented May 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Sweep blocker #1: completes the post-PR-#262 absorption that left
EXPORTER_SDK_AGG=dd-delta rejected at the OTLP exporter and
EXPORTER_SDK_AGG=dd-full rejected at the agent / backend decoder.
Without this, none of run_e2e_sweep.sh's DDSketch matrix cells run,
blocking paper claims ① ④ ⑤.

The PR delivers Fix 1 (encoding-string mapping) cleanly and pins the
agent-decoder wire-format invariants with regression tests. Fixes 2

  • 3 share a structural root cause that requires a sketchlib-go
    upstream migration outside this PR's touch list — see
    "User-attention" below.

Per-failure root cause

Failure 1 — unknown ddsketch encoding: ddsketch_proto_delta

Root cause: DDSketchEncodingValue in
exporters/otlp/otlpmetric/{otlpmetricgrpc,otlpmetrichttp}/internal/ transform/metricdata.go only matched
metricdata.DDSketchEncodingProto. PR #262 added
metricdata.DDSketchEncodingProtoDelta (+ the mpb.DDSketchEncoding_ DDSKETCH_ENCODING_PROTO_DELTA proto enum) but the encoding-switch
arm was not extended, so any data point carrying the new enum hit
the default-case errUnknownDDSketchEncoding.

Fix: added the missing arm in both exporters and the shared
template (internal/shared/otlp/otlpmetric/transform/ metricdata.go.tmpl). Sibling sketches (HLL / CountSketch /
CountMinSketch) were already absorbed in #262; only DDSketch was
missed. KLL has no delta path, so its single-arm switch is correct.

Failure 2 — DdSketchState.alpha: invalid wire type: LengthDelimited (expected SixtyFourBit) + DDSketch SketchEnvelope did not carry a DDSketchState variant

Root cause (single, structural): the SDK's DDSketch aggregator
(opentelemetry-go-patch/sdk/metric/internal/aggregate/ddsketch.go)
serializes via proto.Marshal(sk.ToProto()) against
github.com/DataDog/sketches-go/ddsketch/pb/sketchpb.DDSketch, but
every consumer (asap-precompute-go's DDSketchWrapper.ApplyDelta,
asap-precompute-rs's DDSketchWrapper::decode_envelope, the agent
processor's decodeDDSketchEnvelope) expects sketchlib-go's
SketchEnvelope{DDSketchState}. The two schemas collide on field 1:
DataDog's Mapping is a sub-message (LengthDelimited wire type)
while sketchlib-go's alpha is double (SixtyFourBit). When the
consumer's proto.Unmarshal hits the field-1 wire-type clash it
emits the reported error; when it doesn't, env.GetDdsketch()
returns nil (no oneof variant set in DataDog bytes) and surfaces
"did not carry a DDSketchState variant".

KLL / HLL / CountSketch / CountMinSketch already migrated to
sketchlib-go's portable format (see PR #262 §"sketchlib-go API
rename refactor (PR #53) absorption"); DDSketch remained on DataDog.
The right structural fix is to migrate DDSketch's aggregator to
sketchlib-go's *ddsketch.DDSketch and serialize via
SerializePortable + proto.Marshal. This sits outside the
sweep-blocker-1 touch list
— see User-attention below.

Test additions (3 new test files, 12 new test functions)

  1. opentelemetry-go-patch/exporters/otlp/otlpmetric/otlpmetricgrpc/ internal/transform/encoding_value_test.go (mirrored to the
    otlpmetrichttp sibling): 7 test functions
    (TestDDSketchEncodingValue, TestDDSketchEncodingValue_Unknown,
    TestKLLSketchEncodingValue_Proto,
    TestCountSketchEncodingValue,
    TestCountMinSketchEncodingValue, TestHLLSketchEncodingValue,
    TestDDSketchDataPointsCarriesProtoDeltaEncoding) covering both
    the new ProtoDelta arm and the sibling-sketch surface to catch
    future re-introductions of the same drift.
  2. opentelemetry-collector-contrib-patch/processor/ddsketchprocessor/ envelope_decode_test.go: 3 test functions
    (TestDecodeDDSketchEnvelope_RoundTripFull,
    TestDecodeDDSketchEnvelope_BareStateRejected,
    TestDecodeDDSketchEnvelope_EmptyEnvelopeRejected) pinning the
    agent decoder's wire-format contract.

Sibling sketches — open question

None left. The post-#262 absorption gap was DDSketch-only.
HLL / CountSketch / CountMinSketch all have working Delta
encoding-value arms verified by the new sibling tests. KLL has no
delta path, which is correct.

Doc-tightening

No stale post-rename comments referencing pre-rename API names
(Add / InsertValue / GetValueAtQuantile etc.) found in any
file touched by this PR. The remaining series.sketch.Add(...)
call in internal/aggregate/ddsketch.go is DataDog's API, not
sketchlib-go's pre-rename API; it'll be replaced when the
structural migration lands.

User attention required

This PR delivers Fix 1 fully and pins decoder invariants. Fixes 2
and 3 require user attention
because they share a structural root
cause that can't be resolved within sweep-blocker-1's touch list:

  • The alpha wire-type mismatch is structural — the proto schema
    (sketchlib-go's DDSketchState) and the SDK encoder (DataDog's
    sketchpb.DDSketch) cannot be aligned without a semantic change.
  • The fix needs a sketchlib-go ↔ SDK migration: rewrite
    opentelemetry-go-patch/sdk/metric/internal/aggregate/ddsketch.go
    to use github.com/ProjectASAP/sketchlib-go/sketches/DDSketch (as
    KLL/HLL/CountSketch/CountMinSketch already do) and emit via
    SerializePortable + proto.Marshal for full state and
    ddsketch.ComputeDelta for the delta encoding. That file is on
    the sweep-blocker-1 do-not-touch boundary (the touch list scopes
    to encoding-value map + OTLP encoding switch + processor decoder
    • Rust wrapper).

Test plan

  • go test ./internal/transform/... green for otlpmetricgrpc
    + otlpmetrichttp exporters.
  • go test ./... green for the patched
    opentelemetry-collector-contrib/processor/ddsketchprocessor.
  • go test ./... green for opentelemetry-go/sdk/metric/....
  • cargo test --release green for asap-precompute-rs (no
    Rust changes; parity tests pass).
  • (Out of this PR's scope) Run run_e2e_sweep.sh after the
    structural follow-up lands to confirm the matrix's DDSketch
    cells produce real PromQL answers.

🤖 Generated with Claude Code

…pe absorption

Sweep blocker #1: `EXPORTER_SDK_AGG=dd-delta` was rejected by the OTLP
exporter with `unknown ddsketch encoding: ddsketch_proto_delta` because
PR #262 introduced `metricdata.DDSketchEncodingProtoDelta` but did not
extend `DDSketchEncodingValue`'s switch to cover it (sibling sketches
HLL/CountSketch/CountMinSketch were absorbed in #262 but DDSketch was
missed). Without this, none of the matrix's DDSketch cells run.

Fix 1 (encoding-value mapping): adds the `ProtoDelta` arm to
`DDSketchEncodingValue` in `otlpmetricgrpc` + `otlpmetrichttp`
exporters and the shared template. New `encoding_value_test.go` pins
the post-#262 surface for all five sketches' `*EncodingValue`
helpers, including a round-trip through `DDSketchDataPoints`.

Pinning tests for the agent-side decoder in
`opentelemetry-collector-contrib-patch/processor/ddsketchprocessor/
envelope_decode_test.go` document the wire-format invariants
`decodeDDSketchEnvelope` relies on (full-state round trip,
bare-state rejection, empty-envelope rejection) so any future
encoder regression is caught at the unit-test layer.

Fix 2 (alpha wire-type) + Fix 3 (oneof variant) are NOT addressed in
this PR. Investigation confirmed they share a single structural root
cause: the SDK's DDSketch aggregator
(`opentelemetry-go-patch/sdk/metric/internal/aggregate/ddsketch.go`)
still serializes via `proto.Marshal(sk.ToProto())` — DataDog's
`sketchpb.DDSketch` proto — while every consumer
(asap-precompute-go's `DDSketchWrapper.ApplyDelta`,
asap-precompute-rs's `DDSketchWrapper::decode_envelope`, the agent's
`decodeDDSketchEnvelope`) expects sketchlib-go's
`SketchEnvelope{DDSketchState}`. The schemas collide on field 1 wire
types (DataDog's `Mapping` is LengthDelimited; sketchlib-go's `alpha`
is fixed64), surfacing as the reported
`DDSketchState.alpha: invalid wire type` error. Aligning consumers
with the SDK's DataDog format would lose the cross-language byte
parity #243 just landed; aligning the SDK
with sketchlib-go requires migrating the aggregator off
DataDog/sketches-go onto sketchlib-go's DDSketch — same path KLL,
HLL, CountSketch, CountMinSketch already took. That migration sits
outside this PR's touch list (only the encoding-value map, OTLP
encoding-switch, processor.go decoder, and Rust wrapper were in
scope per the sweep agent's autopilot envelope).

Verification:
- `go test ./internal/transform/...` green for both grpc + http
  exporters (8 new tests under `encoding_value_test.go`).
- `go test ./...` green for `opentelemetry-collector-contrib/
  processor/ddsketchprocessor` including the 3 new envelope-decode
  tests.
- `go test ./...` green for `opentelemetry-go/sdk/metric/...`.
- `cargo test --release` green for `asap-precompute-rs` (no Rust
  changes; verified parity tests still pass).

No sibling sketch (HLL/CS/CMS) gap was found — the original PR #262
absorption is complete for them; only DDSketch was missed.
KLLSketchEncodingValue intentionally has no Delta arm (KLL has no
delta wire path).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zzylol
zzylol merged commit fc182eb into main May 5, 2026
@zzylol
zzylol deleted the fix/sweep-blocker-1-ddsketch-encoding-gap branch May 5, 2026 22:06
zzylol added a commit that referenced this pull request May 5, 2026
…etchlib-go (post-#262) (#270)

PR #262 migrated KLL / HLL / CountSketch / CountMinSketch off DataDog
sketches-go onto sketchlib-go but left DDSketch on DataDog. PR #269
diagnosed the resulting structural mismatch — DataDog's `Mapping` is a
`LengthDelimited` sub-message at field 1 while sketchlib-go's `alpha`
is a `SixtyFourBit` `double` at field 1, so every consumer of the SDK's
DDSketch envelope (asap-precompute-{go,rs}, the agent's
ddsketchprocessor decoder, the ASAPQuery backend) failed to decode the
SDK's emitted bytes. This blocked all `EXPORTER_SDK_AGG=dd-{full,delta}`
cells in `run_e2e_sweep.sh`, which in turn blocks paper claims (1)(4)(5)
for the DDSketch row of the §accuracy table.

Migration shape (mirrors KLL/HLL/CS/CMS siblings):

- Imports: drop `github.com/DataDog/sketches-go/{ddsketch,pb/sketchpb}`,
  pick up `github.com/ProjectASAP/sketchlib-go/sketches/DDSketch`.
- Series struct: hold `*ddsketch.DDSketch` (sketchlib-go) keyed off
  `alpha` carried through `NewDDSketch(alpha)`.
- `measure`: invoke sketchlib-go's `Update(float64)` (silently drops
  non-positive / NaN / Inf, matching the consumer-side invariant).
- Full-state serialization: `SerializePortable() + proto.Marshal`,
  stripping `Producer` / `HashSpec` so bytes match the
  asap-precompute-{go,rs} wrapper outputs and the parity golden
  fixtures (`integration/parity/golden_test.go`).
- Delta path: replace the hand-rolled `sketchpb.DDSketch` bucket-diff
  with sketchlib-go's `ddsketch.ComputeDelta(prev, curr, threshold)`,
  caching `*DDSketch` clones (not bytes) per-series so the next-tick
  delta has a typed snapshot to walk directly.
- Encoding signal: emit `metricdata.DDSketchEncodingProto` for full,
  `DDSketchEncodingProtoDelta` for delta — both already wired through
  the OTLP exporter encoding-value switch by PR #269.

Pool reuse note: sketchlib-go's DDSketch has no `Reset()`, so
`newSeries` allocates a fresh sketch on each call. The
`ddSketchSeries[N]` struct header is still pooled, which is the
dominant allocation cost on the hot path (the buckets backing array
itself starts at 128 entries and grows lazily).

Test additions:

- `TestDDSketchPayloadIsSketchlibPortableEnvelope`: confirms the
  SDK's DDSketchDataPoint.Sketch payload now round-trips through
  `proto.Unmarshal` into an `envpb.SketchEnvelope` carrying a
  `DDSketchState` — the exact decode path the agent processor
  takes via `decodeDDSketchEnvelope` and asap-precompute-rs's
  `DDSketchWrapper::decode_envelope`.
- `TestDDSketchDeltaEncodingViaComputeDelta`: confirms the
  cumulative-mode `deltaTransmission=true` path emits a
  `DDSketchEncodingProtoDelta` payload on the second tick (after a
  snapshot exists), and that the delta bytes feed cleanly into
  sketchlib-go's `ApplyDelta`.

Cross-language byte-parity preserved: the existing
`asap-precompute-rs::ddsketch_byte_parity_with_go` test still passes
unchanged — both sides now route through sketchlib-go's
`SerializePortable`, so the SDK and the asap-precompute-{go,rs}
wrappers emit byte-identical envelopes.

DataDog dependency cleanup: `sdk/metric/go.mod` and
`deploy/fake-exporter/go.mod` no longer pull
`github.com/DataDog/sketches-go` (was a direct require / indirect
respectively); the larger `sdk/go.mod` still carries it as indirect
(transitive from upstream OTel — left alone, out of scope).

LOC: ~150 net lines source (193 add / 122 remove in ddsketch.go);
+111 lines new test functions; documentation refresh in
metricdata/data.go for the `DDSketchEncodingProto{,Delta}` constants.

Verification:

- `go test ./opentelemetry-go-patch/sdk/metric/...` green.
- `go test ./opentelemetry-collector-contrib-patch/processor/ddsketchprocessor/...` green.
- `go test ./asap-precompute-go/...` green.
- `cargo test --release -p asap-precompute-rs ddsketch` green
  (cross-language parity intact).
- `go build ./deploy/fake-exporter/...` clean.
- `integration/parity` golden fixtures unchanged.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant