Skip to content

Add caller-side DuckDB CPU attribution metric#5642

Closed
realtonyyoung wants to merge 4 commits into
masterfrom
feat/duckdb-cpu-metric
Closed

Add caller-side DuckDB CPU attribution metric#5642
realtonyyoung wants to merge 4 commits into
masterfrom
feat/duckdb-cpu-metric

Conversation

@realtonyyoung

Copy link
Copy Markdown
Contributor

What

A new KurrentDB.DuckDB meter exposing kurrentdb.duckdb.cpu.seconds — a counter of CPU time consumed by DuckDB operations executed on KurrentDB threads, tagged activity=query|read|commit|checkpoint and source=caller. Today the process-level CPU metric is one opaque number; this is the first step toward decomposing it into "KurrentDB vs DuckDB" on all supported platforms, e.g.:

rate(kurrentdb_duckdb_cpu_seconds_total[1m]) / rate(kurrentdb_proc_cpu[1m])

How

  • ThreadCpuTime (KurrentDB.Core/DuckDB): cumulative CPU time of the calling thread — clock_gettime(CLOCK_THREAD_CPUTIME_ID) on Linux (clock id 3) and macOS (16), GetThreadTimes(GetCurrentThread()) on Windows. No-ops on unsupported platforms.
  • DuckDBCpuMetrics.Measure(activity) returns a ref struct scope, so the compiler rejects any use across an await — per-thread CPU deltas are only valid when start and stop happen on the same thread.
  • Instrumented synchronous DuckDB sections: default/user index commits, user index checkpoints, all four index readers (one wrap point in SecondaryIndexReaderBase), and QueryEngine's snapshot-capture/parse/plan/execute section.
  • The meter is registered in AddDuckDb() and listed in metricsconfig.json, so it exports through the standard /metrics endpoint with no extra wiring.

Verification

  • Two new unit tests: a busy-spin scope records CPU seconds with the right tags; an idle (sleeping) scope records ~nothing. Full SecondaryIndexing suite passes (75/75).
  • Live smoke test on a dev server (CommitBatchSize=1, one event written):
    kurrentdb_duckdb_cpu_seconds_total{otel_scope_name="KurrentDB.DuckDB",activity="commit",source="caller"} 0.0011465
    

Known limits

  • This measures the caller-executed share of DuckDB CPU. DuckDB also schedules work on its own native worker pool (and burns CPU during streaming chunk fetches inside ConsumeAsync); that share is invisible from the caller side and is planned as source=workers via Quack owning DuckDB's workers (threads=N, external_threads=N + duckdb_execute_tasks_state) — which also yields a CPU cap. Until then this metric is a lower bound, tightest for commit/checkpoint/read activities which are fully synchronous.
  • Windows accrues thread CPU at scheduler-quantum granularity: cumulative rates are statistically sound, but individual short operations under-resolve.

🤖 Generated with Claude Code

Adds a KurrentDB.DuckDB meter with a kurrentdb.duckdb.cpu.seconds
counter (tags: activity=query|read|commit|checkpoint, source=caller)
measuring CPU consumed by DuckDB operations executed on KurrentDB
threads, so node CPU can be decomposed into KurrentDB vs DuckDB.

ThreadCpuTime reads the calling thread's cumulative CPU time via
clock_gettime(CLOCK_THREAD_CPUTIME_ID) on Linux/macOS and
GetThreadTimes on Windows. Measurement scopes are ref structs so they
cannot span an await, which would invalidate per-thread CPU deltas.

Instrumented synchronous DuckDB sections: index commits, user index
checkpoints, the index readers, and QueryEngine's prepare/execute
section. CPU burned on DuckDB's internal worker threads and during
streaming chunk fetches is not visible from the caller side; that
share lands with the planned Quack external_threads work as
source=workers.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 12, 2026 16:58
@realtonyyoung realtonyyoung requested a review from a team as a code owner June 12, 2026 16:58
@qodo-code-review

Copy link
Copy Markdown
Contributor

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a new caller-side DuckDB CPU attribution metric (kurrentdb.duckdb.cpu.seconds) to help decompose overall process CPU usage into “KurrentDB vs DuckDB” CPU consumption, and wires it through the standard metrics pipeline.

Changes:

  • Added ThreadCpuTime + DuckDBCpuMetrics to measure per-thread CPU deltas and emit a counter tagged with activity and source=caller.
  • Instrumented synchronous DuckDB sections in query execution and secondary index read/commit/checkpoint paths.
  • Registered the new meter in metricsconfig.json and added unit tests validating “busy” vs “idle” behavior.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/KurrentDB/metricsconfig.json Enables exporting the new KurrentDB.DuckDB meter via the standard metrics endpoint.
src/KurrentDB.SecondaryIndexing/Query/QueryEngine.cs Measures caller-side CPU for the synchronous query setup/prepare phase.
src/KurrentDB.SecondaryIndexing/Indexes/User/UserIndexReader.cs Threads DuckDBCpuMetrics through user index reader construction.
src/KurrentDB.SecondaryIndexing/Indexes/User/UserIndexProcessor.cs Adds CPU attribution for commit/checkpoint work executed on caller threads.
src/KurrentDB.SecondaryIndexing/Indexes/User/UserIndexEngineSubscription.cs Passes DuckDBCpuMetrics through subscription wiring to processors/readers.
src/KurrentDB.SecondaryIndexing/Indexes/User/UserIndexEngine.cs Injects DuckDBCpuMetrics into the user index engine subscription graph.
src/KurrentDB.SecondaryIndexing/Indexes/SecondaryIndexReaderBase.cs Attributes CPU for synchronous DuckDB index-record lookups in read paths.
src/KurrentDB.SecondaryIndexing/Indexes/EventType/EventTypeIndexReader.cs Updates constructor to accept DuckDBCpuMetrics via base class changes.
src/KurrentDB.SecondaryIndexing/Indexes/Default/DefaultIndexReader.cs Updates constructor to accept DuckDBCpuMetrics via base class changes.
src/KurrentDB.SecondaryIndexing/Indexes/Default/DefaultIndexProcessor.cs Adds commit CPU attribution and injects DuckDBCpuMetrics.
src/KurrentDB.SecondaryIndexing/Indexes/Category/CategoryIndexReader.cs Updates constructor to accept DuckDBCpuMetrics via base class changes.
src/KurrentDB.SecondaryIndexing.Tests/Indexes/DefaultIndexReaderTests/IndexTestBase.cs Updates tests to construct readers/processors with DuckDBCpuMetrics.
src/KurrentDB.SecondaryIndexing.Tests/Indexes/DefaultIndexProcessorTests.cs Updates processor test setup for new DuckDBCpuMetrics dependency.
src/KurrentDB.SecondaryIndexing.Tests/Diagnostics/DuckDBCpuMetricsTests.cs Adds tests verifying CPU measurement/tagging for busy vs idle scopes.
src/KurrentDB.Core/DuckDB/ThreadCpuTime.cs Implements platform-specific per-thread CPU time reading.
src/KurrentDB.Core/DuckDB/InjectionExtensions.cs Registers DuckDBCpuMetrics as a singleton created from a dedicated meter.
src/KurrentDB.Core/DuckDB/DuckDBCpuMetrics.cs Defines the meter/counter and the ref-struct scope for caller-side CPU deltas.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/KurrentDB.Core/DuckDB/DuckDBCpuMetrics.cs

@realtonyyoung realtonyyoung left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One actionable issue I found:

The new query CPU metric stops before the result fetch loop, but every current Flight SQL consumer uses streaming and spends the actual DuckDB result production in IQueryResultReader.TryRead() after ExecuteAsync leaves the measurement scope. That means large/expensive streamed queries can report only snapshot/prepare/initial ExecuteQuery CPU while omitting the per-chunk TryFetch work, and QueryResultReader.Dispose() can also drain remaining chunks without attribution. Please thread DuckDBCpuMetrics into QueryResultReader and measure _result.TryFetch(...) in TryRead()/FinalizeEnumeration(), or otherwise keep the query metric scoped around those synchronous fetch calls too.

The query CPU scope in QueryEngine.ExecuteAsync covered only the
synchronous setup (snapshot capture, parse/plan, ExecuteQuery). In
streaming mode DuckDB produces result chunks later, inside
QueryResultReader.TryRead -> _result.TryFetch, called from
ConsumeAsync after that scope closes; FinalizeEnumeration (via
Dispose) could also drain chunks unattributed. Large streamed
queries therefore under-reported query CPU.

Thread DuckDBCpuMetrics into QueryResultReader and measure the
synchronous TryFetch calls in TryRead and FinalizeEnumeration under
the same query activity. Adds an integration test asserting the
streaming read path emits caller-attributed query CPU (tagging/wiring;
a per-fetch count assertion is avoided as it would be flaky under the
Windows GetThreadTimes quantum).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@realtonyyoung

Copy link
Copy Markdown
Contributor Author

Good catch — fixed in ae5a03e. You were right that the query scope stopped before the streaming fetch loop.

I threaded DuckDBCpuMetrics into QueryResultReader and now measure the synchronous _result.TryFetch(...) calls under the query activity in both TryRead() and FinalizeEnumeration() (the latter covers the drain in Dispose()). The setup scope in ExecuteAsync stays as-is, so prepare/plan and per-chunk fetch CPU are now both attributed additively to activity=query, source=caller.

Added an integration test (QueryEngineStreamingReadAttributesQueryCpuToCaller) that runs the streaming QueryEngine read and asserts the path emits caller-attributed query CPU (filtered against background commit activity on the shared meter). I deliberately assert tagging/wiring rather than a per-fetch measurement count: a single sub-vector-size chunk fetch can finish below the Windows GetThreadTimes quantum and record zero, so a count assertion would be flaky on the Windows runner. Local suite: 76/76 green.

Comment thread src/KurrentDB.SecondaryIndexing.Tests/IntegrationTests/ReadTests.cs Outdated
@realtonyyoung realtonyyoung force-pushed the feat/duckdb-cpu-metric branch from f7ca695 to ae5a03e Compare June 19, 2026 02:16
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jun 19, 2026

Copy link
Copy Markdown

Deploying eventstore with  Cloudflare Pages  Cloudflare Pages

Latest commit: e537152
Status: ✅  Deploy successful!
Preview URL: https://1491e025.eventstore.pages.dev
Branch Preview URL: https://feat-duckdb-cpu-metric.eventstore.pages.dev

View logs

@realtonyyoung

Copy link
Copy Markdown
Contributor Author

Heads-up: the two vulnerability checks fail on a repo-wide advisory, not this PR

ci/github/scan-vulnerabilities (and the build's NU1903 restore audit) fail on CVE-2025-6965 / GHSA-2m69-gcr7-jv3q, a high-severity advisory on SQLitePCLRaw.lib.e_sqlite3 2.1.11, pulled in transitively via Microsoft.Data.Sqlite. That package is the latest published version, is now deprecated, and has no patched release, so it can't be pinned forward. The advisory post-dates master's last green run (2026-06-12), so it breaks restore + the vuln scan for every PR repo-wide, independent of this change.

Per discussion, this is being handled in a dedicated repo-wide PR (suppress the audit + allowlist the scan for this advisory, or migrate to the recommended SourceGear.sqlite3), not bundled into this feature PR. I briefly added a NuGetAuditSuppress here and then reverted it — it only silenced the build gate, while dotnet list package --vulnerable in the scan job ignores that suppression, so it wasn't a complete fix and didn't belong here.

This PR's own code is green: build clean (0 warnings) and the full KurrentDB.SecondaryIndexing.Tests suite passes (76/76) locally, including the new streaming-CPU test. Once the SQLite advisory is remediated on master, I'll rebase and the vuln checks will clear.

Comment thread src/KurrentDB.Core/DuckDB/InjectionExtensions.cs Outdated
realtonyyoung and others added 2 commits June 18, 2026 22:37
…ally testable

Review fixes for PR #5642:

- AddDuckDb resolved the meter's service name via
  sp.GetService<MetricsConfiguration>(), which is not registered in the
  collection AddDuckDb runs from, so it always fell back to "kurrentdb"
  and mis-named the meter under legacy "eventstore" naming. Pass the
  configured ServiceName in from ClusterVNodeStartup instead, dropping
  the dead DI lookup.

- DuckDBCpuMetrics now takes an optional CPU-time source (defaulting to
  ThreadCpuTime), so tests can inject a deterministic clock. Replace the
  timing-dependent streaming-read integration assertion (which could
  flake when a small query's caller CPU rounded to zero ticks, notably
  under Windows' quantum-granular GetThreadTimes) with deterministic
  unit tests that verify the exact recorded delta, tags, and the
  zero-delta no-op gate.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts:
#	src/KurrentDB.Core/ClusterVNodeStartup.cs
realtonyyoung added a commit that referenced this pull request Jul 1, 2026
- #5642 is open (not merged): "shipped" -> "proposed", especially since
  the caller-side metric is being pulled per this spec.
- kurrentdb_proc_cpu is an ObservableUpDownCounter (gauge), so rate() is
  wrong; compare the DuckDB CPU-seconds rate against the gauge directly.
- "expected-standard" -> "expected to be standard".

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@realtonyyoung

Copy link
Copy Markdown
Contributor Author

Superseded by the dedicated-executor approach. The caller-side metric fundamentally could not capture DuckDB's parallel worker-thread CPU; it's replaced by the Kurrent.Quack DuckDBExecutor (kurrent-io/Kurrent.Quack#52) plus the KurrentDB integration, which now lives on #5655. Closing as superseded.

realtonyyoung added a commit that referenced this pull request Jul 3, 2026
Co-Authored-By: Claude Fable 5 <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.

2 participants