feat(telemetry)!: adopt DuckDB 2 and upgrade dependencies - #272
Conversation
Preserve typed OTLP attributes and UTC nanoseconds in format-3 telemetry, use native extraction and timestamp pushdown, bound keyed dependency traversal, and verify Parquet with a reusable native engine. Reject prior formats without rewriting them and keep SQLite control state separate.
vishr
left a comment
vishr
left a comment
There was a problem hiding this comment.
Review: DuckDB 2 adoption and why the benchmark did not move
The DuckDB 2 integration is sound where it is used: timestamp predicates stay on native TIMESTAMPTZ_NS columns and prune, the views keep union_by_name/Hive inference off, VARIANT is shredded and the hot messaging projection reaches the scan, and async I/O is correctly left at its memory-governed defaults. The benchmark is flat because the engine is not the bottleneck. Details and inline findings below.
What I measured
I replayed the exact dashboard SQL and bound arguments the benchmark issues (dumped from each source tree) against the retained benchmark datasets, inside the same 4-CPU/6 GiB Docker VM, with no ingest running.
1. Every dashboard statement is fast in isolation. The slowest statement in either build is about 0.3 s (baseline endpoints query 288 ms; PR endpoints query 169 ms, recent-trace query 173 ms). Under the benchmark, /api/observability/performance had p50 8.1 s. The roughly 40x gap is queueing on a saturated CPU and the 4-connection pool, not statement speed.
2. The 20 reads/s gate is not reachable on this host with either engine. With 4 closed-loop workers, no ingest and all 4 CPUs, the full dashboard mix peaks at about 8.2 requests/s on the baseline and 9.5 requests/s on this PR (+16%). The mix costs about 1.8 CPU-seconds per five requests on the PR (/performance ~0.63, /trace ~0.73, /logs ~0.42). At 20 requests/s that is about 7 cores of demand on a 4-vCPU VM that also runs telemetrygen and ingest. All three builds completed about 7.4/s. In the first mixed phase Fanout used 3.3-3.7 cores, versus about 0.2 cores for write-only ingest at a similar rate, so reads are what saturate the box.
3. Engine-only comparison on identical data and SQL is roughly flat (baseline format-2 dataset and baseline SQL, threads=4, median of 5):
| Statement | DuckDB 1.5.5 | DuckDB 2 alpha |
|---|---|---|
/performance endpoints (rollup + raw boundary) |
288 ms | 225 ms |
/trace recent-trace selection |
224 ms | 244 ms |
/logs severity histogram |
80 ms | 78 ms |
/logs entries |
8 ms | 14 ms |
| Rollup-table reads (overview, topology, points) | ~1 ms | ~2-3 ms |
The PR's faster endpoints query on its own dataset (169 ms) comes mostly from the format-3 layout and a smaller dataset, not the engine. On a generic suite over the same data, DuckDB 2 is clearly faster only on some shapes: wide ORDER BY ... LIMIT 100 (297 ms to 18 ms), grouped quantiles (-36%), parent/child self-join (-22%) and windows (-11%). High-cardinality GROUP BY, COUNT(DISTINCT), sorting, string search and regexp are flat. The large DuckDB 2 wins (async I/O for remote Parquet, recursive CTE USING KEY, VARIANT shredding) are not on the dashboard hot path. Parquet is written by parquet-go, so DuckDB's writer changes do not apply either.
4. Per-request work scales with all retained data. The benchmark's 6h/12h/24h windows exceed the dataset's age (about 6 minutes), so every request scans about 5M rows. The inline comments on performance.go, trace.go and logs.go cover the three statements that account for nearly all read CPU.
Findings outside the diff
- Medium:
.github/workflows/site.yml:80will fail.go run ./cmd/fanout-docgen --checkruns withoutscripts/with-duckdb.sh, but docgen now linksinternal/duckdbthroughinternal/query. In a scratch copy it fails withinternal/duckdb/native.go:6:10: fatal error: 'duckdb.h' file not found. The justfile recipe was wrapped; this workflow step was not. - Low:
ensureLimit(internal/query/sql.go:387) is now dead.projectSQLResultsowns theLIMIT; only tests call it. - Context for the inline
performance.gocomment:rollupPublicationSafetyLag = 5 * time.Minute(internal/query/duck.go:136). At the end of the final run, the endpoint rollup watermark was 4.6 minutes behind the newest raw span.
Benchmark methodology
The current harness cannot show a read-path improvement on a 4-vCPU same-host VM: the read gate is infeasible for every build, and sweep-phase ingest volume (which differs per build) sets the dataset size for the sustained phase. A meaningful comparison needs generators on a separate host (or a read rate scaled to the cores), windows that match the dataset age, and at least three repetitions. Many of the reported "query errors" are 15-second client cancellations logged as server 500s; see the inline comment on mapQueryError.
|
Follow-up 9d6a7c4 fixes five inline correctness/build/error-classification findings, plus the site docgen workflow and dead SQL-limit helper from the review summary. The pre-commit lint job also now uses the pinned native wrapper. Added regression tests and reusable opt-in ingestion profiling. Local |
|
Implemented the nine-comment review follow-up in commit 3dd6f25 and replied individually to all nine inline comments.
Validation passes: report and reproduction, complete measured JSON, and harness patch. The full read-capacity and encoding findings remain explicitly open; they should not be closed by merging #272. CI at |
vishr
left a comment
vishr
left a comment
There was a problem hiding this comment.
Re-review at cd916d5: correctness is clean; the read caches do not keep up under sustained load
Verdict: I found no correctness bugs in file pruning, the three caches, or the writer. The performance claims hold only while the read cache is current. At the harness's sustained rate with dashboard reads, it is not current: the cache worker falls behind, reads fall back to raw scans, and latency climbs. The ingest "+19%" comes from the shorter admission window; CPU per row is still about 23-26% higher.
Correctness: verified
Tests ran in detached worktrees at cd916d5, natively on macOS ARM64 with the pinned engine.
- File pruning: pruned dashboard results equal full scans for 58 windows across 3 signals, with 0 mismatches. The windows include random spans, ±1 ns half-open edges, late publications and log/metric offsets.
parquetTimeRangematches real min/max for multi-row-group batches with ±48h outliers, compaction outputs, and late batches with old timestamps. Redaction survives thejson_serialize_sql/json_deserialize_sqlrewrite, and searching for secret values returns nothing. - Caches: a randomized cached-vs-raw parity test agrees for trace candidates, log histograms (with and without severity filters) and endpoints. It used 6 seeds, 25 windows each, and covered sub-minute and minute-aligned edges, empty services and namespaces, mixed-case statuses and severities, and traces split across cached and uncached batches. It agrees in three states: mixed, after
CompactParquetwith stale markers, and after refresh. There is no double counting. - Writer: the same batch written through 9d6a7c4 and cd916d5 reads back identically: 50,300 rows across the row-group boundary, with shared, pooled (more than 64) and unique resources, 2^53+1, bytes, nested values, mismatched types and nanosecond timestamps. Byte-array reuse is safe because parquet-go copies on write, and the shared-resource cache is per call and keyed by map identity.
-racepasses on telemetry, store and ingest.
Performance: reproduced cache backlog
Setup: a cd916d5 Linux ARM64 build in --cpus 4 --memory 6g, telemetrygen at the harness's selected sustained rate (2 processes × 8 workers × 2,500/s per signal, 120k rows/s offered), plus 20 reads/s rotating the harness's five requests, with fanout_read_cache_pending_batches scraped every 2 s:
| Interval | Pending batches | Read p50 / p95 | /performance p50 |
Shed reads |
|---|---|---|---|---|
| 0-45 s | 11-51 | 0.03-0.10 s / 0.13-0.23 s | 0.08-0.13 s | 0 |
| 45-60 s | 94 | 0.20 s / 0.71 s | 0.32 s | 0 |
| 60-75 s | 223-359 | 2.07 s / 7.49 s | 5.38 s | 67 |
| 75-120 s | 671-843 | 2.41-3.47 s / 8.5-9.4 s | 5.2-7.5 s | 92-135 per 15 s |
The read-cache worker spent 126 s of the run's ~150 s inside passes; one pass took about 9 s. It wrote 20.4M cache rows for 7.8M ingested rows. The query directory ended at 361 MB against 201 MB of telemetry. The same 120 s at the same rate without reads kept the backlog at 12-69 batches, so the cliff comes from the worker competing with reads for CPU and the write lock. Once behind, reads scan uncached files raw, which slows the worker further.
The retained fanout-review272-final run shows the same shape: /performance p50 rose from 168 ms to 1.6 s within the 15 s p2 sweep, and from 6 s to 15 s during the sustained phase. At shutdown, 402 of 845 active batches (32% of spans, 31% of logs) were uncached and 37 markers pointed at retired batches. That is why p2 qualifies in the sweep but fails when sustained.
Inline comments cover the mechanisms and the measurement claims. Suggested direction: maintain the trace index per batch rather than recomputing it globally; store per-minute aggregates and read only the two boundary minutes raw instead of copying every span and log row; derive a compaction output's contributions from its inputs; and have the harness scrape the backlog gauge with a sustained phase of at least five minutes.
When every cached file that overlaps a window lies inside it, the candidate set came only from index rows whose cached spans all start inside the window. A trace with another cached span in a file outside the window was dropped, although its in-window spans should rank it, so the trace view could return a different trace or none. Add those traces' in-window parts as candidates.
vishr
left a comment
vishr
left a comment
There was a problem hiding this comment.
Re-review at e818a8d: no open correctness issues; the cache backlog is fixed
Verdict: The incremental-cache rework in 3dd6f25 removes the backlog failure from the previous review. One correctness bug in it, trace-candidate selection, is fixed in e7dbc79, which I pushed to this branch; that fix was reviewed independently and is described below. I found no other correctness defects. CI is green, including Gate. What remains is performance and footprint, listed inline and below. None of it blocks correctness.
Correctness
- Trace candidate selection (fixed in e7dbc79). At 3dd6f25, when every cached file overlapping a window lay inside it, the candidates came only from index rows whose cached spans all started inside the window. A trace with another cached span in a file outside the window was dropped, so
/tracereturned another trace or none.- Reproduction: raw returned
T; cached returnedU. - Randomized time-local batches gave 17, 17 and 11 mismatches across three seeds.
- The fix adds those traces' in-window parts as candidates. Two regression tests are in
batch_reads_test.go; both failed before the change.
- Reproduction: raw returned
- Independent review of the fix, which I wrote, so I did not want to check it myself:
- Every comparison asserted at most one candidate row per trace. Removing the
partialfilter as a mutation trips that assertion, and restoring the old branch fails the scoped and randomized tests. - It covered scoped reads with out-of-window parts in other scopes, the changed-retired repair path, compaction and pruning before and after refresh, and partly uncached data.
- It ran 10 randomized seeds, 2,160 comparisons in all; all passed.
- Every comparison asserted at most one candidate row per trace. Removing the
- Endpoints and log histograms against the raw query (3dd6f25 review): zero mismatches across spread and time-local batches, two-level compaction, retention and late spans. Traces also matched on spread batches; the time-local trace mismatches were the bug above.
- Other 3dd6f25 changes: compaction transfer and crash recovery, the cache-version rebuild, the two write gates and checkpoint ordering, the 64-file admission limit, and snapshot isolation of the captured glob were reviewed with no defects found.
-racepasses oninternal/query,internal/observabilityandinternal/telemetry/store, and the pre-pushjust checkpassed.
Sustained load
I used the same driver for every build: the harness's selected sustained rate (2 processes × 8 workers × 2,500/s per signal, 120k rows/s offered) plus 20 reads/s, on 4 CPUs / 6 GiB, scraping fanout_read_cache_pending_batches every 2 s. Single runs:
| Build | Duration | Max pending | Read p50 / p95 / p99 | Shed |
|---|---|---|---|---|
| cd916d5 | 120 s | 843 | up to 3.5 s / 9.4 s / n/a | 386 |
| 3dd6f25 | 300 s | 50 | 0.08 s / 2.26 s / 4.42 s | 56 |
| e818a8d | 300 s | 42 | 0.06 s / 0.47 s / 0.94 s | 0 |
The backlog no longer grows. The remaining slowdowns repeat every 60 s and get longer as data accumulates. With 5 s buckets on 3dd6f25, read p50 was under 0.1 s, then about 1.1 s at 180-195 s and 0.7-2.3 s from 240 s, recovering to 0.07-0.09 s between ticks. That matches the service and edge rollup cadence (averages of about 4 s and 7 s per run in that test). Those rollups glob every file and filter on ingested_unix_nano with a MAX across all three signals, so each tick costs more as data grows. That code predates this PR and is #278. The difference between the two 300 s rows is within run-to-run variance; I don't attribute it to e7dbc79.
The opt-in read benchmark (FANOUT_READ_BENCH_LARGE=1, 3 alternating runs per build) shows no change from e7dbc79 beyond noise. Its windows end one second after an hour boundary, so a cached file straddles the edge and traces take the unchanged branch.
Not inline
- #277 stays open. The one-CPU ingest result confirms −22.5% throughput and +29.5% CPU per row.
- Pre-existing, low: a data directory containing
[or?fails at startup. DuckDB treats[…]as a glob class, andduckDSNputs the path into a DSN. It fails loudly and never silently drops rows. - Report: e818a8d corrects the 24h rows of the cache-current read table, which had repeated the 20m values. The JSON shows 24h cached reads 3.3-3.6x faster than raw.
vishr
left a comment
vishr
left a comment
There was a problem hiding this comment.
Whole-PR review against main at e818a8d
Earlier rounds reviewed each new commit's changes. This pass reviewed the cumulative PR against main in four areas, each by a reviewer that wrote and ran its own tests in a detached worktree:
- ingest and telemetry storage;
- query engine, SQL boundary, snapshots and locking;
- caches, observability services, API, MCP and agent;
- build, CI, dependencies, generated artifacts and docs.
I reproduced the most severe finding myself on macOS and Linux.
Verdict: one high-severity crash that affects the macOS binaries the release workflow ships; Linux is unaffected at reachable depths. Everything else is medium or low (inline). No other correctness defects were found.
Checked and found correct
- Ingest and storage: NaN/Inf, nil and empty values, and shared resource maps (never mutated after conversion). Event bounds per signal. Empty signals write no file. Format and quarantine safety: unsupported formats and operational errors never authorize quarantine. Compaction recovery through
PublishParquetReplacement. No file or handle leaks.-racepasses oninternal/ingest/...andinternal/telemetry/.... - SQL boundary (tested through
ExecuteSQLon a realNewDuck): DML inside CTEs and subqueries, file paths, system catalogs (duckdb_*,information_schema,pg_catalog,sqlite_master), privateread_*caches, table functions,getenv,glob, PIVOT and comment tricks are all rejected; row counts confirm nothing is mutated. The engine policy is applied before any caller. Nanosecond timestamps, 2^53+1, HUGEINT and NaN serialize correctly. - Locking: reader, cache-worker, compaction, rollup and maintenance lock orders form no wait cycle. The two writer connections touch disjoint tables, and
CHECKPOINTsucceeds with an open read transaction. - Dependency traversal: 1,600 randomized cases against a reference BFS, 0 mismatches for nodes, hops, ordering,
truncatedanddepth_limit_reached. Namespaces stay separate. Input bounds return 400. The route is covered bytelemetry:read, and the MCP tool name and docs match. - Build and supply chain: checksum pins for all four platforms; the release image builds and runs on ARM64 with every needed runtime library present (non-root, healthcheck).
gitleaksfinds nothing inmain..HEAD.trivy configshows only existing style warnings.osv-scannershows the PR fixes x/crypto and 23 site dev-dependency advisories; it introduces one, gRPC (inline).notices -checkandui-checkmatch fresh builds. The AG-UI 1.0.1 schemas validate the Go runtime's events. Benchmark report numbers match their JSON.
Sustained-load attribution (e818a8d, 300 s single runs, same driver as before)
| Variant | Read p50 / p95 / p99 | Shed | 60 s bumps |
|---|---|---|---|
| Default | 0.061 / 0.47 / 0.94 s | 0 | at 120, 180, 240 s (p50 up to 0.40 s) |
| Anomaly detector disabled | 0.051 / 0.45 / 0.87 s | 0 | still present (p50 up to 0.26 s), plus 0.4-0.55 s at 280-295 s |
FANOUT_ROLLUP_INTERVAL=1h |
0.065 / 1.56 / 5.30 s | 135 | none, but a steady climb to 3 s p50 after 255 s |
The 60 s bumps follow the service and edge rollup tick, not the detector. Compaction runs on its own 10 s cycle. Separately, average statement time climbs with retained data (41 → 172 ms between 210 s and 300 s in the rollup-off run, CPU about 3.5 cores, pool waits rising), so the 20 reads/s gate at this ingest rate is a CPU-capacity limit on 4 CPUs. Variance between single runs is large; this attribution is directional only.
Not inline
- Low, introduced: build docs are stale.
README.md:86andCONTRIBUTING.md:13still say "Go and a C compiler withCGO_ENABLED=1". A plaingo build ./cmd/fanoutnow fails withinternal/duckdb/native.go:6:10: fatal error: 'duckdb.h' file not found. The wrapper (scripts/with-duckdb.shorjust), the C++ runtime, first-build network access, and the native Linux/macOS AMD64/ARM64-only constraint are documented only in AGENTS.md. Editor tooling also needs the wrapper's CGO flags. - Low:
ExecuteSQLresult types. Bytes attributes now come back as escaped strings (\x00\x01\xFF) that cannot be told apart from string attributes; main returned base64. UUID, BLOB and DuckDB 2 GEOMETRY values come back as raw bytes cast to strings (sql.go:154, also on main).TIME_NSfails withunsupported data type: unknown type: index: 0from the 1.5-era driver.ExecuteSQLcurrently has only internal callers, so the impact is limited. - Low, pre-existing:
/logs(entries, then histogram) and/trace(candidate, then index read, then logs) run several statements, each with its own snapshot. A publication between them can briefly make the parts of one response disagree.
1 participant
What changed
Fanout now embeds DuckDB
v2.0.0-alpha43763and stores OTLP attributes as typed, shredded Parquet VARIANT values. Integers, booleans, bytes, nested values, and nanosecond UTC instants survive ingestion and compaction instead of being flattened into JSON strings.SELECT version().Breaking contracts
Telemetry batch format 3 and its physical schema are the only accepted format. Startup rejects unsupported metadata before cleanup or schema rewriting and preserves those files. There is no legacy reader, automatic conversion, route alias, or format fallback. Start with an empty telemetry directory and retain prior telemetry for its original reader. SQLite control state remains on modernc/database/sql, sqlc, and Goose.
SQL uses typed
attributesandresourcecolumns instead ofattributes_jsonandresource_json;attr()returns a typed VARIANT and callers cast scalars explicitly. Attribute and resource values accept at most 128 nested arrays/objects; deeper exports fail atomically withINVALID_ARGUMENT/ HTTP 400, without changing accepted value types.General VARIANT extraction across view projections remains a limitation of this pinned preview; the hot messaging projection has a verified scan-level workaround.
Verification
just checkjust test-race(full Go suite; existing documented checkptr workaround for the CGO driver)Additional local validation: native Linux ARM64 query/intelligence/observability and telemetry/storage tests; runtime engine version; typed-Parquet interoperability and compaction; SQL boundary restrictions; nanosecond/DST predicates and executed row-group pruning; executed shredded messaging projections; detector/log-helper queries against production Parquet; late-page corruption, span ordering/index parity, cancellation/OOM quarantine safety; generated docs (13 pages); UI tests, dependency audits, and site build. Native macOS ARM64 was tested locally; the new CI matrix also exercises AMD64.
Review follow-up
Closes #273, closes #274, closes #275, closes #276. #277 remains open for the typed writer's encoding CPU regression. #278 remains open for sustained dashboard-read capacity and peak process memory.
Current review report, complete measured JSON and identities, and reproducible harness patch. The historical report retains earlier measurements with corrected qualifications.
Whole-PR review fixes
All five inline findings from review 5386440469 are implemented in
3be627c82d6980d0ff87d16b683b4164991cd5c4:govulncheck; no reachable vulnerabilities are reported.README and CONTRIBUTING now document the wrapper, native platforms, C++ runtime, initial download, editor environment and nesting limit. The two non-inline follow-ups are explicitly tracked: #279 (SQL result encodings) and #280 (one snapshot per Logs/Trace response). They remain open. The encoding CPU and capacity follow-ups #277/#278 also remain open; these fixes make no new throughput or production capacity claim.
Validation: full
just check(also enforced on push), fulljust test-race, full native Linux ARM64 Go suite using a freshly populated mirror cache, native macOS ARM64 regressions, public mirror checksums andgovulncheck.Measurements and remaining limits
The following are retained measurements from the earlier cache follow-up, not a benchmark of
3be627c. The reviewer later observed zero shedding in one 300-second default run ate818a8db; single-run variance remains substantial, as described in their whole-PR review.Native Linux ARM64, 4 CPUs / 6 GiB, benchmarks run sequentially. A fixed 300.154-second mixed phase with nominal 120k offered rows/s + 20 reads/s accepts 66,086 rows/s. Pending-batch minute medians are 7 / 11 / 12 / 16 / 20.5, sampled maximum 63, last sustained sample 16; cooldown observation 0. The preceding failed iteration ended at 1,690 pending batches. This addresses cache accumulation on that workload.
The full read-capacity gate still fails: 16.4/20 reads/s, 19 failed, 1,099 shed, successful-client p95 5.96s. Peak RSS is 3.32 GiB, cooldown 0.98 GiB; lower peak process memory is not demonstrated. Generator exits, dropped rows, restarts and complete authoritative storage verification pass. These are failed-capacity diagnostics, not a publishable production capacity claim. Same-host generators are limited; offered targets are not actual ingestion rates. #278 tracks the remaining limits.
On an identical persisted fixture containing 1.2M spans + 1.2M logs, the derived catalog shrinks 180.9 → 70.0 MB (-61.3%), while Parquet remains 52.3 MB. The catalog still exceeds highly compressible Parquet. Fully built-cache broad endpoint/trace/log medians are 14.23 / 17.46 / 15.31ms, versus raw 46.62 / 56.71 / 55.71ms. Narrow endpoints retain planning overhead (11.53 → 17.49ms). Fully built-cache reads do not measure cache lag; ten requests per case do not establish production tails.
Synthetic 1,000-span append passes cost 16.85ms at 200k retained spans → 21.74ms at 8M, without a global rewrite. Broad captured-ID glob binding is flat with an explicit list in the 1,000-file fixture and remains slower than an unguarded glob; the snapshot contract is preserved.
Three alternating baseline/current durable-ingestion repetitions at one CPU / 6 GiB measure 414,700 → 321,598 rows/s (-22.5%), 2.405 → 3.115 CPU seconds/million rows (+29.5%), and 3,746 → 3,943 Go bytes/row (+5.3%). All six have zero export errors and pass storage verification. #277 remains open. The earlier four-CPU +19% fixture result was admission-limited; it is not evidence that VARIANT encoding is faster or that the regression is resolved.
Validation:
just check; fulljust test-racewith the documented CGO checkptr workaround; full native Linux ARM64 Go suite; semantic cache rebuild, exact cold/warm scopes and clipped windows, late publications, compaction/retention and two-level lineage, publication cancellation/protected output markers, independent analytical/cache transactions, row-budget/oversized-file progress, Unicode/relative filename identities, transactional rollback and public-SQL private-cache rejection. AGENTS.md and storage documentation describe the final design.