feat(countsketchprocessor): emit typed CountSketchDataPoint - #159
Merged
Merged
Conversation
Mirrors PR #158 for the CountSketch variant: replaces Gauge-with- byte-attribute emission with typed `CountSketchDataPoint` messages so ASAPQuery-backend's modified-OTLP sketch router sees them as `Metric.data = CountSketch{...}` variants instead of anonymous Gauges. ## Why Before this PR the processor emitted: metric.SetEmptyGauge() dp := gauge.DataPoints().AppendEmpty() dp.Attributes().PutEmptyBytes("sketch_payload").FromRaw(payload) dp.Attributes().PutStr("encoding", "proto_full") dp.Attributes().PutStr("partition_key", partitionKey) dp.Attributes().PutDouble("epsilon", p.config.Epsilon) dp.Attributes().PutDouble("delta", p.config.Delta) The ASAPQuery-backend's modified-OTLP decoder matches on `Metric.data = CountSketch{data_points: [...]}` (oneof tag 15, typed `CountSketchDataPoint` messages) and never looked at Gauge attribute maps. The decoder for CountSketch (ASAPQuery PR #15) was ready but nothing produced the typed data points it wanted. After this PR the same loop emits `CountSketchDataPoint` with `SetSketch` / `SetEncoding` / `SetDimension` / `SetEpsilon` / `SetDelta`, and the previously-orphaned backend decoder starts seeing real sketch bytes on the wire. ## What changed ### `processor.go` Split emission on `p.config.TransmitSketch`: * **TransmitSketch = true** (production): typed `CountSketchDataPoint`. The processor's `partition_key` string maps naturally onto `CountSketchDataPoint.Dimension` (both identify which sub-population the sketch covers). `Epsilon` and `Delta` get dedicated setters on the typed DP. `sample_count` and `window_duration_seconds` remain attribute-map entries — the typed DP has no setter for them, and they're observability-only (the backend doesn't use them for routing). `AggregationTemporality` set to `Delta` because each emission represents one window's delta. * **TransmitSketch = false**: keep the legacy Gauge emission so existing scalar-series dashboards continue to work. ### `delta_transmission_test.go` Same pattern as PR #158 — introduced a `csTestDataPoint` adapter and rewrote `getCSOutputDPs` to dispatch on `pmetric.MetricTypeCountSketch` (typed path) or `MetricTypeGauge` (legacy path) and synthesize the legacy attribute keys (`sketch_payload`, `encoding`, `partition_key`, `epsilon`, `delta`) from the typed fields, so every existing test assertion that reads via `dps[0].Attributes().Get(...)` keeps compiling without per-site rewrites. `csEncodingToLegacyString` maps `CountSketchEncodingProto` → "proto_full" / `CountSketchEncodingDelta` → "proto_delta" for the legacy string comparisons. ### `processor_test.go` Two tests (`TestGroupByPartitioning`, `TestWindowModeGroupBy`) previously walked `ms.At(k).Gauge().DataPoints()` directly to extract `partition_key` attributes from the output. Both now route through the `getCSOutputDPs` adapter from `delta_transmission_test.go` so they see the typed path transparently. Incidental gofmt drift on pre-existing struct literal indentation is included because `gofmt -w` fired on the whole file. None of it is functional. ## Validation Same pre-existing `go.opentelemetry.io/collector/processor/selfmonitor` resolution issue as PR #158 blocks local `go build` in this worktree layout — a go.mod replace directive gap that predates my changes. `gofmt -l` is clean on my three touched files. All API methods used (`SetEmptyCountSketch`, `SetSketch`, `SetEncoding`, `SetDimension`, `SetEpsilon`, `SetDelta`, `CountSketchEncodingProto` / `Delta`, `MetricTypeCountSketch`) are already exposed by pmetric — no new API needed. ## Follow-ups * `hllprocessor` — same refactor next, then `kllprocessor`. * MSGPACK encoding option per-processor once all four typed refactors land — one-line branch selecting sketchlib-go's `SerializeMsgpack` and `CountSketchEncodingMsgpack` (from PR #157). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Apr 15, 2026
… test `hllprocessor` was already emitting typed `HLLSketchDataPoint`s in both the batch (processor.go:289-334) and windowed (processor.go:553-637) paths — the same refactor PRs #158 and #159 just did for countmin and countsketch was already done for HLL. But two dead helper functions (`appendHLLSketchDataPoint`, `appendHLLDeltaDataPoint`) from an earlier iteration still lived in the file, and an integration test (`TestIntegrationTransmitSketch`) still asserted against the old `hll.sketch_payload` Gauge-attribute shape those dead helpers had once produced. The test has been silently failing to match anything on every CI run that exercised it, because the processor's live code path never calls those helpers. ## Changes ### `processor.go` Deleted `appendHLLSketchDataPoint` and `appendHLLDeltaDataPoint` (~40 lines of dead code). Both were unreachable from the live emission paths and contained the old attribute-bytes shape the ASAPQuery-backend modified-OTLP decoder doesn't consume. No behavior change — the live paths at lines 289-334 and 553-637 already emit `SetSketch()` / `SetEncoding()` / `SetCount()` / `SetCardinality()` / `SetPrecision()` on proper typed `HLLSketchDataPoint`s with `SetEmptyHLLSketch()`. ### `integration_test.go` `TestIntegrationTransmitSketch` updated to look for a typed `HLLSketch` metric with a non-empty `Sketch()` field and `HLLSketchEncodingProto` in the `Encoding()` field — the real wire shape the live processor code has been emitting all along. The old assertion scanned `pmetric.MetricTypeGauge` data points for an `hll.sketch_payload` byte attribute, which never existed because the helper that would have produced it was never called. The test was a silent no-op under any CI configuration that ran it, or failed under any configuration that inspected the assertion more carefully. Fixed to reflect reality. ## Why this is still useful hllprocessor not needing a functional refactor is the good news: the data plane for HLL has been correct since whoever last touched this file. The cleanup here removes dead code and fixes the formerly-silently-broken test so the CI assertion actually validates what the processor does. This unblocks the MSGPACK encoding option follow-up from confidently using HLL as a template for "what a correctly-emitting sketch processor looks like." ## Validation Same pre-existing `go.opentelemetry.io/collector/processor/selfmonitor` resolution issue as PRs #158 and #159 blocks local `go build`. gofmt is clean on both modified files. All API methods used (`pmetric.MetricTypeHLLSketch`, `HLLSketch()`, `DataPoints()`, `Sketch()`, `Encoding()`, `HLLSketchEncodingProto`) are already exposed by the pmetric patch. ## Follow-up * `kllprocessor` — the last of the four processors on the typed-DP refactor series. May or may not need changes (TBD; will inspect next PR). * MSGPACK encoding option per-processor once all four typed paths are confirmed. HLL is already positioned for a one-line branch: `switch p.cfg.Encoding { case "msgpack": dp.SetSketch(hll.SerializeMsgpack()); dp.SetEncoding(pmetric.HLLSketchEncodingMsgpack) }`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Apr 15, 2026
Last of the four processor typed-emission refactors (PR #158: countmin, PR #159: countsketch, PR #160: hll dead-code cleanup, this PR: kll). Replaces Gauge-with-byte-attribute emission with typed `KLLSketchDataPoint` messages so ASAPQuery-backend's modified-OTLP sketch router sees them as `Metric.data = KLLSketch{...}` variants instead of anonymous Gauges. ## Why Before this PR the `TransmitSketch` path called `appendKLLSketchDataPoint` which emitted: dp := metric.Gauge().DataPoints().AppendEmpty() dp.Attributes().PutInt("kll.k", int64(k)) dp.Attributes().PutInt("kll.count", int64(sketch.Count())) dp.Attributes().PutEmptyBytes("kll.sketch_payload").FromRaw(payload) dp.SetDoubleValue(float64(sketch.Count())) ASAPQuery-backend's modified-OTLP decoder matches on `Metric.data = KLLSketch{data_points: [...]}` (oneof tag 14, typed `KLLSketchDataPoint`). The Rust-side KLL decoder (ASAPQuery PR #8 §KLL, with lossy statistical reconstruction via item replay) was already wired up, but no processor produced the typed data points it wanted. The processor and the decoder were both ready for each other but speaking past each other on the wire. ## What changed ### `processor.go` * **Batch path** (around line 226) — `TransmitSketch` branch now calls `findOrCreateKLLSketchMetric` (creates a typed `pmetric.MetricTypeKLLSketch` metric with `AggregationTemporalityDelta`) and `appendTypedKLLSketchDataPoint` (writes `SetSketch` / `SetEncoding` / `SetCount` on a typed `KLLSketchDataPoint`). * **Windowed path** (around line 486) — same two helpers. The inline `m.SetEmptyGauge()` is replaced with `m.SetEmptyKLLSketch().SetAggregationTemporality(...)`. * New helper `findOrCreateKLLSketchMetric` mirrors the existing `findOrCreateGaugeMetric` but creates typed KLL metrics and matches on `MetricType` so legacy Gauge metrics with the same name don't get accidentally reused. * New helper `appendTypedKLLSketchDataPoint` replaces the old `appendKLLSketchDataPoint`. Writes the sketch payload via `SetSketch` and tags the encoding with `KLLSketchEncodingProto`. Still carries `kll.k` on the attribute map for operator visibility (the typed DP has no dedicated setter for it; the Rust side derives k from the serialized payload). Sum/Min/Max fields on the typed DP are left at zero because sketchlib-go's `KLLSketch` type doesn't track them — KLL is quantile-only and the proto fields are observability-only. * Old `appendKLLSketchDataPoint` function deleted. Old `findOrCreateGaugeMetric` retained because the non-TransmitSketch quantile-emission path (lines 233-246 and 503-540) still exports gauge-shaped scalar quantile series. ### `processor_test.go` `TestBatchMode_TransmitSketch` (lines 68-115) previously asserted on `m.Gauge().DataPoints().At(0)` and read the payload from the `kll.sketch_payload` byte attribute. Updated to: * Assert `m.Type() == pmetric.MetricTypeKLLSketch` * Read the payload from `outDP.Sketch()` directly * Assert `outDP.Encoding() == pmetric.KLLSketchEncodingProto` Other tests in the file (TestBatchModeNoStatePersistence and below) all target the quantile-emission path (`TransmitSketch: false`), which still uses Gauge — they're unaffected. ## Validation Same pre-existing `go.opentelemetry.io/collector/processor/selfmonitor` resolution issue as PRs #158/#159/#160 blocks local `go build`. `gofmt -l` clean on both modified files. All API methods used (`pmetric.MetricTypeKLLSketch`, `SetEmptyKLLSketch`, `KLLSketch().DataPoints().AppendEmpty()`, `SetSketch`, `SetEncoding`, `SetCount`, `KLLSketchEncodingProto`) already exist on the pmetric patch. ## Stack * PR #158 (merged) — countminsketchprocessor typed emission * PR #159 — countsketchprocessor typed emission * PR #160 — hllprocessor dead-code cleanup + broken-test fix * **This PR** — kllprocessor typed emission * Next — per-processor MSGPACK config option (one-line branch each, calling sketchlib-go `SerializeMsgpack` from PR #51 and setting `*SketchEncodingMsgpack` from PR #157). Note: for KLL specifically, sketchlib-go #50/#51 out-of-scope'd the msgpack wire format because sketchlib-go's KLL and sketch-core's KLL don't share a byte-level backend — so KLL producers should keep using `_ENCODING_PROTO`. The MSGPACK enum value in PR #157 is reserved but not usable for KLL today. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Apr 15, 2026
… test (#160) `hllprocessor` was already emitting typed `HLLSketchDataPoint`s in both the batch (processor.go:289-334) and windowed (processor.go:553-637) paths — the same refactor PRs #158 and #159 just did for countmin and countsketch was already done for HLL. But two dead helper functions (`appendHLLSketchDataPoint`, `appendHLLDeltaDataPoint`) from an earlier iteration still lived in the file, and an integration test (`TestIntegrationTransmitSketch`) still asserted against the old `hll.sketch_payload` Gauge-attribute shape those dead helpers had once produced. The test has been silently failing to match anything on every CI run that exercised it, because the processor's live code path never calls those helpers. ## Changes ### `processor.go` Deleted `appendHLLSketchDataPoint` and `appendHLLDeltaDataPoint` (~40 lines of dead code). Both were unreachable from the live emission paths and contained the old attribute-bytes shape the ASAPQuery-backend modified-OTLP decoder doesn't consume. No behavior change — the live paths at lines 289-334 and 553-637 already emit `SetSketch()` / `SetEncoding()` / `SetCount()` / `SetCardinality()` / `SetPrecision()` on proper typed `HLLSketchDataPoint`s with `SetEmptyHLLSketch()`. ### `integration_test.go` `TestIntegrationTransmitSketch` updated to look for a typed `HLLSketch` metric with a non-empty `Sketch()` field and `HLLSketchEncodingProto` in the `Encoding()` field — the real wire shape the live processor code has been emitting all along. The old assertion scanned `pmetric.MetricTypeGauge` data points for an `hll.sketch_payload` byte attribute, which never existed because the helper that would have produced it was never called. The test was a silent no-op under any CI configuration that ran it, or failed under any configuration that inspected the assertion more carefully. Fixed to reflect reality. ## Why this is still useful hllprocessor not needing a functional refactor is the good news: the data plane for HLL has been correct since whoever last touched this file. The cleanup here removes dead code and fixes the formerly-silently-broken test so the CI assertion actually validates what the processor does. This unblocks the MSGPACK encoding option follow-up from confidently using HLL as a template for "what a correctly-emitting sketch processor looks like." ## Validation Same pre-existing `go.opentelemetry.io/collector/processor/selfmonitor` resolution issue as PRs #158 and #159 blocks local `go build`. gofmt is clean on both modified files. All API methods used (`pmetric.MetricTypeHLLSketch`, `HLLSketch()`, `DataPoints()`, `Sketch()`, `Encoding()`, `HLLSketchEncodingProto`) are already exposed by the pmetric patch. ## Follow-up * `kllprocessor` — the last of the four processors on the typed-DP refactor series. May or may not need changes (TBD; will inspect next PR). * MSGPACK encoding option per-processor once all four typed paths are confirmed. HLL is already positioned for a one-line branch: `switch p.cfg.Encoding { case "msgpack": dp.SetSketch(hll.SerializeMsgpack()); dp.SetEncoding(pmetric.HLLSketchEncodingMsgpack) }`. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Apr 15, 2026
Last of the four processor typed-emission refactors (PR #158: countmin, PR #159: countsketch, PR #160: hll dead-code cleanup, this PR: kll). Replaces Gauge-with-byte-attribute emission with typed `KLLSketchDataPoint` messages so ASAPQuery-backend's modified-OTLP sketch router sees them as `Metric.data = KLLSketch{...}` variants instead of anonymous Gauges. ## Why Before this PR the `TransmitSketch` path called `appendKLLSketchDataPoint` which emitted: dp := metric.Gauge().DataPoints().AppendEmpty() dp.Attributes().PutInt("kll.k", int64(k)) dp.Attributes().PutInt("kll.count", int64(sketch.Count())) dp.Attributes().PutEmptyBytes("kll.sketch_payload").FromRaw(payload) dp.SetDoubleValue(float64(sketch.Count())) ASAPQuery-backend's modified-OTLP decoder matches on `Metric.data = KLLSketch{data_points: [...]}` (oneof tag 14, typed `KLLSketchDataPoint`). The Rust-side KLL decoder (ASAPQuery PR #8 §KLL, with lossy statistical reconstruction via item replay) was already wired up, but no processor produced the typed data points it wanted. The processor and the decoder were both ready for each other but speaking past each other on the wire. ## What changed ### `processor.go` * **Batch path** (around line 226) — `TransmitSketch` branch now calls `findOrCreateKLLSketchMetric` (creates a typed `pmetric.MetricTypeKLLSketch` metric with `AggregationTemporalityDelta`) and `appendTypedKLLSketchDataPoint` (writes `SetSketch` / `SetEncoding` / `SetCount` on a typed `KLLSketchDataPoint`). * **Windowed path** (around line 486) — same two helpers. The inline `m.SetEmptyGauge()` is replaced with `m.SetEmptyKLLSketch().SetAggregationTemporality(...)`. * New helper `findOrCreateKLLSketchMetric` mirrors the existing `findOrCreateGaugeMetric` but creates typed KLL metrics and matches on `MetricType` so legacy Gauge metrics with the same name don't get accidentally reused. * New helper `appendTypedKLLSketchDataPoint` replaces the old `appendKLLSketchDataPoint`. Writes the sketch payload via `SetSketch` and tags the encoding with `KLLSketchEncodingProto`. Still carries `kll.k` on the attribute map for operator visibility (the typed DP has no dedicated setter for it; the Rust side derives k from the serialized payload). Sum/Min/Max fields on the typed DP are left at zero because sketchlib-go's `KLLSketch` type doesn't track them — KLL is quantile-only and the proto fields are observability-only. * Old `appendKLLSketchDataPoint` function deleted. Old `findOrCreateGaugeMetric` retained because the non-TransmitSketch quantile-emission path (lines 233-246 and 503-540) still exports gauge-shaped scalar quantile series. ### `processor_test.go` `TestBatchMode_TransmitSketch` (lines 68-115) previously asserted on `m.Gauge().DataPoints().At(0)` and read the payload from the `kll.sketch_payload` byte attribute. Updated to: * Assert `m.Type() == pmetric.MetricTypeKLLSketch` * Read the payload from `outDP.Sketch()` directly * Assert `outDP.Encoding() == pmetric.KLLSketchEncodingProto` Other tests in the file (TestBatchModeNoStatePersistence and below) all target the quantile-emission path (`TransmitSketch: false`), which still uses Gauge — they're unaffected. ## Validation Same pre-existing `go.opentelemetry.io/collector/processor/selfmonitor` resolution issue as PRs #158/#159/#160 blocks local `go build`. `gofmt -l` clean on both modified files. All API methods used (`pmetric.MetricTypeKLLSketch`, `SetEmptyKLLSketch`, `KLLSketch().DataPoints().AppendEmpty()`, `SetSketch`, `SetEncoding`, `SetCount`, `KLLSketchEncodingProto`) already exist on the pmetric patch. ## Stack * PR #158 (merged) — countminsketchprocessor typed emission * PR #159 — countsketchprocessor typed emission * PR #160 — hllprocessor dead-code cleanup + broken-test fix * **This PR** — kllprocessor typed emission * Next — per-processor MSGPACK config option (one-line branch each, calling sketchlib-go `SerializeMsgpack` from PR #51 and setting `*SketchEncodingMsgpack` from PR #157). Note: for KLL specifically, sketchlib-go #50/#51 out-of-scope'd the msgpack wire format because sketchlib-go's KLL and sketch-core's KLL don't share a byte-level backend — so KLL producers should keep using `_ENCODING_PROTO`. The MSGPACK enum value in PR #157 is reserved but not usable for KLL today. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mirrors #158 for the CountSketch variant: replaces Gauge-with-byte-attribute emission with typed
CountSketchDataPointmessages so ASAPQuery-backend's modified-OTLP sketch router sees them asMetric.data = CountSketch{...}variants instead of anonymous Gauges.Why
Before this PR the processor emitted a Gauge with
sketch_payload/encoding/partition_key/epsilon/deltastuffed as attributes. The ASAPQuery-backend's modified-OTLP decoder (ASAPQuery #15 for CountSketch) was ready but nothing produced the typed data points it wanted. This PR closes that gap.What changed
processor.goSplit emission on
p.config.TransmitSketch:TransmitSketch = true(production): typedCountSketchDataPoint. The processor'spartition_keymaps naturally ontoCountSketchDataPoint.Dimension(both identify which sub-population the sketch covers).EpsilonandDeltaget dedicated setters.sample_countandwindow_duration_secondsremain attribute-map entries — the typed DP has no dedicated setter for them, and they're observability-only.AggregationTemporalityset toDelta.TransmitSketch = false: legacy Gauge emission preserved so existing scalar dashboards keep working.delta_transmission_test.goSame pattern as PR #158 — introduced a
csTestDataPointadapter and rewrotegetCSOutputDPsto dispatch onMetricTypeCountSketch(typed path) orMetricTypeGauge(legacy path) and synthesize the legacy attribute keys (sketch_payload,encoding,partition_key,epsilon,delta) from the typed fields. Every existing assertion that reads viadps[0].Attributes().Get(...)keeps compiling without per-site rewrites.csEncodingToLegacyStringmaps the proto enum back to\"proto_full\"/\"proto_delta\"for legacy string comparisons.processor_test.goTwo tests (
TestGroupByPartitioning,TestWindowModeGroupBy) previously walkedms.At(k).Gauge().DataPoints()directly to extractpartition_keyattributes. Both now route throughgetCSOutputDPsso they see the typed path transparently.Incidental gofmt drift on pre-existing struct literal indentation is included because
gofmt -wfired on the whole file. None of it is functional.Validation
Same pre-existing
go.opentelemetry.io/collector/processor/selfmonitorresolution issue as #158 blocks localgo buildin this worktree layout — a go.mod replace directive gap that predates my changes.gofmt -l: clean on my three touched filesSetEmptyCountSketch,SetSketch,SetEncoding,SetDimension,SetEpsilon,SetDelta,CountSketchEncodingProto/Delta,MetricTypeCountSketch) are already exposed by pmetricCI should compile cleanly in a properly-set-up module graph.
Stack
countminsketchprocessortyped emission (template for this PR)countsketchprocessortyped emissionhllprocessor, thenkllprocessor🤖 Generated with Claude Code