Conversation
zzylol
self-requested a review
December 10, 2025 21:52
zzylol
requested changes
Dec 11, 2025
Contributor
There was a problem hiding this comment.
Please replace this sender with telemetrygen in otel.
zzylol
added a commit
that referenced
this pull request
Apr 15, 2026
Refactors the TransmitSketch path from Gauge-with-byte-attribute emission to typed `CountMinSketchDataPoint` messages so ASAPQuery- backend's modified-OTLP sketch router (ASAPQuery-backend PRs #5-#9) actually sees them as sketch 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") ASAPQuery-backend's modified-OTLP decoder matches on `Metric.data = CountMinSketch{data_points: [...]}` (oneof tag 16, typed `CountMinSketchDataPoint` messages) and never looks inside anonymous Gauge attribute maps for sketch bytes. Even though the backend has a full CountMin decoder (PR #6), the whole end-to-end flow was broken for this processor because nothing produced the typed data points the decoder wanted. After this PR: metric.SetEmptyCountMinSketch() cmsMetric.SetAggregationTemporality(pmetric.AggregationTemporalityDelta) dp := cmsMetric.DataPoints().AppendEmpty() outputAttrs.CopyTo(dp.Attributes()) dp.SetSampleCount(...) dp.SetRows(...) dp.SetCols(...) dp.SetSketch(payload) dp.SetEncoding(pmetric.CountMinSketchEncodingProto | Delta) The backend's router now sees `Metric.data.CountMinSketch{...}` and routes it straight into `CountMinSketchAccumulator::from_sketchlib_proto_bytes`. This unblocks the ASAPQuery PR #11 / #12 feedback loop for CountMin queries — a capability miss now leads to a plan push which leads to the backend actually finding a precomputed match next time, because the sketch bytes are now flowing through the typed hot path. ## What changed ### `processor.go` Split the emission path on `p.cfg.TransmitSketch`: * **TransmitSketch = true** (the production path): emit a typed `CountMinSketchDataPoint` with `SetSketch` / `SetEncoding` / `SetSampleCount` / `SetRows` / `SetCols`. The internal encoding string ("proto_delta" / "proto_full") is mapped onto the proto enum (`CountMinSketchEncodingDelta` / `...Proto`). * **TransmitSketch = false** (monitoring-only path): keep the legacy Gauge emission so existing dashboards that read the processor's output as a scalar `sample_count` series keep working. Nothing in that mode carries sketch bytes anyway. `aggregationTemporality` is set to Delta because the processor always produces one data point per window, and every window represents the delta within that window (not a cumulative sketch). The backend's accumulator merges across windows itself. ### `processor_test.go` Introduced a tiny `cmsTestDataPoint` adapter so existing test assertions that read fields via `.Attributes().Get("sketch_payload")` / `"encoding"` / `"sample_count"` / `"rows"` / `"cols"` keep compiling without per-site rewrites. The helper `getAllDataPoints` now dispatches on `pmetric.MetricTypeCountMinSketch` for the typed path and synthesizes the legacy attribute keys from the typed fields (`Sketch()` / `Encoding()` / etc.). The Gauge path (`TransmitSketch = false`) still works via the same helper — the adapter carries `doubleValue` for the one test (`TestBatchModeQueryMetricsWhenTransmitSketchDisabled`) that reads `dps[0].DoubleValue()`. `encodingToLegacyString` maps the proto enum back to the "proto_full" / "proto_delta" strings that existing assertions compare against. Drive-by fix: two assertions in processor_test.go previously looked for `"cms.sketch_payload"` (with a `cms.` prefix) which had never matched what the processor actually wrote (`"sketch_payload"`, no prefix). Those assertions were silently always failing on any CI that exercised them. Fixed to `"sketch_payload"` to match the (legacy-compatible, now synthesized) attribute key. ## What's NOT in this PR * **MSGPACK encoding option**: the typed emission path is now ready to accept a config flag (next PR will add `encoding: msgpack` and call sketchlib-go's `SerializeMsgpack` from PR #51, setting `CountMinSketchEncodingMsgpack` from PR #157). One-line branch once this refactor lands. * **The other 3 processors** (`countsketchprocessor`, `hllprocessor`, `kllprocessor`) need the same refactor but each has its own attribute quirks and test suite. Will ship as 3 separate PRs to keep review size bounded. ## Validation Local `go build` was attempted but the worktree's processor module is missing a `replace` directive for `go.opentelemetry.io/collector/processor`, so `processor/selfmonitor` fails to resolve — a pre-existing build setup issue unrelated to this PR. `gofmt -l` is clean on both modified files. All my changes use already-existing pmetric API methods (`SetEmptyCountMinSketch`, `SetSketch`, `SetEncoding`, `SetSampleCount`, `SetRows`, `SetCols`) and existing enum constants (`CountMinSketchEncodingProto` / `Delta`), so compilation should be straightforward in CI's properly-set-up module graph. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Apr 15, 2026
Refactors the TransmitSketch path from Gauge-with-byte-attribute emission to typed `CountMinSketchDataPoint` messages so ASAPQuery- backend's modified-OTLP sketch router (ASAPQuery-backend PRs #5-#9) actually sees them as sketch 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") ASAPQuery-backend's modified-OTLP decoder matches on `Metric.data = CountMinSketch{data_points: [...]}` (oneof tag 16, typed `CountMinSketchDataPoint` messages) and never looks inside anonymous Gauge attribute maps for sketch bytes. Even though the backend has a full CountMin decoder (PR #6), the whole end-to-end flow was broken for this processor because nothing produced the typed data points the decoder wanted. After this PR: metric.SetEmptyCountMinSketch() cmsMetric.SetAggregationTemporality(pmetric.AggregationTemporalityDelta) dp := cmsMetric.DataPoints().AppendEmpty() outputAttrs.CopyTo(dp.Attributes()) dp.SetSampleCount(...) dp.SetRows(...) dp.SetCols(...) dp.SetSketch(payload) dp.SetEncoding(pmetric.CountMinSketchEncodingProto | Delta) The backend's router now sees `Metric.data.CountMinSketch{...}` and routes it straight into `CountMinSketchAccumulator::from_sketchlib_proto_bytes`. This unblocks the ASAPQuery PR #11 / #12 feedback loop for CountMin queries — a capability miss now leads to a plan push which leads to the backend actually finding a precomputed match next time, because the sketch bytes are now flowing through the typed hot path. ## What changed ### `processor.go` Split the emission path on `p.cfg.TransmitSketch`: * **TransmitSketch = true** (the production path): emit a typed `CountMinSketchDataPoint` with `SetSketch` / `SetEncoding` / `SetSampleCount` / `SetRows` / `SetCols`. The internal encoding string ("proto_delta" / "proto_full") is mapped onto the proto enum (`CountMinSketchEncodingDelta` / `...Proto`). * **TransmitSketch = false** (monitoring-only path): keep the legacy Gauge emission so existing dashboards that read the processor's output as a scalar `sample_count` series keep working. Nothing in that mode carries sketch bytes anyway. `aggregationTemporality` is set to Delta because the processor always produces one data point per window, and every window represents the delta within that window (not a cumulative sketch). The backend's accumulator merges across windows itself. ### `processor_test.go` Introduced a tiny `cmsTestDataPoint` adapter so existing test assertions that read fields via `.Attributes().Get("sketch_payload")` / `"encoding"` / `"sample_count"` / `"rows"` / `"cols"` keep compiling without per-site rewrites. The helper `getAllDataPoints` now dispatches on `pmetric.MetricTypeCountMinSketch` for the typed path and synthesizes the legacy attribute keys from the typed fields (`Sketch()` / `Encoding()` / etc.). The Gauge path (`TransmitSketch = false`) still works via the same helper — the adapter carries `doubleValue` for the one test (`TestBatchModeQueryMetricsWhenTransmitSketchDisabled`) that reads `dps[0].DoubleValue()`. `encodingToLegacyString` maps the proto enum back to the "proto_full" / "proto_delta" strings that existing assertions compare against. Drive-by fix: two assertions in processor_test.go previously looked for `"cms.sketch_payload"` (with a `cms.` prefix) which had never matched what the processor actually wrote (`"sketch_payload"`, no prefix). Those assertions were silently always failing on any CI that exercised them. Fixed to `"sketch_payload"` to match the (legacy-compatible, now synthesized) attribute key. ## What's NOT in this PR * **MSGPACK encoding option**: the typed emission path is now ready to accept a config flag (next PR will add `encoding: msgpack` and call sketchlib-go's `SerializeMsgpack` from PR #51, setting `CountMinSketchEncodingMsgpack` from PR #157). One-line branch once this refactor lands. * **The other 3 processors** (`countsketchprocessor`, `hllprocessor`, `kllprocessor`) need the same refactor but each has its own attribute quirks and test suite. Will ship as 3 separate PRs to keep review size bounded. ## Validation Local `go build` was attempted but the worktree's processor module is missing a `replace` directive for `go.opentelemetry.io/collector/processor`, so `processor/selfmonitor` fails to resolve — a pre-existing build setup issue unrelated to this PR. `gofmt -l` is clean on both modified files. All my changes use already-existing pmetric API methods (`SetEmptyCountMinSketch`, `SetSketch`, `SetEncoding`, `SetSampleCount`, `SetRows`, `SetCols`) and existing enum constants (`CountMinSketchEncodingProto` / `Delta`), so compilation should be straightforward in CI's properly-set-up module graph. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
6 tasks
zzylol
added a commit
that referenced
this pull request
May 5, 2026
…ries (#260) Adds datasets_eval/google_cluster/ — the real-workload evidence backing the five evaluation claims in docs/paper-outline.md. Without this dataset, every claim rests on synthetic data; this directory is the workload-credibility hook for paper blocker #6. Contents: - fetcher.py: streaming download + sha256-checksummed cache of documented head subsamples of the public Google cluster traces (2011 task_usage CSV.gz, 2019 instance_usage JSON-Lines.gz). Idempotent: cache hit short-circuits the download. - otlp_mapper.py: deterministic projection from trace rows to OTLP-shaped JSONL matching deploy/fake-exporter/'s {zone, rack, host, service, task} attribute schema. Cardinality cap N folds (machine, service, task) tuples onto an N-element hashed subset for the 1k/10k/100k sweep matrix; bias documented in the docstring. - queries.json: ten PromQL queries grouped by claim (quantile / topk / sum / count_unique), each with an expected_ground_truth_query for the accuracy reducer. - run.py: orchestrator with fetch/map/replay/validate subcommands. Replay defaults to dry-run; OTLP/gRPC sender is opt-in via --endpoint when opentelemetry-proto+grpcio are installed. - README.md: subset selection rationale, cardinality scaling, disk + wall-time budget, one-command smoke entrypoint. - tests/: 16 unit tests covering golden mapper output across both years + queries.json schema match against deploy/scripts/queries-e2e.json. Constraints respected: only datasets_eval/google_cluster/ touched; no changes to deploy/scripts/run_e2e_sweep.sh, deploy/scripts/measure-baseline.py, deploy/fake-exporter/, processor/, controller/, or ASAPQuery-backend/. Co-authored-by: Claude Opus 4.7 (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
2 participants
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.
Docs: https://docs.google.com/document/d/1u8vq2NIWEndMhNFTR4V1PDKHVPGmS3P1HIwyoMgNX7E/edit?tab=t.9s30kda6y66