fix: enable DB instrumentation metrics via Elysia OTel plugin#452
fix: enable DB instrumentation metrics via Elysia OTel plugin#452turisanapo wants to merge 18 commits into
Conversation
…rumentation Closes #449 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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 (2)
📝 WalkthroughWalkthroughThis PR refactors database initialization and OpenTelemetry instrumentation coordination. GREPTIME_HOST becomes a shared constant, OTel instrumentations move from import-time registration into getOtelConfig, the Prisma adapter switches to runtime instantiation, and Prisma client creation defers to first request via getPrisma() to ensure OTel registers before the database driver loads. ChangesOTel instrumentation coordination and eager-to-lazy database initialization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
🧹 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?
- pg: opaque require prevents Bun bundler from inlining @prisma/adapter-pg, so OTel RITM hooks intercept pg at runtime - bun:sql: lazy getGreptimeSqlClient() defers new SQL() to after BunSqlInstrumentation wraps the constructor (Proxy-based, per-instance) - otel: instrumentations passed to NodeSDK via getOtelConfig() so sdk.start() calls setMeterProvider(), fixing the NoopMeter issue Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/shared-api/lib/otel.ts (1)
95-96: 💤 Low valueConsider enabling
maskStatementbefore production traffic carries PII.The
FUTUREcomment acknowledges this, butmaskStatement: falsewill log raw SQL statements including any embedded literals (user IDs, emails, etc.) to your telemetry backend. If PII flows through these queries, enable masking or ensure Greptime access is appropriately restricted.🤖 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 `@packages/shared-api/lib/otel.ts` around lines 95 - 96, The telemetry configuration currently sets maskStatement: false which will log raw SQL (possibly containing PII); update the configuration in packages/shared-api/lib/otel.ts to enable masking (set maskStatement to true) or make it configurable via an environment flag (e.g., OTEL_MASK_STATEMENT) and ensure the code reads that flag to default to true in production; locate the maskStatement property in the DB/tracing config object in this file and change it to true or wire it to the env toggle accordingly.
🤖 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 `@packages/shared-api/lib/otel.ts`:
- Around line 95-96: The telemetry configuration currently sets maskStatement:
false which will log raw SQL (possibly containing PII); update the configuration
in packages/shared-api/lib/otel.ts to enable masking (set maskStatement to true)
or make it configurable via an environment flag (e.g., OTEL_MASK_STATEMENT) and
ensure the code reads that flag to default to true in production; locate the
maskStatement property in the DB/tracing config object in this file and change
it to true or wire it to the env toggle accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97c511ad-6639-409d-9c18-4fbc7d0fc528
📒 Files selected for processing (4)
apps/api/src/middlewares/greptime.tspackages/shared-api/db/greptime.tspackages/shared-api/db/postgres.tspackages/shared-api/lib/otel.ts
turisanapo
force-pushed
the
fix/otel-db-instrumentation
branch
2 times, most recently
from
6a4a950 to
8696ed3
Compare
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
turisanapo
changed the title
Deferring is unnecessary for metrics — the fix is solely the instrumentations field in getOtelConfig(). Reverts to eager client init. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@heiwen, the solution works in dev, and it correctly generates spans and metrics for all DB operations. However, the solution does not work when bundling with Do you have any suggestions on how to continue working on the fix? The changes include a simplification of how we work with |
…l.ts The only change needed for metrics is passing instrumentations via getOtelConfig() instead of registerInstrumentations(). No changes to the Greptime client or middleware are required. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ype annotations and improve clarity
2 participants
Summary
Fixes #449 — DB operations were invisible in telemetry (no
db_*metric tables, no@opentelemetry/instrumentation-pgspans).PgInstrumentationandBunSqlInstrumentationfrom a standaloneregisterInstrumentations()call into theinstrumentationsfield ofgetOtelConfig()@opentelemetry/instrumentation-pgpatches thepgdriver before the first connectionGREPTIME_HOSTresolution@prisma/adapter-pgimport to avoid loadingpgbefore OTel patches itKnown limitation
The fix works locally with
bun dev, but does not work when bundling withbun build.🤖 Generated with Claude Code
Summary by CodeRabbit