Skip to content

fix(build): unbreak sketchcollector OCB build against current sketchlib-go - #208

Merged
zzylol merged 1 commit into
mainfrom
fix/sketchcollector-build-api-drift
May 1, 2026
Merged

zzylol merged 1 commit into
mainfrom
fix/sketchcollector-build-api-drift

Conversation

@zzylol

@zzylol zzylol commented May 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Two root causes when running `build_sketchcollector.sh` against today's sketchlib-go — one tooling-glue (the actual blocker), one real API drift in two processors.

(1) sketchlib-go was resolving via the proxy, not the local checkout

The OCB step writes `cmd/sketchcollector/go.mod` with `sketchlib-go ... // indirect` in the require block. The build script's "if no replace, append one" check was `grep -q "sketchlib-go"`, which matches the require line, short-circuits the `if`, and never appends the replace. So `go build` resolved sketchlib-go via `sum.golang.org`, pinning to a months-stale published commit (`20260328`) that lacked the methods the processors call.

That's why `InsertValue`, `EstimateCardinality`, `SerializeMsgpack` etc. came back "undefined" even though they exist on `sketchlib-go@main`.

Fix in two places:

  • `builder-config.yaml` gets a `sketchlib-go` entry in `replaces:` so OCB writes the replace into the generated `go.mod` from the start. Path is `../../../../sketchlib-go` relative to `cmd/sketchcollector/go.mod`, which lands on the sibling repo.
  • `build_sketchcollector.sh` check now matches the replace line specifically (`^replace[[:space:]]+...`) — defensive backup.

(2) Real API drift — two processors

After (1), the build still fails on:

`ddsketchprocessor`: `DeserializeState` / `DeserializePortable` are gone

sketchlib-go #52 (`cbbe94c`) renamed the inbound constructors:

  • `DeserializeState(&state)` → `NewFromState(&state)`
  • `DeserializePortable(&env)` folded into `NewFromState(env.GetDdsketch())` (extract the oneof variant)

The bare-state fallback swaps the function name. The envelope path uses the generated `GetDdsketch()` accessor (returns nil safely if upstream sent a different variant — guarded with a clear error).

CMS / CountSketch merge processors: `ApplyDelta` signature changed

sketchlib-go's `ApplyDelta` for both CMS and CountSketch is now `(*X, *Delta)` (no return), not `(*X, []byte) error`. Callers must call `DeserializeDelta(payload)` first. Updated both merge processors to do that and surface decode failures with the same logger/zap pattern they used before.

(`DDSketch.ApplyDelta` still takes `[]byte` and returns error, but `ddsketchprocessor` doesn't actually call it today — its delta-encoding inbound path is stubbed pending the go-side delta encoder. No change there.)

Verification

`./build_sketchcollector.sh --skip-patches` now produces a clean binary:

```
$ ls -la opentelemetry-collector-contrib-patch/cmd/sketchcollector/sketchcollector
-rwxrwxr-x ... 494990016 ... sketchcollector

$ ./sketchcollector components | grep -E 'name: (HLL|KLL|ddsketch|countmin|countsketch|countminsketchmerge|countsketchmerge)'
- name: HLL
- name: KLL
- name: countmin
- name: countminsketchmerge
- name: countsketch
- name: countsketchmerge
- name: ddsketch
```

All seven sketch processors compiled in.

`Dockerfile.sketchcol`'s "build prereqs" comment block was tracking the just-in-time replace approach; updated to point at the sibling `../sketchlib-go` checkout (no manifest edit needed at build time anymore).

Test plan

  • `build_sketchcollector.sh --skip-patches` exits 0 against current `sketchlib-go@main`.
  • `sketchcollector components` lists all 7 sketch processors.
  • `docker build -f deploy/docker/Dockerfile.sketchcol -t asap/sketchcol:dev .` and re-run e2e — should now exercise the real merge processors and the patched ddsketch decode path. (The agent-side ddsketch→batch wiring issue I flagged in fix(gateway): unbreak boot post-#206 + simplify merge config #207's PR description is independent of this.)

🤖 Generated with Claude Code

…ib-go

Two root causes — one tooling-glue, one real API drift:

## (1) sketchlib-go was resolving via the proxy, not the local checkout

The OCB step generates `cmd/sketchcollector/go.mod` with
`sketchlib-go ... // indirect` in the require block. Our build
script's "if no replace, append one" check was

    if ! grep -q "sketchlib-go" "${SKETCHCOL_DIR}/go.mod"; then …

which matches the require line, short-circuits the if, and never
appends the replace. So `go build` resolved sketchlib-go via
`sum.golang.org`, pinning to a months-stale published commit
(20260328) that lacked the methods the processors call.

Fix in two places:
- `opentelemetry-collector-contrib-patch/cmd/sketchcollector/builder-config.yaml`
  gets a sketchlib-go entry in `replaces:` so OCB writes the
  replace into go.mod from the start (path is
  `../../../../sketchlib-go` relative to the generated
  `cmd/sketchcollector/go.mod`, lands on the sibling repo).
- `build_sketchcollector.sh`'s defensive check now matches the
  replace line specifically (`^replace[[:space:]]+...`) so a
  future stripped-down manifest still gets the safety net.

After this, sketchlib-go resolves from the local checkout and
`InsertValue`, `EstimateCardinality`, `SerializeMsgpack` (HLL),
`SerializeMsgpack` (CMS, CountSketch) all resolve. They existed
on `main` the whole time.

## (2) Two real API drifts in sketchlib-go that processors hadn't caught up to

### `ddsketchprocessor`: DeserializeState / DeserializePortable removed

sketchlib-go 0518599 + cbbe94c renamed the inbound constructors:
`DeserializeState` → `NewFromState`, and `DeserializePortable`
folded into `NewFromState(env.GetDdsketch())`. The bare-state
fallback path swaps the function name; the envelope path now
extracts the oneof variant via the generated `GetDdsketch()`
accessor (returns nil safely if the upstream sent a different
variant — guarded with a clear error).

### CMS/CountSketch merge processors: ApplyDelta sig changed

sketchlib-go's `ApplyDelta` for both CMS and CountSketch is now
`(*X, *Delta)` (no return), not `(*X, []byte) error`. Callers
must `DeserializeDelta(payload)` first. Updated both merge
processors to do exactly that and surface decode failures with
the same logger/error-zap pattern they used before.

(DDSketch's `ApplyDelta` still takes `[]byte` and returns error,
but the ddsketchprocessor doesn't currently call it — its
delta-encoding path is stubbed pending sketchlib-go's go-side
delta encoder. So no change there.)

## Verification

`./build_sketchcollector.sh --skip-patches` now produces
`opentelemetry-collector-contrib-patch/cmd/sketchcollector/sketchcollector`
(473 MiB, 0.141.0-dev) cleanly. `sketchcollector components` lists
all seven sketch components (HLL, KLL, ddsketch, countmin,
countsketch, countminsketchmerge, countsketchmerge).

`Dockerfile.sketchcol`'s "build prereqs" comment block updated to
match: the `sketchlib-go` replace is in the OCB manifest now, no
just-in-time edit needed; just check out the repo at the sibling
path and run the script.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zzylol
zzylol merged commit 9d1726c into main May 1, 2026
@zzylol
zzylol deleted the fix/sketchcollector-build-api-drift branch May 1, 2026 14:51
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