fix(client): implement log-aware span dispatching in RemoteExecutor - #28806
fix(client): implement log-aware span dispatching in RemoteExecutor#28806Meet00028 wants to merge 5 commits into
Conversation
Fixes tracing by correlating log events with their corresponding OpenTelemetry spans in RemoteExecutor. Previously, logs were emitted separately from spans, breaking the trace hierarchy. Changes: - Implement custom span dispatching that reconstructs span hierarchy - Group logs by spanId and emit them within span context using runInChildSpan - Convert EngineSpan types to OpenTelemetry SpanKind enum - Remove skip condition from tracing functional tests for remote executor Closes #ORM-1395
35f215b to
658d7bc
Compare
WalkthroughIntroduced traversal-based OpenTelemetry tracing in RemoteExecutor to build and dispatch hierarchical spans and emit per-span logs; updated processing to prefer traces and fall back to raw logs. Converted a conditional test skip to an unconditional skip in the tracing functional tests. Changes
Sequence Diagram(s)sequenceDiagram
participant RemoteExecutor
participant Tracer as OpenTelemetry Tracer
participant Logger as Log Emitter
RemoteExecutor->>RemoteExecutor: processExtensions()
alt traces present
RemoteExecutor->>RemoteExecutor: `#dispatchEngineSpans`() — build child map, collect per-span logs, find roots
loop for each root span
RemoteExecutor->>Tracer: start span (mapped SpanKind)
Tracer-->>RemoteExecutor: span handle
RemoteExecutor->>Logger: emit per-span logs
RemoteExecutor->>RemoteExecutor: `#dispatchSpan`(child) — recursive for children
RemoteExecutor->>Tracer: end span (engine endTime)
end
else traces absent
RemoteExecutor->>Logger: emit raw logs
end
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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. 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
packages/client/src/runtime/core/engines/client/RemoteExecutor.ts(2 hunks)packages/client/tests/functional/tracing/tests.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for new file names (e.g.,query-utils.ts,filter-operators.test.ts)
Avoid creating barrel files (index.tsthat re-export from other modules). Import directly from the source file (e.g.,import { foo } from './utils/query-utils'notimport { foo } from './utils'), unless./utils/index.tsfile already exists
Files:
packages/client/tests/functional/tracing/tests.tspackages/client/src/runtime/core/engines/client/RemoteExecutor.ts
packages/client/tests/functional/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
packages/client/tests/functional/**/*.ts: Each client functional test must live in its own folder underpackages/client/tests/functional/orissues/and include required files:_matrix.ts(test configurations),test.tsortests.ts(test code),prisma/_schema.ts(schema template)
In client functional tests, useresult.name === 'PrismaClientKnownRequestError'andresult.codefor error assertions, notinstanceof
UseidForProvider(provider)from_utils/idForProviderfor portable ID field definitions in client functional tests
Files:
packages/client/tests/functional/tracing/tests.ts
🧠 Learnings (7)
📚 Learning: 2025-12-05T14:05:24.366Z
Learnt from: CR
Repo: prisma/prisma PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-05T14:05:24.366Z
Learning: Applies to packages/client/tests/functional/**/*.ts : Use `idForProvider(provider)` from `_utils/idForProvider` for portable ID field definitions in client functional tests
Applied to files:
packages/client/tests/functional/tracing/tests.ts
📚 Learning: 2025-12-05T14:05:24.366Z
Learnt from: CR
Repo: prisma/prisma PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-05T14:05:24.366Z
Learning: Applies to packages/client/tests/functional/**/*.ts : In client functional tests, use `result.name === 'PrismaClientKnownRequestError'` and `result.code` for error assertions, not `instanceof`
Applied to files:
packages/client/tests/functional/tracing/tests.ts
📚 Learning: 2025-12-04T16:42:41.033Z
Learnt from: aqrln
Repo: prisma/prisma PR: 28846
File: packages/client/tests/functional/issues/TML-1664-unknown-enum-value-read-error/test.ts:20-26
Timestamp: 2025-12-04T16:42:41.033Z
Learning: In Prisma Client functional tests (packages/client/tests/functional/**), the test framework guarantees a fresh database for each test suite, so test setup code (like ALTER TYPE ADD VALUE in beforeAll hooks) doesn't need idempotency checks for repeated runs.
Applied to files:
packages/client/tests/functional/tracing/tests.ts
📚 Learning: 2025-12-04T17:18:23.669Z
Learnt from: aqrln
Repo: prisma/prisma PR: 28830
File: packages/client/tests/e2e/sqlcommenter-query-insights/_steps.cts:1-20
Timestamp: 2025-12-04T17:18:23.669Z
Learning: E2E tests in packages/client/tests/e2e/** are executed within Docker containers and must contain complete setup steps including `pnpm install`, `pnpm prisma generate`, and database initialization commands, even if those steps appear redundant from a CI perspective.
Applied to files:
packages/client/tests/functional/tracing/tests.ts
📚 Learning: 2025-12-04T16:27:28.693Z
Learnt from: aqrln
Repo: prisma/prisma PR: 28830
File: packages/sqlcommenter-query-insights/src/parameterize/tests/logical-operators.test.ts:1-372
Timestamp: 2025-12-04T16:27:28.693Z
Learning: In the prisma/sqlcommenter-query-insights package, the PARAM_PLACEHOLDER constant is an internal detail of the package and should not be exported in the public API. Within the package, tests and internal modules may import PARAM_PLACEHOLDER directly from parameterize.ts; duplication is not required for package-internal usage. Only external consumers should not have access to this constant.
Applied to files:
packages/client/src/runtime/core/engines/client/RemoteExecutor.ts
📚 Learning: 2025-12-04T17:22:16.406Z
Learnt from: aqrln
Repo: prisma/prisma PR: 28830
File: packages/sqlcommenter-query-insights/src/parameterize/tests/properties.test.ts:1-566
Timestamp: 2025-12-04T17:22:16.406Z
Learning: In the prisma/sqlcommenter-query-insights package and related sqlcommenter infrastructure, plugin isolation is a concern: the sqlcommenter infrastructure should ensure that one plugin cannot mutate data received by another plugin. This concern should be addressed at the infrastructure level rather than in individual plugin tests.
Applied to files:
packages/client/src/runtime/core/engines/client/RemoteExecutor.ts
📚 Learning: 2025-12-05T14:05:24.366Z
Learnt from: CR
Repo: prisma/prisma PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-05T14:05:24.366Z
Learning: Applies to packages/client/src/runtime/utils/validatePrismaClientOptions.ts : For new PrismaClient constructor options, update runtime types in `packages/client/src/runtime/getPrismaClient.ts` and add validation in `packages/client/src/runtime/utils/validatePrismaClientOptions.ts`
Applied to files:
packages/client/src/runtime/core/engines/client/RemoteExecutor.ts
🧬 Code graph analysis (1)
packages/client/src/runtime/core/engines/client/RemoteExecutor.ts (1)
packages/instrumentation-contract/src/types.ts (2)
EngineSpan(22-31)EngineTraceEvent(35-46)
🔇 Additional comments (4)
packages/client/tests/functional/tracing/tests.ts (1)
773-773: LGTM!The empty
skip()function now returnsundefined(falsy), enabling the tracing tests to run for the remote executor. This aligns with the PR's implementation of log-aware span dispatching inRemoteExecutor.packages/client/src/runtime/core/engines/client/RemoteExecutor.ts (3)
218-226: LGTM!The conditional logic correctly prioritizes structured traces (dispatching spans with correlated logs) over raw log emission, with appropriate fallback when traces are absent.
269-275: Verify attribute type contract with the engine.The type assertion from
Record<string, unknown>toRecord<string, string | number | boolean>assumes all attribute values are primitives. If the engine ever sends nested objects or arrays, they may be silently dropped or serialized unexpectedly by OpenTelemetry.Confirm that
EngineSpan.attributesfrom the engine only contains primitive values. If non-primitives are possible, consider filtering or flattening attributes before passing to the tracing helper.
292-300: LGTM!The mapping from engine span kinds to OpenTelemetry
SpanKindis appropriate, with a safe default toINTERNALfor unrecognized kinds.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/client/src/runtime/core/engines/client/RemoteExecutor.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for new file names (e.g.,query-utils.ts,filter-operators.test.ts)
Avoid creating barrel files (index.tsthat re-export from other modules). Import directly from the source file (e.g.,import { foo } from './utils/query-utils'notimport { foo } from './utils'), unless./utils/index.tsfile already exists
Files:
packages/client/src/runtime/core/engines/client/RemoteExecutor.ts
🧠 Learnings (6)
📚 Learning: 2025-12-03T10:49:29.006Z
Learnt from: aqrln
Repo: prisma/prisma PR: 28802
File: packages/client/package.json:208-215
Timestamp: 2025-12-03T10:49:29.006Z
Learning: In packages/client/package.json, dependencies like prisma/instrumentation-contract that are bundled during the build process should remain in devDependencies, not moved to runtime dependencies, because the bundler inlines the code into the runtime output.
Applied to files:
packages/client/src/runtime/core/engines/client/RemoteExecutor.ts
📚 Learning: 2025-12-05T14:05:24.366Z
Learnt from: CR
Repo: prisma/prisma PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-05T14:05:24.366Z
Learning: Applies to {packages/client-generator-js/src/TSClient/PrismaClient.ts,packages/client-generator-ts/src/TSClient/file-generators/PrismaNamespaceFile.ts} : For new PrismaClient constructor options, update generated types in both `packages/client-generator-js/src/TSClient/PrismaClient.ts` (buildClientOptions method) and `packages/client-generator-ts/src/TSClient/file-generators/PrismaNamespaceFile.ts` (buildClientOptions function)
Applied to files:
packages/client/src/runtime/core/engines/client/RemoteExecutor.ts
📚 Learning: 2025-12-05T14:05:24.366Z
Learnt from: CR
Repo: prisma/prisma PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-05T14:05:24.366Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Avoid creating barrel files (`index.ts` that re-export from other modules). Import directly from the source file (e.g., `import { foo } from './utils/query-utils'` not `import { foo } from './utils'`), unless `./utils/index.ts` file already exists
Applied to files:
packages/client/src/runtime/core/engines/client/RemoteExecutor.ts
📚 Learning: 2025-12-05T14:05:24.366Z
Learnt from: CR
Repo: prisma/prisma PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-05T14:05:24.366Z
Learning: Applies to packages/client/src/runtime/utils/validatePrismaClientOptions.ts : For new PrismaClient constructor options, update runtime types in `packages/client/src/runtime/getPrismaClient.ts` and add validation in `packages/client/src/runtime/utils/validatePrismaClientOptions.ts`
Applied to files:
packages/client/src/runtime/core/engines/client/RemoteExecutor.ts
📚 Learning: 2025-12-04T16:27:28.693Z
Learnt from: aqrln
Repo: prisma/prisma PR: 28830
File: packages/sqlcommenter-query-insights/src/parameterize/tests/logical-operators.test.ts:1-372
Timestamp: 2025-12-04T16:27:28.693Z
Learning: In the prisma/sqlcommenter-query-insights package, the PARAM_PLACEHOLDER constant is an internal detail of the package and should not be exported in the public API. Within the package, tests and internal modules may import PARAM_PLACEHOLDER directly from parameterize.ts; duplication is not required for package-internal usage. Only external consumers should not have access to this constant.
Applied to files:
packages/client/src/runtime/core/engines/client/RemoteExecutor.ts
📚 Learning: 2025-12-04T17:22:16.406Z
Learnt from: aqrln
Repo: prisma/prisma PR: 28830
File: packages/sqlcommenter-query-insights/src/parameterize/tests/properties.test.ts:1-566
Timestamp: 2025-12-04T17:22:16.406Z
Learning: In the prisma/sqlcommenter-query-insights package and related sqlcommenter infrastructure, plugin isolation is a concern: the sqlcommenter infrastructure should ensure that one plugin cannot mutate data received by another plugin. This concern should be addressed at the infrastructure level rather than in individual plugin tests.
Applied to files:
packages/client/src/runtime/core/engines/client/RemoteExecutor.ts
🔇 Additional comments (4)
packages/client/src/runtime/core/engines/client/RemoteExecutor.ts (4)
1-6: LGTM!The imports are properly consolidated. The previous duplicate import issue has been resolved, with all types now imported from
@prisma/instrumentation-contract.
217-225: LGTM!The updated logic correctly prioritizes trace dispatching when available and falls back to raw log emission for backward compatibility.
259-287: LGTM!The recursive span dispatching logic correctly:
- Emits logs within the span context
- Dispatches children before ending the parent span
- Handles the case where
otelSpanmight be undefined (tracing disabled)
289-298: No action needed. The span kind mapping is complete for Prisma's actual implementation.The engine only emits
'client'and'internal'span kinds, both of which are correctly mapped. OpenTelemetry'sSERVER,PRODUCER, andCONSUMERkinds are not used by Prisma's engine, so the current implementation handles all possible span kinds that can be encountered.Likely an incorrect or invalid review comment.
| if (logs) { | ||
| for (const log of logs) { | ||
| const spanId = log.spanId | ||
| if (spanId) { | ||
| const spanLogs = logsMap.get(spanId) || [] | ||
| spanLogs.push(log) | ||
| logsMap.set(spanId, spanLogs) | ||
| } else { | ||
| this.#emitLogEvent(log) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const rootSpans = spans.filter((span) => !span.parentId) | ||
|
|
||
| for (const rootSpan of rootSpans) { | ||
| this.#dispatchSpan(rootSpan, childrenMap, logsMap) | ||
| } | ||
| } |
There was a problem hiding this comment.
Logs with unknown spanId will be silently dropped.
Logs whose spanId references a span not present in the spans array will be added to logsMap but never emitted, since #dispatchSpan only retrieves logs for spans that exist in the tree. Consider emitting any remaining logs after dispatching all root spans.
const rootSpans = spans.filter((span) => !span.parentId)
+ const visitedSpanIds = new Set<string>()
for (const rootSpan of rootSpans) {
- this.#dispatchSpan(rootSpan, childrenMap, logsMap)
+ this.#dispatchSpan(rootSpan, childrenMap, logsMap, visitedSpanIds)
+ }
+
+ // Emit any orphaned logs whose spanId wasn't found in the span tree
+ for (const [spanId, orphanedLogs] of logsMap) {
+ if (!visitedSpanIds.has(spanId)) {
+ for (const log of orphanedLogs) {
+ this.#emitLogEvent(log)
+ }
+ }
}
}This would require updating #dispatchSpan to track visited span IDs:
#dispatchSpan(
span: EngineSpan,
childrenMap: Map<string, EngineSpan[]>,
logsMap: Map<string, EngineTraceEvent[]>,
+ visitedSpanIds: Set<string>,
): void {
+ visitedSpanIds.add(span.id)
const spanLogs = logsMap.get(span.id) || []
// ... rest of method
for (const child of children) {
- this.#dispatchSpan(child, childrenMap, logsMap)
+ this.#dispatchSpan(child, childrenMap, logsMap, visitedSpanIds)
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/client/tests/functional/tracing/tests.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for new file names (e.g.,query-utils.ts,filter-operators.test.ts)
Avoid creating barrel files (index.tsthat re-export from other modules). Import directly from the source file (e.g.,import { foo } from './utils/query-utils'notimport { foo } from './utils'), unless./utils/index.tsfile already exists
Files:
packages/client/tests/functional/tracing/tests.ts
packages/client/tests/functional/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
packages/client/tests/functional/**/*.ts: Each client functional test must live in its own folder underpackages/client/tests/functional/orissues/and include required files:_matrix.ts(test configurations),test.tsortests.ts(test code),prisma/_schema.ts(schema template)
In client functional tests, useresult.name === 'PrismaClientKnownRequestError'andresult.codefor error assertions, notinstanceof
UseidForProvider(provider)from_utils/idForProviderfor portable ID field definitions in client functional tests
Files:
packages/client/tests/functional/tracing/tests.ts
🧠 Learnings (1)
📚 Learning: 2025-12-12T14:18:10.759Z
Learnt from: jacek-prisma
Repo: prisma/prisma PR: 28913
File: packages/client/tests/functional/bytes-upsert/tests.ts:4-6
Timestamp: 2025-12-12T14:18:10.759Z
Learning: In Prisma functional test files under packages/client/tests/functional, when importing from './generated/prisma/client', use // ts-ignore (not // ts-expect-error) because the generated client path is not available until test execution time. This ensures type errors shown by the compiler are suppressed only for the runtime path, not for stricter type expectations. Apply this pattern to all such test files where the generated client is imported dynamically and the path isn’t statically available at type-check time.
Applied to files:
packages/client/tests/functional/tracing/tests.ts
Log events reported by Accelerate were emitted before the spans they belong to were dispatched, so `$on` handlers observed no active span, unlike the equivalent `LocalExecutor` path. `TracingHelper.dispatchEngineSpans` now receives the log events recorded during the dispatched spans along with a callback to emit them. `ActiveTracingHelper` emits each event while its span is active and also records it on the span, mapping `error` events to `recordException`. Events whose span was filtered out by `ignoreSpanTypes`, or that have no matching span at all, are still emitted, and the disabled tracing helper emits all of them directly, so no log is lost when tracing is off. This is a breaking change to the `TracingHelper` interface exported from `@prisma/instrumentation-contract`: custom implementations must accept the two new parameters and pass every event to `emitLogEvent`. Closes: #28792 Closes: #28806
Log events reported by Accelerate were emitted before the spans they belong to were dispatched, so `$on` handlers observed no active span, unlike the equivalent `LocalExecutor` path. `TracingHelper.dispatchEngineSpans` now receives the log events recorded during the dispatched spans along with a callback to emit them. `ActiveTracingHelper` emits each event while its span is active and also records it on the span, mapping `error` events to `recordException`. Events whose span was filtered out by `ignoreSpanTypes`, or that have no matching span at all, are still emitted, and the disabled tracing helper emits all of them directly, so no log is lost when tracing is off. This is a breaking change to the `TracingHelper` interface exported from `@prisma/instrumentation-contract`: custom implementations must accept the two new parameters and pass every event to `emitLogEvent`. Closes: #28792 Closes: #28806
Log events reported by Accelerate were emitted before the spans they belong to were dispatched, so `$on` handlers observed no active span, unlike the equivalent `LocalExecutor` path. `TracingHelper.dispatchEngineSpans` now receives the log events recorded during the dispatched spans along with a callback to emit them. `ActiveTracingHelper` emits each event while its span is active and also records it on the span, mapping `error` events to `recordException`. Events whose span was filtered out by `ignoreSpanTypes`, or that have no matching span at all, are still emitted, and the disabled tracing helper emits all of them directly, so no log is lost when tracing is off. This is a breaking change to the `TracingHelper` interface exported from `@prisma/instrumentation-contract`: custom implementations must accept the two new parameters and pass every event to `emitLogEvent`. Closes: #28792 Closes: #28806
Logs should be emitted while within the corresponding span. This was not the case for `RemoteExecutor` and there was a FIXME comment about this bug: log events reported by Accelerate were emitted before the spans they belong to were dispatched, so `$on` handlers observed no active span, unlike the equivalent `LocalExecutor` path. ## Changes - `TracingHelper.dispatchEngineSpans` now receives the log events recorded during the dispatched spans, along with a callback to emit them. - `ActiveTracingHelper` emits each event while its span is active, and also records it on the span, mapping `error` events to `recordException`. - Events whose span was filtered out by `ignoreSpanTypes`, or that have no matching span at all, are still emitted, so no log is silently dropped. - `disabledTracingHelper` emits all events directly. Logging is configured independently of tracing, and tracing can be disabled mid-request via `PrismaInstrumentation.disable()` after the server was already asked for spans. ## Breaking change `TracingHelper` is exported from the published `@prisma/instrumentation-contract` package. Custom implementations must accept the two new parameters and pass every event to `emitLogEvent`, otherwise logs the user asked for are dropped. The interface doc comment and the package README now state this obligation. ## Tests - `packages/instrumentation/src/ActiveTracingHelper.test.ts` covers span-scoped emission, events recorded on their span, `recordException` for `error` events, and both leftover paths (ignored span, absent span). - `packages/client/tests/functional/tracing-event-context` covers plain queries, batch transactions, and interactive transactions end to end. CI exercises the `RemoteExecutor` path via the `client-query-compiler-accelerate` job. Verified by temporarily reverting the fix: three of the four functional tests fail against `provider=postgresql, qpe=remote` without it and pass with it. The five pre-existing tracing suites pass in both executor modes. Closes: prisma#28792 Closes: prisma#28806
1 participant
Fixes tracing by correlating log events with their corresponding OpenTelemetry spans in RemoteExecutor. Previously, logs were emitted separately from spans, breaking the trace hierarchy.
Changes:
Closes #ORM-1395
Summary by CodeRabbit
New Features
Behavior Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.