Skip to content

fix(ddsketch): make delta_transmission actually work end-to-end - #210

Merged
zzylol merged 1 commit into
mainfrom
fix/ddsketch-delta-emit
May 1, 2026
Merged

zzylol merged 1 commit into
mainfrom
fix/ddsketch-delta-emit

Conversation

@zzylol

@zzylol zzylol commented May 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Three connected bugs that meant `delta_transmission: true` on the DDSketch processor silently behaved like full-state-only after the first window. End-to-end verification against the b3-delta config now shows three consecutive windows ingesting cleanly:

```
W1: OTLP modified-proto sketch ingest: 1000 routed, 0 decode-failed (full)
W2: OTLP modified-proto sketch ingest: 1000 routed, 0 decode-failed (delta)
W3: OTLP modified-proto sketch ingest: 1000 routed, 0 decode-failed (delta)
```

Pre-fix: W2/W3 logged `0 routed, 1000 decode-failed (fallback)` or `OTLP delta-sketch arrived before any base snapshot`.

Bugs

(1) Typed encoding always set to `Proto` (full)

```go
dp.SetEncoding(pmetric.DDSketchEncodingProto) // always — even for deltas
```

The other four sketch processors (CMS / CountSketch / HLL) already switch on the encoding string and emit the matching typed encoding. DDSketch was the outlier. The backend dispatches on `dp.encoding` (int), so deltas tagged as `Proto` landed in the proto_full decoder which tried to parse them as a `SketchEnvelope` and failed.

Fix: switch on the local `encoding` variable (already populated to `proto_full` / `proto_delta`) and call the matching `pmetric.DDSketchEncoding*` setter.

(2) `computeDDSketchDelta` was a stub returning full state

```go
func computeDDSketchDelta(snapPayload []byte, current *ddsketch.DDSketch, threshold uint64) ([]byte, error) {
_ = snapPayload
_ = threshold
return serializeDDSketch(current) // <-- always full state
}
```

Comment claimed sketchlib-go's delta encoder wasn't ready. It is — `sketches/DDSketch/delta.go` ships `ComputeDelta(snapshot, current, threshold) ([]byte, error)` producing proto bytes wire-compatible with the backend's `asap_otel_proto::sketchlib::v1::DdSketchDelta`.

Fix: deserialize `snapPayload` into a `*DDSketch` (`proto.Unmarshal` → `env.GetDdsketch()` → `ddsketch.NewFromState`), then call `ddsketch.ComputeDelta`.

(3) `ddsketch.encoding` attribute broke per-series snapshot lookup

Even with (1) + (2) fixed, deltas failed with `OTLP delta-sketch arrived before any base snapshot`. The backend caches snapshots keyed by the data point's full attribute set. The agent was adding:

```go
dp.Attributes().PutStr("ddsketch.encoding", encoding)
```

So full frames carried `…,ddsketch.encoding=proto_full,…` while delta frames carried `…,ddsketch.encoding=proto_delta,…` — different attribute set → different `series_key` → cache miss on every delta.

Fix: drop the attribute. The typed `Encoding()` field is the source of truth and what the backend dispatches on. Sibling processors don't carry the attribute either.

Companion required on backend side

Three full-state DDSketch envelopes for 1000 series exceeds the default tonic gRPC receive cap (4 MiB) — the gateway-to-backend export retries forever with `decoded message length too large: found 17103632 bytes`. Backend PR pending: bump `max_decoding_message_size` to 64 MiB to match what agent / gateway already do on their own receivers.

Test plan

  • `build_sketchcollector.sh --skip-patches` clean.
  • First window flush: backend decodes full state (`encoding=Proto`, 1000 routed).
  • Second window flush: backend looks up per-series snapshot, applies delta, routes (`encoding=ProtoDelta`, 1000 routed, 0 decode-failed).
  • Third window flush: same as second.
  • PromQL query against the materialized data — separate residual issue (worker emits to store, but query-side lookup returns `No precomputed outputs found`; data is in the store per the runtime info endpoint). Independent of this PR.

🤖 Generated with Claude Code

Three connected bugs that meant `delta_transmission: true` silently
behaved like full-state-only after the first window:

## (1) Typed encoding always set to ProtoFull

`SetEncoding(pmetric.DDSketchEncodingProto)` ran unconditionally on
every emitted DDSketchDataPoint, regardless of whether `payload`
actually carried a full state or a delta. The other four sketch
processors (CMS / CountSketch / HLL) already switch on the
encoding string and emit the matching typed encoding —
`*EncodingDelta` for deltas, `*EncodingProto` for full. DDSketch
was the outlier.

Backend's `decode_modified_otlp_sketch_bytes` dispatches on the
typed `dp.encoding` int, not the `ddsketch.encoding` attribute
string. With the always-ProtoFull tag, second-window delta bytes
landed in the proto_full decoder which tried to parse them as a
`SketchEnvelope` and failed → the whole batch became
`decode-failed (fallback)`.

Fix: switch on the local `encoding` variable (already populated
to `proto_delta` / `proto_full`) and call the matching
`pmetric.DDSketchEncoding*` setter. Mirrors what
countminsketchprocessor / countsketchprocessor / hllprocessor
already do.

## (2) `computeDDSketchDelta` was a stub

```go
func computeDDSketchDelta(snapPayload []byte, current *ddsketch.DDSketch, threshold uint64) ([]byte, error) {
    _ = snapPayload
    _ = threshold
    return serializeDDSketch(current)  // <-- always full state
}
```

Comment claimed "the sketchlib-go-side delta encoder isn't
generated yet". It IS — `sketches/DDSketch/delta.go` ships
`ComputeDelta(snapshot, current *DDSketch, threshold uint64) ([]byte, error)`
producing proto-marshalled `pb.DDSketchDelta` bytes that are
byte-for-byte compatible with the backend's
`asap_otel_proto::sketchlib::v1::DdSketchDelta`. So the agent
was emitting full-state bytes BUT (after fix #1) tagging them as
PROTO_DELTA — the backend would then look up the per-series
snapshot, hand the bytes to `apply_proto_delta_bytes` which
expected a delta proto, and fail decode.

Fix: deserialize `snapPayload` (the previously-emitted envelope
bytes) into a `*DDSketch` via `proto.Unmarshal` →
`env.GetDdsketch()` → `ddsketch.NewFromState(...)`, then call
sketchlib-go's `ComputeDelta(snapshot, current, threshold)`. The
result is what the backend's apply path expects.

## (3) `ddsketch.encoding` attribute broke series_key matching

Even with (1) and (2) fixed, the second window still failed —
this time with `OTLP delta-sketch arrived before any base
snapshot`. The backend caches per-series snapshots keyed by the
data point's full attribute set, and a delta lookup that misses
the cache drops the frame.

The agent was setting `dp.Attributes().PutStr("ddsketch.encoding", encoding)`
on every typed DDSketchDataPoint. So full frames carried
`…,ddsketch.encoding=proto_full,…` while delta frames carried
`…,ddsketch.encoding=proto_delta,…`. Different attribute set →
different `series_key` → cache miss on every delta lookup.

Fix: drop the attribute. The typed `Encoding()` field is the
source of truth and what the backend dispatches on. Sibling
processors don't carry the attribute either.

## Verification

End-to-end with `b3-delta` (`delta_transmission: true`,
`window_duration: 60s`) — three sequential windows now decode
cleanly:

```
W1: OTLP modified-proto sketch ingest: 1000 routed, 0 decode-failed (full)
W2: OTLP modified-proto sketch ingest: 1000 routed, 0 decode-failed (delta)
W3: OTLP modified-proto sketch ingest: 1000 routed, 0 decode-failed (delta)
```

Pre-fix: W2 + W3 logged `0 routed, 1000 decode-failed (fallback)`
or `OTLP delta-sketch arrived before any base snapshot`.

Companion fix on the backend side (PR-pending in
ASAPQuery-backend) bumps the OTLP gRPC receiver's
`max_decoding_message_size` from tonic's 4 MiB default to 64 MiB
to match what agent / gateway already declare on their
receivers — required when the first-window full-state batch
runs ~17 MiB at 1k cardinality.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zzylol
zzylol merged commit 71b76ce into main May 1, 2026
@zzylol
zzylol deleted the fix/ddsketch-delta-emit branch May 1, 2026 16:10
zzylol added a commit that referenced this pull request May 1, 2026
So a single pipeline like `[ddsketch, hll, batch]` actually works.

## Bug

ddsketch / kll / hll processors in `mode: window` returned `nil`
from `ConsumeMetrics` — they accumulated inputs into their
windowed state and dropped the input batch on the floor without
forwarding it to the next consumer:

```go
case ModeWindow:
    p.accumulateIntoWindow(md)
    return nil   // <-- inputs go nowhere
```

In a chained pipeline `[ddsketch, hll, batch]`, that means:

1. otlp delivers raw `http_requests_total` → ddsketch.ConsumeMetrics.
   ddsketch accumulates, returns nil. **HLL never sees the raw input.**
2. ddsketch's window timer fires → it emits typed-DDSketch metrics
   to its nextConsumer (HLL.ConsumeMetrics).
3. HLL.accumulateIntoWindow has only `case MetricTypeGauge` and
   `case MetricTypeHLLSketch` in its switch — typed DDSketch
   metrics fall through and HLL **silently drops them**.
4. batch sees nothing.

Net: with multiple windowed sketch processors in one pipeline,
nothing reaches the exporter.

countminsketchprocessor / countsketchprocessor already do the
right thing — their ModeWindow paths return `md` (forward
inputs) by default, with an opt-in `drop_original` flag. The
other three were the outliers.

## Fix

Match the CMS/CountSketch pattern: forward `md` unchanged after
accumulating. Same one-liner in all three processors:

```go
case ModeWindow:
    p.accumulateIntoWindow(md)
    return p.nextConsumer.ConsumeMetrics(ctx, md)
```

Now the chain works as expected:

- `ddsketch.ConsumeMetrics(raw)`: accumulate, forward raw to HLL.
- `HLL.ConsumeMetrics(raw)`: accumulate, forward raw to batch.
- `batch.ConsumeMetrics(raw)`: batch & ship.
- ddsketch tick fires → typed-DDSketch → HLL (no match, forward) → batch.
- HLL tick fires → typed-HLL → batch.

So both DDSketch and HLL sketches reach the exporter in addition
to the original raw inputs. Users who don't want the raw inputs
shipped can add a `filter` processor at the end of the pipeline.

## Verification

End-to-end with `b3-delta` agent
(`processors: [ddsketch, HLL, batch]`, `delta_transmission: true`,
`window_duration: 60s`):

```
W1 (16:21:09):
  Ddsketch received: dps=1000
  Hllsketch received: dps=1000   ← HLL now reaches backend
  ingest: 1000 routed, 0 decode-failed, 1000 unconfigured
W2 (16:22:09):
  Ddsketch received: dps=1000   (delta)
  ingest: 1000 routed, 0 decode-failed, 1000 unconfigured
```

Pre-fix: HLL never reached backend; only ddsketch's outputs were
emitted (and only after #210's DDSketch delta wire fix).

## Supersedes #209

PR #209 worked around this bug at the config level by splitting
`[ddsketch, HLL, batch]` into two parallel pipelines
(`metrics/ddsketch` and `metrics/hll`). That goes against the
goal of one pipeline that supports any controller-chosen
combination of sketches. With this fix, the original chained
config works directly. #209 can be closed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol added a commit that referenced this pull request May 1, 2026
)

So a single pipeline like `[ddsketch, hll, batch]` actually works.

## Bug

ddsketch / kll / hll processors in `mode: window` returned `nil`
from `ConsumeMetrics` — they accumulated inputs into their
windowed state and dropped the input batch on the floor without
forwarding it to the next consumer:

```go
case ModeWindow:
    p.accumulateIntoWindow(md)
    return nil   // <-- inputs go nowhere
```

In a chained pipeline `[ddsketch, hll, batch]`, that means:

1. otlp delivers raw `http_requests_total` → ddsketch.ConsumeMetrics.
   ddsketch accumulates, returns nil. **HLL never sees the raw input.**
2. ddsketch's window timer fires → it emits typed-DDSketch metrics
   to its nextConsumer (HLL.ConsumeMetrics).
3. HLL.accumulateIntoWindow has only `case MetricTypeGauge` and
   `case MetricTypeHLLSketch` in its switch — typed DDSketch
   metrics fall through and HLL **silently drops them**.
4. batch sees nothing.

Net: with multiple windowed sketch processors in one pipeline,
nothing reaches the exporter.

countminsketchprocessor / countsketchprocessor already do the
right thing — their ModeWindow paths return `md` (forward
inputs) by default, with an opt-in `drop_original` flag. The
other three were the outliers.

## Fix

Match the CMS/CountSketch pattern: forward `md` unchanged after
accumulating. Same one-liner in all three processors:

```go
case ModeWindow:
    p.accumulateIntoWindow(md)
    return p.nextConsumer.ConsumeMetrics(ctx, md)
```

Now the chain works as expected:

- `ddsketch.ConsumeMetrics(raw)`: accumulate, forward raw to HLL.
- `HLL.ConsumeMetrics(raw)`: accumulate, forward raw to batch.
- `batch.ConsumeMetrics(raw)`: batch & ship.
- ddsketch tick fires → typed-DDSketch → HLL (no match, forward) → batch.
- HLL tick fires → typed-HLL → batch.

So both DDSketch and HLL sketches reach the exporter in addition
to the original raw inputs. Users who don't want the raw inputs
shipped can add a `filter` processor at the end of the pipeline.

## Verification

End-to-end with `b3-delta` agent
(`processors: [ddsketch, HLL, batch]`, `delta_transmission: true`,
`window_duration: 60s`):

```
W1 (16:21:09):
  Ddsketch received: dps=1000
  Hllsketch received: dps=1000   ← HLL now reaches backend
  ingest: 1000 routed, 0 decode-failed, 1000 unconfigured
W2 (16:22:09):
  Ddsketch received: dps=1000   (delta)
  ingest: 1000 routed, 0 decode-failed, 1000 unconfigured
```

Pre-fix: HLL never reached backend; only ddsketch's outputs were
emitted (and only after #210's DDSketch delta wire fix).

## Supersedes #209

PR #209 worked around this bug at the config level by splitting
`[ddsketch, HLL, batch]` into two parallel pipelines
(`metrics/ddsketch` and `metrics/hll`). That goes against the
goal of one pipeline that supports any controller-chosen
combination of sketches. With this fix, the original chained
config works directly. #209 can be closed.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol added a commit that referenced this pull request May 1, 2026
…able e2e (#212)

Capture this session's merged work at the top, retire stale
follow-ups, add new ones discovered along the way.

New top section: "Single-pipeline multi-sketch + delta +
queryable warm tier (2026-05-01)" — itemises #210 (DDSketch
delta wire fixes — three connected bugs), #211 (windowed
processors pass-through so chained `[ddsketch, HLL, batch]`
works), and the companion backend PRs (#70 OTLP gRPC max msg
size, #71 store overlap filter + closest-pane + window
annotation). Live e2e capture pasted in as evidence.

Retired follow-up #1 ("Warm-tier sketch ingest is dropped at
the gateway") — superseded by the new top section. Replaced
the lingering "agent-side ddsketch→batch wiring quirk" note
that was the chain bug now fixed by #211.

Replaced the abbreviated follow-up list (#2 cold reader,
#3 reducer self-check) with the actual residual after this
session, in priority order:
  1. Run the P7 sweep + accuracy reducer over all five sketches.
  2. Backend in-memory sketch_snapshots cache loss on restart.
  3. Inference config breadth (only [1m] patterns today).
  4. Cold reader torn-line tolerance (carried over).
  5. Reducer self-check vs engine (carried over).
  6. Inline GOPRIVATE / GOTOOLCHAIN env vars in build_sketchcollector.sh.
  7. Decide on OTel submodule bumps still dirty in working tree.

Updated `_Last updated_` to 2026-05-01.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol added a commit that referenced this pull request May 2, 2026
…post-#211 sink assertions (#220)

Why:
- DataDog/sketches-go is no longer a runtime dep (production
  switched to sketchlib-go in #210). Tests that still imported
  DataDog/sketches-go to build sketch fixtures were leftover.
- After #211, window-mode ConsumeMetrics forwards raw inputs
  through nextConsumer so chained windowed sketch processors can
  see the original payload. Tests written before #211 expected
  sink.AllMetrics() to be empty after ConsumeMetrics; they now
  observe both the input pass-through AND the synthesized output
  emitted by flushWindow. Update assertions to count both and
  scan-by-name where ordering is non-deterministic.
- KLL/CountSketch/CountMinSketch processors imported
  go.opentelemetry.io/collector/processor/selfmonitor (the local
  fork's selfmonitor package) but the corresponding go.mod was
  missing the `replace` directive that points
  go.opentelemetry.io/collector/processor at the local fork. Add
  the replace; drop the now-unused gob deserializer in two test
  sites where the test was reading proto-encoded emit bytes.

Per-processor:
- ddsketchprocessor: fix 5 window-mode tests + concurrent test;
  treat sink as [forwarded inputs..., synthesized flush output].
- kllprocessor: add `replace go.opentelemetry.io/collector/processor`
  to go.mod, fix 5 window-mode test assertions, swap
  DeserializeKLLSketchFromBytes (gob) → DeserializeKLLSketchFromProtoBytes
  (proto) in TestBatchModeTransmitSketch to match emit format.
- hllprocessor: fix 2 window-mode test assertions (selfmonitor
  was already resolvable).
- countsketchprocessor: add the `replace` directive only — tests
  already accounted for pass-through.
- countminsketchprocessor: add the `replace` directive, swap
  DeserializeCountMinSketchFromBytes (gob) →
  DeserializeCountMinSketchFromProtoBytes (proto) to match
  SerializeProtoBytesFO emit path.

All 5 packages pass `go test ./... -count=1` and `go build ./...`.
Full sketchcollector build via build_sketchcollector.sh succeeds.
gofmt is clean for all files modified by this change; pre-existing
gofmt debt in unrelated files left untouched.

Submodule pointer drift on opentelemetry-collector and
opentelemetry-go was left unstaged (pre-existing noise).

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