fix: enable DB spans and metrics in telemetry#452
Conversation
…rumentation Closes #449 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactored OpenTelemetry and Greptime DB initialization: instrumentation registration moved from import-time to on-demand inside Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application (request)
participant Middleware as Greptime Middleware
participant OTEL as getOtelConfig()
participant BunSQL as Bun.SQL client
participant PgPkg as `@prisma/adapter-pg` (pg)
App->>Middleware: incoming request
Middleware->>BunSQL: call getGreptimeSqlClient()
BunSQL->>BunSQL: initialize client if not exists
Middleware->>OTEL: call getOtelConfig(serviceName)
OTEL->>OTEL: instantiate PgInstrumentation & BunSqlInstrumentation
OTEL->>PgPkg: dynamic require('@prisma/adapter-pg') (try)
PgPkg-->>OTEL: (if present) provide pg client
OTEL->>PgPkg: call PgInstrumentation._patchPgClient(pg) (errors ignored)
OTEL-->>Middleware: return instrumentation config
Middleware-->>App: attach greptimeDb and continue
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 52 minutes and 39 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/shared-api/lib/otel.ts (1)
90-100: Debug logging would aid troubleshooting when pg manual patching fails silently.The workaround is well-documented and addresses a real Bun limitation (GitHub issue
#3775). The version is already pinned to 0.66.0, which mitigates the risk of API breakage. However, the silentcatch {}block makes it hard to debug if the patching fails. Consider adding a debug log:try { // oxlint-disable no-unsafe-assignment no-unsafe-call no-unsafe-member-access const { createRequire } = require("module"); const pg = createRequire(require.resolve("@prisma/adapter-pg"))("pg"); // `@ts-expect-error` _patchPgClient is a private method on PgInstrumentation pgInstrumentation._patchPgClient(pg.Client); // oxlint-enable no-unsafe-assignment no-unsafe-call no-unsafe-member-access - } catch {} + } catch (err) { + if (!IS_PRODUCTION) console.debug("[otel] pg manual patching failed:", err); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-api/lib/otel.ts` around lines 90 - 100, The empty catch around the manual pg patching hides failures; update the try/catch in the otel patch block so the catch logs debug-level details including the caught error and context (e.g., that createRequire(require.resolve("@prisma/adapter-pg"))("pg") and pgInstrumentation._patchPgClient(pg.Client) failed), using the existing logger (or console.debug if none) so failures to patch pg.Client are visible for troubleshooting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/shared-api/lib/otel.ts`:
- Around line 90-100: The empty catch around the manual pg patching hides
failures; update the try/catch in the otel patch block so the catch logs
debug-level details including the caught error and context (e.g., that
createRequire(require.resolve("@prisma/adapter-pg"))("pg") and
pgInstrumentation._patchPgClient(pg.Client) failed), using the existing logger
(or console.debug if none) so failures to patch pg.Client are visible for
troubleshooting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e845411-5f16-4cc9-b3d8-046d88cd626e
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
packages/shared-api/lib/otel.ts
The bun:sql client was created at module-eval time, before BunSqlInstrumentation could wrap the SQL constructor. Make it lazy so the first access goes through the instrumented constructor. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
turisanapo
force-pushed
the
fix/otel-db-instrumentation
branch
from
26b0c42 to
ad05bd3
Compare
turisanapo
force-pushed
the
fix/otel-db-instrumentation
branch
from
20cc603 to
ca30fc1
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
turisanapo
force-pushed
the
fix/otel-db-instrumentation
branch
from
1dee971 to
1501084
Compare
turisanapo
force-pushed
the
fix/otel-db-instrumentation
branch
from
62caa11 to
a61932d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/shared-api/lib/otel.ts (1)
85-100: Empty catch block pattern is intentional for resilience—logging suggestion should align with structured logger pattern.The empty
catch {}at line 100 mirrors similar graceful fallbacks elsewhere in the codebase (e.g.,secret.ts,postgres.ts), confirming this is intentional. If added, observability logging should use the codebase's structuredlogger(already imported in this file and used consistently withlogger.info/error/warn) rather thanconsole.debug.Additionally,
_patchPgClientis a private API that may change between@opentelemetry/instrumentation-pgversions, though this is already mitigated by the@ts-expect-errorcomment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-api/lib/otel.ts` around lines 85 - 100, The empty catch swallowing errors when attempting to patch pg.Client should emit a structured log via the existing logger instead of doing nothing; update the try/catch around the createRequire + pgInstrumentation._patchPgClient call to catch the error (e.g., catch (err)) and call logger.debug or logger.warn with a short message like "failed to patch pg.Client for Bun: falling back" and include the caught error object, referencing the pgInstrumentation, createRequire(require.resolve("@prisma/adapter-pg"))("pg"), and _patchPgClient call so maintainers can locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/shared-api/lib/otel.ts`:
- Around line 85-100: The empty catch swallowing errors when attempting to patch
pg.Client should emit a structured log via the existing logger instead of doing
nothing; update the try/catch around the createRequire +
pgInstrumentation._patchPgClient call to catch the error (e.g., catch (err)) and
call logger.debug or logger.warn with a short message like "failed to patch
pg.Client for Bun: falling back" and include the caught error object,
referencing the pgInstrumentation,
createRequire(require.resolve("@prisma/adapter-pg"))("pg"), and _patchPgClient
call so maintainers can locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d858a7dd-35e9-4aaf-b6ca-9db395cbd51e
📒 Files selected for processing (3)
apps/api/src/middlewares/greptime.tspackages/shared-api/db/greptime.tspackages/shared-api/lib/otel.ts
| .as("scoped"); | ||
|
|
||
| export type GreptimeDb = BunSqlClient; | ||
| export type GreptimeDb = Bun.SQL; |
There was a problem hiding this comment.
We don't know this, its completely encapsulated by the shared-api layer. Please revert.
| url, | ||
| let _client: Bun.SQL; | ||
|
|
||
| /** Lazily created so BunSqlInstrumentation wraps `bun:sql` before the first `new SQL()` call. */ |
There was a problem hiding this comment.
The previous createBunSqlClient was lazy as well, what exactly is the change here?
| pgInstrumentation, | ||
| new BunSqlInstrumentation({ | ||
| requireParentSpan: true, | ||
| ignoreConnectionSpans: true, | ||
| // FUTURE: set to true to avoid leaking sensitive information | ||
| maskStatement: false, | ||
| }), |
There was a problem hiding this comment.
Align coding pattern across pg and bun instrumentation.
| // Pg is already loaded via @prisma/adapter-pg before OTel's module-load | ||
| // hooks can patch it. Patch pg.Client directly as a workaround, while still | ||
| // passing the instrumentation to NodeSDK so providers are configured correctly. | ||
| try { | ||
| // oxlint-disable no-unsafe-assignment no-unsafe-call no-unsafe-member-access | ||
| const { createRequire } = require("module"); | ||
| const pg = createRequire(require.resolve("@prisma/adapter-pg"))("pg"); | ||
| // @ts-expect-error _patchPgClient is a private method on PgInstrumentation | ||
| pgInstrumentation._patchPgClient(pg.Client); | ||
| // oxlint-enable no-unsafe-assignment no-unsafe-call no-unsafe-member-access | ||
| } catch {} |
There was a problem hiding this comment.
Looks like a big workaround / very hacky. Is this either (a) something that needs to be fixed upstream in @prisma/adapter-pg or otel-instrumentation-otel or (b) something where the initialisation needs to follow a similar lazy pattern like we do with our greptime client?
2 participants
Summary
Fixes #449 — DB operations were invisible in telemetry (no
db_*metric tables, no@opentelemetry/instrumentation-pgspans).Two root causes, both fixed in
packages/shared-api/lib/otel.ts:Missing metrics (NoopMeter):
registerInstrumentations()registered the DB instrumentations separately from the NodeSDK, so it never calledsetMeterProvider()on them — they kept a permanentNoopMeter. Fixed by creating them insidegetOtelConfig()and returning them via theinstrumentationsfield, so the NodeSDK owns them and sets a realMeterProvideraftersdk.start().Missing pg spans (RITM bypass): Bun resolves all static ESM imports before any module body runs (oven-sh/bun#3775), so the RITM hooks registered by
PgInstrumentationnever interceptpg(already loaded via@prisma/adapter-pg). Fixed by directly calling_patchPgClient(pg.Client)insidegetOtelConfig(), which runs after the full module graph is available.Test plan
bun run typecheck— 0 errors across all 8 workspaceshebo-auth, created users, queried GreptimeDB@opentelemetry/instrumentation-pg: 59 spans (SELECT, INSERT, UPDATE, BEGIN, COMMIT)@8monkey/opentelemetry-instrumentation-bun-sql: 2 spansdb_client_operation_duration_seconds_{bucket,count,sum}tables created with datahttp_server_request_duration_seconds_*) unaffected🤖 Generated with Claude Code