fix(client): fix incorrect log context in RemoteExecutor - #28892
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
Summary by CodeRabbit
WalkthroughThe tracing-helper contract now accepts engine spans, trace events, and an event callback. Runtime tracing dispatch associates events with OpenTelemetry spans, emits events for missing or ignored spans, and preserves direct log emission when tracing is disabled or spans are absent. New unit and functional tests cover event-to-span association, error events, Prisma query context, transactions, and provider matrices. Documentation and instrumentation development dependencies were updated accordingly. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
8e1b20a to
6664e0b
Compare
4545663 to
95b6642
Compare
6664e0b to
53ebee7
Compare
size-limit report 📦
|
95b6642 to
ad14ce9
Compare
53ebee7 to
1e17bd9
Compare
ad14ce9 to
4efda66
Compare
2d63c80 to
2d18fac
Compare
408c055 to
6703b38
Compare
2d18fac to
44d6b14
Compare
51de82c to
ea9e5bf
Compare
44d6b14 to
d934d15
Compare
d934d15 to
3198fda
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/instrumentation/src/ActiveTracingHelper.test.ts (1)
1-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest file name uses PascalCase, not kebab-case.
New file
ActiveTracingHelper.test.tsmirrors the source file it tests, but the stated convention requires kebab-case for new TS files (e.g.active-tracing-helper.test.ts).As per coding guidelines, "Use kebab-case for new TypeScript and TSX file names."
Test logic itself (span-context assertions, error/exception mapping, ignored/unmatched fallback coverage) is correct and thorough.
🤖 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/instrumentation/src/ActiveTracingHelper.test.ts` around lines 1 - 148, Rename the test file represented by ActiveTracingHelper.test.ts to kebab-case, active-tracing-helper.test.ts, while preserving its contents and ensuring the test import/reference to ActiveTracingHelper remains valid.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@packages/instrumentation/src/ActiveTracingHelper.test.ts`:
- Around line 1-148: Rename the test file represented by
ActiveTracingHelper.test.ts to kebab-case, active-tracing-helper.test.ts, while
preserving its contents and ensuring the test import/reference to
ActiveTracingHelper remains valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 957dfa4b-1c57-4aba-a7c8-75d04fe6e492
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
AGENTS.mdpackages/client/src/runtime/core/engines/client/RemoteExecutor.tspackages/client/src/runtime/core/tracing/TracingHelper.tspackages/client/tests/functional/tracing-event-context/_matrix.tspackages/client/tests/functional/tracing-event-context/prisma/_schema.tspackages/client/tests/functional/tracing-event-context/tests.tspackages/instrumentation-contract/README.mdpackages/instrumentation-contract/src/types.tspackages/instrumentation/package.jsonpackages/instrumentation/src/ActiveTracingHelper.test.tspackages/instrumentation/src/ActiveTracingHelper.ts
3198fda to
32b4e31
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/instrumentation/src/ActiveTracingHelper.ts`:
- Around line 66-87: Isolate caller-supplied log listener failures so tracing
and query processing continue. In
packages/instrumentation/src/ActiveTracingHelper.ts lines 66-87, wrap the
fallback-loop emitLogEvent call in try/catch so later leftover events are still
emitted; in lines 182-199, wrap dispatchEngineSpan’s emitLogEvent call so
span.addEvent, recordException, child dispatch, and span.end still execute.
Apply the same try/catch protection to emitLogEvent in
disabledTracingHelper.dispatchEngineSpans in
packages/client/src/runtime/core/tracing/TracingHelper.ts lines 22-34.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ea673da5-db75-4649-b297-1739bc273364
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
AGENTS.mdpackages/client/src/runtime/core/engines/client/RemoteExecutor.tspackages/client/src/runtime/core/tracing/TracingHelper.tspackages/client/tests/functional/tracing-event-context/_matrix.tspackages/client/tests/functional/tracing-event-context/prisma/_schema.tspackages/client/tests/functional/tracing-event-context/tests.tspackages/instrumentation-contract/README.mdpackages/instrumentation-contract/src/types.tspackages/instrumentation/package.jsonpackages/instrumentation/src/ActiveTracingHelper.test.tspackages/instrumentation/src/ActiveTracingHelper.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
32b4e31 to
cba357c
Compare
|
Addressed the review in cba357c.
Test file naming — declined. The convention in |
Merging this PR will degrade performance by 98.05%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | compile findMany filtered (uncached baseline) |
1.1 ms | 57.7 ms | -98.05% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing aqrln-vtlmsprroskt (cba357c) with main (a6d0155)
Footnotes
-
30 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
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
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@prisma/adapter-pg](https://github.com/prisma/prisma) ([source](https://github.com/prisma/prisma/tree/HEAD/packages/adapter-pg)) | imports | minor | [`7.9.1` -> `7.10.0`](https://renovatebot.com/diffs/npm/@prisma%2fadapter-pg/7.9.1/7.10.0) | | [@prisma/client](https://www.prisma.io) ([source](https://github.com/prisma/prisma/tree/HEAD/packages/client)) | imports | minor | [`7.9.1` -> `7.10.0`](https://renovatebot.com/diffs/npm/@prisma%2fclient/7.9.1/7.10.0) | --- ### Release Notes <details> <summary>prisma/prisma (@​prisma/adapter-pg)</summary> ### [`v7.10.0`](https://github.com/prisma/orm/releases/tag/7.10.0) [Compare Source](prisma/orm@7.9.1...7.10.0) ##### Prisma ORM 7.10.0 Prisma ORM 7.10.0 introduces a compatibility package for running Prisma 7 alongside newer Prisma versions, secures Prisma Studio's local server, and includes fixes across Prisma Client and the PostgreSQL, MariaDB, Neon, SQLite, and Prisma Postgres Serverless adapters. ##### Highlights ##### Run Prisma 7 alongside Prisma 8 This release introduces `@prisma/prisma7`, a compatibility package that lets you retain a matching Prisma 7 CLI and configuration while installing Prisma 8 in the same project. Once 7.10.0 is released, a side-by-side installation can use: ```sh npm install --save-dev prisma@8 @prisma/prisma7@7.10.0 npm install @prisma/client@7.10.0 ``` Use `prisma` for the directly installed Prisma 8 CLI and `prisma7` for Prisma 7: ```sh npx prisma --version npx prisma7 --version npx prisma7 generate npx prisma7 migrate dev npx prisma7 db push ``` Prisma 7 now prefers version-specific configuration files, allowing its configuration to coexist with Prisma 8's `prisma.config.*` files: ```ts // prisma7.config.ts import { defineConfig } from '@prisma/prisma7/config' export default defineConfig({ schema: 'prisma/schema.prisma', migrations: { path: 'prisma/migrations', }, }) ``` Without an explicit `--config` option, Prisma 7 searches for: 1. Root-level `prisma7.config.*` files. 2. `.config/prisma7.*` files. 3. Existing `prisma.config.*` files as a backwards-compatible fallback. The supported extensions are `.js`, `.ts`, `.mjs`, `.cjs`, `.mts`, and `.cts`. An explicit config path always takes precedence: ```sh npx prisma7 generate --config ./custom/prisma7.config.ts ``` New projects initialized by the Prisma 7 CLI use `prisma7.config.ts`. Existing projects containing only `prisma.config.*` continue to work without migration or additional warnings. If a `prisma7.config.*` file exists but cannot be loaded, Prisma reports the error rather than silently falling back to another configuration. The `prisma7` identity is carried through CLI help, version output, shell completion, initialization, migration, database, and generation guidance. Stable Prisma concepts such as `schema.prisma`, Prisma Migrate, `@prisma/client`, and `PRISMA_*` environment variables remain unchanged. Together, the separate executable and configuration namespace make it possible to operate Prisma 7 and Prisma 8 side by side without command or config-file collisions. [#​29949](prisma/orm#29949), [#​29969](prisma/orm#29969), [#​29994](prisma/orm#29994), [#​30000](prisma/orm#30000), [#​30002](prisma/orm#30002), [#​30020](prisma/orm#30020) ##### Prisma Studio security hardening Prisma Studio's local HTTP server now: - Binds explicitly to `127.0.0.1` instead of all network interfaces. - Rejects browser requests from origins other than the active `localhost` or `127.0.0.1` Studio URL. - No longer returns wildcard CORS headers. - Applies the same protections across Node.js, Bun, and Deno. This prevents network clients or malicious websites from accessing Studio's database endpoints while Studio is running. [#​29890](prisma/orm#29890) ##### Prisma Client - Fixed `P2002` errors from nested writes so `meta.modelName` identifies the model where the unique constraint violation occurred, including models using `@@map` and `@@schema`. [#​29628](prisma/orm#29628) - Fixed automatically batched `findUniqueOrThrow()` calls so every missing record rejects with `P2025`; later misses no longer resolve to `undefined`. [#​29654](prisma/orm#29654) - Parameter-chunked statements are now executed atomically in a transaction and rolled back if a later chunk fails. [#​29771](prisma/orm#29771) - Improved interactive transaction cleanup during `$disconnect()`, including transactions whose driver-level startup is still in progress. [#​28768](prisma/orm#28768) - Prevented transaction cleanup failures after a timeout or backend termination from becoming unhandled promise rejections. [#​29611](prisma/orm#29611) - Fixed fluent relation queries when relation fields are literally named `select` or `include`. [#​29683](prisma/orm#29683) - Fixed handling of `Date` and `Uint8Array` values created in other JavaScript realms, such as iframes, jsdom, and Node.js `vm` contexts. [#​29177](prisma/orm#29177) - Invalid `Date` values passed to `$queryRaw` or `$executeRaw` now throw `PrismaClientValidationError` instead of a generic error. [#​29718](prisma/orm#29718) - Fixed `moduleFormat` inference for the `prisma-client` generator in TypeScript projects using `module: "node16"` or `"nodenext"`. Generated output now follows the nearest `package.json` `type`, defaulting to CommonJS when absent. [#​29712](prisma/orm#29712) - Deserialized `Bytes` values now own standalone `ArrayBuffer`s rather than exposing unrelated contents from Node.js's shared `Buffer` pool. This applies to both regular and raw query results. [#​29701](prisma/orm#29701) - Fixed an incorrect logging context in the remote executor, including Accelerate-backed query execution. [#​28892](prisma/orm#28892) ##### Client extensions and observability - Result-extension `compute` callbacks now receive the current model name as a typed second argument: ```ts compute(data, modelName) { // ... } ``` The model name is also preserved when multiple extensions compose the same computed field. [#​29782](prisma/orm#29782) - Improved OpenTelemetry context for remotely executed queries: - `$on('query')` callbacks run within the matching `db_query` span. - Events from one operation share the same trace. - Error events are recorded as span exceptions. - Log events continue to be emitted when tracing is disabled or their reported span is unavailable. [#​28892](prisma/orm#28892) ##### Driver adapters ##### MariaDB - `@prisma/adapter-mariadb` now accepts an existing `mariadb` pool. External pools remain caller-owned unless `disposeExternalPool: true` is supplied. [#​27992](prisma/orm#27992) - Fixed pooled connection leaks during commit, rollback, and failed transaction startup. Connections are now returned with `release()` and transaction-specific listeners are removed before reuse. [#​29612](prisma/orm#29612) - Added support for bracketed IPv6 addresses in both `mysql://` and `mariadb://` connection strings. [#​29026](prisma/orm#29026) - Prevented malformed connection strings from exposing embedded passwords in retained debug output and diagnostic reports. [#​27992](prisma/orm#27992) ##### PostgreSQL, Neon, and Prisma Postgres Serverless - PostgreSQL deadlocks using SQLSTATE `40P01` are now reported as `P2034` transaction write conflicts. [#​29717](prisma/orm#29717) - PostgreSQL `RESTRICT` violations using SQLSTATE `23001` are now reported as `P2003`, preserving an available field or constraint name. [#​29554](prisma/orm#29554) - `@prisma/adapter-pg` now preserves database constraint names when reporting unique constraint violations through `P2002`. [#​29587](prisma/orm#29587) - Prisma Postgres Serverless now prefers the named constraint for `P2002`, falling back to parsed field names when no constraint name is available. [#​29801](prisma/orm#29801) - Fixed Neon HTTP adapter serialization for typed parameters such as `Bytes` and `DateTime`. [#​29747](prisma/orm#29747) ##### SQLite - `@prisma/adapter-better-sqlite3` now converts previously unhandled SQLite result codes into typed database errors instead of exposing raw driver errors. - The complete `SQLITE_BUSY` family is now mapped to socket timeout errors, with numeric extended result codes preserved where available. [#​29794](prisma/orm#29794) ##### CLI and Migrate - `prisma generate` can now offer to install Prisma's agent skills. The opt-in prompt: - Is shown at most once per machine. - Is skipped in CI, containers, Git hooks, npm lifecycle scripts, and watch mode. - Is skipped when `--no-hints` is used or Prisma skills are already installed. - Times out after 30 seconds. - Never causes generation to fail if installation is unsuccessful. [#​29690](prisma/orm#29690) - A globally installed CLI now warns during `prisma generate` when its version differs from the project's local `prisma` or `@prisma/client`, and recommends running the local CLI. The check is best-effort and does not fail generation. [#​29593](prisma/orm#29593) - `prisma version` and `prisma version --json` now include the resolved Prisma CLI package path, making global-versus-local installation issues easier to diagnose. [#​29573](prisma/orm#29573) - Empty or generator-only schema files now report `Schema must contain a datasource block` from `db pull`, `db push`, and `migrate dev`, rather than reaching the schema engine and potentially producing inconsistent errors. [#​29657](prisma/orm#29657) - CLI commands now tolerate corrupt, unreadable, or unwritable command-state files. Invalid state is reinitialized, writes are atomic, and persistence failures fall back to in-memory state. [#​29609](prisma/orm#29609) - Studio now recognizes semicolon-delimited `sqlserver://` connection strings before reporting the existing explicit message that SQL Server is not supported by Studio. [#​29623](prisma/orm#29623) - The AI-agent safety checkpoint now also covers interactive `prisma db push` confirmations involving data-loss warnings, rather than only invocations using `--accept-data-loss`. [#​29793](prisma/orm#29793) ##### Performance and reliability - Optimized query-plan execution by eagerly evaluating plans with one unconditional database operation and synchronously interpreting the remaining pure plan. Cached plans remain immutable. [#​29004](prisma/orm#29004) - Prevented call-stack overflows when rendering very large parameter lists or combining chunked results containing hundreds of thousands of rows. [#​29751](prisma/orm#29751) - Reduced ordinary query setup overhead by constructing fluent-relation field maps lazily and in linear time. Non-fluent queries no longer build this map. [#​29752](prisma/orm#29752) ##### Dependencies - Updated the transitive `fast-uri` dependency to a patched release addressing production audit advisories affecting versions through `3.1.3`. [#​29758](prisma/orm#29758) </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate CLI](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0NC4zMC4zIiwidXBkYXRlZEluVmVyIjoiNDQuMzAuMyIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==--> Reviewed-on: https://git.oirnoir.dev/OIRNOIR/YouTube-Helper-Server/pulls/41
Logs should be emitted while within the corresponding span. This was not the case for
RemoteExecutorand there was a FIXME comment about this bug: log events reported by Accelerate were emitted before the spans they belong to were dispatched, so$onhandlers observed no active span, unlike the equivalentLocalExecutorpath.Changes
TracingHelper.dispatchEngineSpansnow receives the log events recorded during the dispatched spans, along with a callback to emit them.ActiveTracingHelperemits each event while its span is active, and also records it on the span, mappingerrorevents torecordException.ignoreSpanTypes, or that have no matching span at all, are still emitted, so no log is silently dropped.disabledTracingHelperemits all events directly. Logging is configured independently of tracing, and tracing can be disabled mid-request viaPrismaInstrumentation.disable()after the server was already asked for spans.Breaking change
TracingHelperis exported from the published@prisma/instrumentation-contractpackage. Custom implementations must accept the two new parameters and pass every event toemitLogEvent, 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.tscovers span-scoped emission, events recorded on their span,recordExceptionforerrorevents, and both leftover paths (ignored span, absent span).packages/client/tests/functional/tracing-event-contextcovers plain queries, batch transactions, and interactive transactions end to end. CI exercises theRemoteExecutorpath via theclient-query-compiler-acceleratejob.Verified by temporarily reverting the fix: three of the four functional tests fail against
provider=postgresql, qpe=remotewithout it and pass with it. The five pre-existing tracing suites pass in both executor modes.Closes: #28792
Closes: #28806