feat: Add tracing channels support - #193
Conversation
375b187 to
0ae9a84
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds Node.js tracing channels support to db0, enabling observability and performance monitoring for database query operations. The implementation follows an opt-in approach where users must explicitly wrap their database instance with withTracing() to enable tracing.
Key Changes:
- Introduces
withTracing()wrapper function that instruments database operations with Node.js diagnostic channels - Adds comprehensive test coverage for all traced database operations (exec, sql, prepare.all, prepare.run, prepare.get)
- Exports tracing functionality and types through the main index
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/tracing.ts | Implements the core tracing functionality with withTracing() wrapper and channel management for db query operations |
| test/tracing.test.ts | Provides comprehensive test coverage for tracing behavior including opt-in verification, success/error scenarios, and query reconstruction |
| src/index.ts | Exports the new tracing API (withTracing, TraceContext, TracedOperation) for public consumption |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@pi0 Friendly ping on this, it's the only remaining piece 😄 |
📝 WalkthroughWalkthroughAdds opt-in Node.js diagnostics channel tracing for database queries and prepared statements, exports and builds the tracing entrypoint, and adds validation, documentation, and a runnable SQLite example. ChangesQuery tracing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant withTracing
participant Database
participant diagnostics_channel
Application->>withTracing: wrap database
withTracing->>Database: intercept exec, sql, and prepare
Application->>Database: execute query
Database->>diagnostics_channel: publish db0.query context
diagnostics_channel-->>Application: start, asyncStart, asyncEnd, end, or error
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
examples/tracing/index.ts (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider marking the type-only import explicitly.
TraceContextis an interface used only as a generic type argument (line 24). Test filetest/tracing.test.tsalready usesimport typefor the same symbols fromsrc/tracing.ts; aligning here avoids relying on the transpiler's local-usage elision and future-proofs against stricterisolatedModules/verbatimModuleSyntaxsettings.-import { TraceContext, withTracing } from "../../src/tracing"; +import { withTracing } from "../../src/tracing"; +import type { TraceContext } from "../../src/tracing";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/tracing/index.ts` at line 2, Update the import in the tracing example to use a type-only import for TraceContext while keeping withTracing as a runtime import. Preserve the existing usage of both symbols.test/tracing.test.ts (1)
78-138: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winManual
listener.cleanup()calls won't run if an assertion throws, leaking subscribers on the shareddb0.querychannel.Every test creates a fresh listener and unsubscribes at the very end of the test body. If any
expect(...)earlier in the test throws,cleanup()is skipped and those handlers remain subscribed to the singleton channel for the rest of the suite run — accumulating stale listeners across (re)test runs and file watch reruns.♻️ Suggested fix: guarantee cleanup via try/finally or afterEach
- it("should not emit tracing events without withTracing wrapper", async () => { - const plainDb = createDatabase( - connector({ - name: ":memory:", - }), - ); - const listener = createTracingListener("query"); - - await plainDb.exec(`CREATE TABLE test (id INTEGER PRIMARY KEY)`); - await plainDb.sql`SELECT * FROM test`; - - // No tracing events should be emitted - expect(listener.handlers.start).not.toHaveBeenCalled(); - ... - - listener.cleanup(); - }); + it("should not emit tracing events without withTracing wrapper", async () => { + const plainDb = createDatabase( + connector({ + name: ":memory:", + }), + ); + const listener = createTracingListener("query"); + try { + await plainDb.exec(`CREATE TABLE test (id INTEGER PRIMARY KEY)`); + await plainDb.sql`SELECT * FROM test`; + + expect(listener.handlers.start).not.toHaveBeenCalled(); + ... + } finally { + listener.cleanup(); + } + });Applying this pattern (or wrapping
createTracingListenerto auto-register cleanup viaafterEach) across the file would prevent cross-test subscriber leakage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/tracing.test.ts` around lines 78 - 138, Ensure every listener created by createTracingListener is cleaned up even when an assertion or test operation throws. Apply a file-wide try/finally cleanup pattern or register listener cleanup through afterEach, including the listeners in the opt-in behavior tests, while preserving the existing tracing assertions.examples/tracing/package.json (1)
7-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
jitipinned to the legacy v1 line.Latest
jitiis 2.x;^1.21.0targets the unmaintained v1 branch. Not critical for example scaffolding, but consider bumping for a modern toolchain and consistency with other examples in the repo.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/tracing/package.json` around lines 7 - 10, Update the jiti entry in the examples package devDependencies from the legacy ^1.21.0 range to the current 2.x release line, matching the versioning used by other examples where applicable. Leave the db0 dependency unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/tracing/index.ts`:
- Around line 1-5: Update the imports in the tracing example to use the
published db0 entry points: import createDatabase from db0, TraceContext and
withTracing from db0/tracing, and sqlite from db0/connectors/better-sqlite3,
preserving the existing symbols and usage.
---
Nitpick comments:
In `@examples/tracing/index.ts`:
- Line 2: Update the import in the tracing example to use a type-only import for
TraceContext while keeping withTracing as a runtime import. Preserve the
existing usage of both symbols.
In `@examples/tracing/package.json`:
- Around line 7-10: Update the jiti entry in the examples package
devDependencies from the legacy ^1.21.0 range to the current 2.x release line,
matching the versioning used by other examples where applicable. Leave the db0
dependency unchanged.
In `@test/tracing.test.ts`:
- Around line 78-138: Ensure every listener created by createTracingListener is
cleaned up even when an assertion or test operation throws. Apply a file-wide
try/finally cleanup pattern or register listener cleanup through afterEach,
including the listeners in the opt-in behavior tests, while preserving the
existing tracing assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e441ed01-5f24-469a-bc26-ead13b588878
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
build.config.tsdocs/1.guide/1.index.mddocs/1.guide/2.tracing.mdexamples/tracing/index.tsexamples/tracing/package.jsonpackage.jsonsrc/tracing.tstest/tracing.test.ts
| import { createDatabase } from "../../src"; | ||
| import { TraceContext, withTracing } from "../../src/tracing"; | ||
| import { tracingChannel } from "node:diagnostics_channel"; | ||
|
|
||
| import sqlite from "../../src/connectors/better-sqlite3"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether other examples/* directories use relative src imports or the public db0 package.
fd -t d . examples | while read -r dir; do
echo "== $dir =="
fd -e ts -e js . "$dir" --exec grep -nE "from ['\"](\.\./\.\./src|db0)" {} \;
doneRepository: unjs/db0
Length of output: 530
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== examples files =="
fd -t f . examples
echo
echo "== package.json =="
sed -n '1,220p' package.json
echo
echo "== examples/drizzle/index.ts =="
sed -n '1,120p' examples/drizzle/index.ts
echo
echo "== examples/tracing/index.ts =="
sed -n '1,120p' examples/tracing/index.tsRepository: unjs/db0
Length of output: 5394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== examples/drizzle/package.json =="
sed -n '1,200p' examples/drizzle/package.json
echo
echo "== examples/tracing/package.json =="
sed -n '1,200p' examples/tracing/package.json
echo
echo "== tsconfig files =="
fd -a -t f 'tsconfig*.json' .Repository: unjs/db0
Length of output: 683
Use the published db0 entry points here
This example already declares db0 in examples/tracing/package.json, so it should import from db0, db0/tracing, and db0/connectors/better-sqlite3 instead of ../../src/*. That makes the example exercise the exported ./tracing subpath and match consumer usage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/tracing/index.ts` around lines 1 - 5, Update the imports in the
tracing example to use the published db0 entry points: import createDatabase
from db0, TraceContext and withTracing from db0/tracing, and sqlite from
db0/connectors/better-sqlite3, preserving the existing symbols and usage.
- wrap traced calls in async fns so connectors that throw synchronously emit the same event sequence (error/asyncStart/asyncEnd) as async ones - build the trace context lazily behind `hasSubscribers`, removing the double `sqlTemplate` parse per `db.sql` call - add `connector` to the trace context (the channel is process-global) - drop the stale `tracePromise` cast, @types/node returns the promise now - document `withTracing` and the `db0/tracing` entry point Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- make the trace wrapper async so subscribing never turns a rejection (e.g. an invalid sql template) into a synchronous throw - copy property descriptors instead of Object.create, so spreading or serializing a traced database keeps connector/dialect/disposed - mark traced instances with a non-enumerable symbol instead of `__traced` - correct the documented connector/dialect values and event ordering - cover param privacy, event ordering and subscriber cleanup in tests Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/tracing.test.ts (1)
175-180: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDispose
plainDbafter the assertion.Unlike the shared
db(disposed inafterEach), this locally created database is never disposed, leaving its sqlite connection open for the rest of the run.♻️ Proposed fix
- it("should not leak internal properties of the traced instance", () => { + it("should not leak internal properties of the traced instance", async () => { const plainDb = createDatabase(connector({ name: ":memory:" })); const tracedDb = withTracing(plainDb); expect(JSON.stringify(tracedDb)).toBe(JSON.stringify(plainDb)); + await plainDb.dispose(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/tracing.test.ts` around lines 175 - 180, Dispose the locally created plainDb in the “should not leak internal properties of the traced instance” test after the JSON assertion, using the database’s existing disposal API and ensuring cleanup occurs even if the assertion fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/tracing.test.ts`:
- Around line 175-180: Dispose the locally created plainDb in the “should not
leak internal properties of the traced instance” test after the JSON assertion,
using the database’s existing disposal API and ensuring cleanup occurs even if
the assertion fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93536c9a-322a-4ea9-932e-2f2f20760037
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
docs/1.guide/2.tracing.mdexamples/tracing/index.tssrc/tracing.tstest/tracing.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/tracing/index.ts
Adds Node.js tracing channels for db query operations, enabling observability and performance monitoring.
Channel Naming Convention
All channels use the
db0.queryconvention which when coupled with tracing channels produce the following diagnostic channels:tracing:db0.query:starttracing:db0.query:endtracing:db0.query:asyncStarttracing:db0.query:asyncEndtracing:db0.query:errorWhich matches nicely with
fastifybut there isn't a lot of convention for them other than the Node.js recommendation.Unlike
unstorageit didn't feel like we need multiple channels since more or less it boils down to executing a query.Tracing context includes
query- query to be executedmethod- source of the query:exec,sql,prepare.all,prepare.runorprepare.getconnector- the connector name (e.g.sqlite,postgresql)dialect- the db dialectBound parameters are never included in the context. The
querystring can still contain literal values (rawdb.exec()strings, or static${}interpolations inside{}in thesqltemplate).Example
Tracing is opt-in like
unstorage. Users create traced instances withwithTracingfromdb0/tracing:SDKs and consumers can then do:
All operations emit standard tracing events:
start,end,asyncStart,asyncEnd, anderror. Connectors that throw synchronously emit the same sequence as connectors that reject asynchronously, soasyncEndalways fires.Tracing requires
node:diagnostics_channel(Node.js20.16+/22.3+). On runtimes where it is unavailable,withTracingreturns the database unchanged. When nothing is subscribed to the channel, traced instances take a fast path and behave like untraced ones.Summary by CodeRabbit
New Features
./tracingpackage entry point.Documentation