Add stream request observability#1523
Merged
Merged
Conversation
Summary by CodeRabbit
WalkthroughThis PR implements stream request observability: adds architecture and product docs, extends stream metadata with observability/profile fields, introduces a typed observe-request hook that POSTs /v1/observe/request and normalizes responses, provides deterministic demo seeding and a scale CLI/ticker, adds StreamObserveSheet plus Timeline/Trace/Event sections and helpers, wires URL-backed navigation state (streamObserve) into StreamView and StreamEventRow, and expands tests and tooling/config for the feature. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 timed out. The project may have too many dependencies for the sandbox. Comment |
Contributor
|
Compute preview deployed. Branch: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Architecture/stream-event-view.md (1)
35-42: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd
use-stream-observe-request.tsto the canonical implementation list.Lines 35-42 still enumerate the stream-view building blocks without the new observe hook, even though Line 295 makes
useStreamObserveRequestthe required fetch boundary. Adding it here keeps the normative doc self-contained for future contributors.🤖 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 `@Architecture/stream-event-view.md` around lines 35 - 42, The canonical list of stream-view building blocks is missing the new observe hook; add `use-stream-observe-request.ts` to the enumerated list and mention the corresponding hook name `useStreamObserveRequest` so the document matches the required fetch boundary referenced elsewhere; update the list alongside the other hooks (`use-stream-events.ts`, `use-stream-details.ts`, `use-stream-aggregations.ts`, `use-ui-state.ts`, `use-navigation.tsx`, `use-stream-observe-request.ts`) and ensure the StreamView/StreamAggregationsPanel/context references remain consistent with `useStreamObserveRequest`.
🤖 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 `@demo/ppg-dev/runtime.ts`:
- Around line 112-131: After startPrismaDevServerImpl(...) succeeds and you've
pushed prismaDevServer.close() into cleanupCallbacks, wrap the subsequent
post-start initialization (seedObservabilityStreamsImpl and
startObservabilityStreamTickerImpl) in a try/catch; on any error, run/drain the
existing cleanupCallbacks (calling each callback, which will invoke
prismaDevServer.close()) before rethrowing the error so the Prisma Dev server is
torn down; update startDemoRuntime() accordingly and add a regression test in
demo/ppg-dev/runtime.test.ts that fakes seedObservabilityStreamsImpl to throw
and asserts prismaDevServer.close() (or the cleanup callback) was called.
In `@demo/ppg-dev/seed-streams-scale.ts`:
- Around line 30-40: The readOptionValue function currently accepts a following
token even if it is another flag; update readOptionValue to detect and reject
values that start with "-" (i.e., treat them as missing) by returning null when
the next token begins with a dash; apply the same change to the other occurrence
referenced (the block around lines 44-54) so both inline and separate-arg
lookups ignore tokens that look like flags, and add a regression test in
demo/ppg-dev/seed-streams-scale.test.ts that calls the CLI with a flag missing
its value (e.g., "--streams-url --batches 2") and asserts the script reports a
clear argument error or null value handling rather than using the flag string as
the URL.
In `@demo/ppg-dev/seed-streams.ts`:
- Around line 238-285: The spans that use "server.address":
"accelerate.prisma-data.net" and are named "fetch POST
accelerate.prisma-data.net" (blocks referencing consoleWorkerResource and
startOffsetMs values like 431/862/1189) must be replaced with direct-TCP demo
hops showing the local DB/service host (e.g., local-db:5432 or the demo service
name) and a TCP/client span name (e.g., "tcp connect to local-db:5432" or
"postgresql client") so the trace reflects the real demo topology; update the
attributes (remove accelerate.* URL fields, set network.protocol.name to tcp and
server.address to the local host/port, adjust name and kind accordingly) and
make the same replacements for the other repeated blocks noted (also present in
the later section around the other occurrence).
In `@scripts/release/check-exports.test.ts`:
- Around line 85-112: The test currently creates a pnpm fixture but never flips
the strict-Corepack condition the bugfix guards against; modify the test to
enable the strict-Corepack failure mode before calling packPackage(directory)
and restore it after the test (e.g., set and later clear the relevant flag or
env var used by the implementation, or stub/mock the helper that detects strict
Corepack such as isStrictCorepack()/detectCorepackStrictness), then call
packPackage(directory), assert the tarball path and existence, and finally call
cleanupPackedTarball(tarballPath); this ensures packPackage and
cleanupPackedTarball are exercised under the strict-Corepack path the fix
targets.
In `@ui/hooks/use-stream-details.test.tsx`:
- Around line 464-494: Add an assertion that the stream profile is normalized to
"evlog" in the test that verifies observability normalization: after rendering
with mockStreamDetailsFetch(createStreamDetailsPayload(...)) and waiting for
harness.getLatestState()?.isSuccess, assert that
harness.getLatestState()?.details?.profile === "evlog" in addition to the
existing observability assertion so the profile-gated request-observability
contract (tested via renderHarness and mockStreamDetailsFetch) is enforced.
In `@ui/studio/views/stream/StreamObserveSheet.test.tsx`:
- Around line 413-434: Add a regression test in StreamObserveSheet.test.tsx that
reproduces the bug when both eventsStream and tracesStream are null: use
renderSheet with lookup: { kind: "requestId", value: "req_8f2k" } and set
tracesStream: null and eventsStream: null, then open the trace section (reuse
waitForSelector('[data-testid="stream-observe-section-trace"]') and click) and
assert that the waterfall element ('[data-testid="stream-observe-waterfall"]')
is absent and the sheet textContent contains the "No otel-traces stream is
available" message (similar to the existing "explains a missing trace stream"
test but with both streams null) to lock down the fix; call sheet.cleanup() at
the end.
In `@ui/studio/views/stream/StreamObserveSheet.tsx`:
- Around line 125-130: StreamObserveSheet opens when lookup !== null but
useStreamObserveRequest does not run if both eventsStream and tracesStream are
null, causing an empty detail pane and active refresh; modify StreamObserveSheet
to detect the "no source" state (eventsStream === null && tracesStream === null)
and handle it explicitly: either render a clear "No source available" message
with the refresh control disabled, or short-circuit before calling
useStreamObserveRequest and avoid rendering the empty result block; ensure the
logic references useStreamObserveRequest, eventsStream, tracesStream, refetch
and the refresh control so stale streamObserve URLs or descriptor misses cannot
produce a blank, still-clickable UI.
In `@ui/studio/views/stream/StreamView.test.tsx`:
- Around line 5297-5334: The mocked streams returned by useStreamsMock do not
include the "configured-traces" stream referenced by the observability
descriptor (tracesStream: "configured-traces"), causing an impossible pairing;
update the test fixture so the useStreamsMock().streams array includes a stream
object with name "configured-traces" (matching the existing
observability/profile values) or alternatively add a separate test that
intentionally omits the stream to validate the missing-stream behavior—ensure
you modify the useStreamsMock return value where the streams array is defined in
StreamView.test.tsx so the names line up with useStreamObserveRequest's
observability descriptor.
- Around line 5283-5390: The test only verifies that clicking an event writes
streamObserveParam but doesn't cover mounting with an existing URL-backed param;
add a new test (or extend this one) that mocks useNavigationMock to return
streamObserveParam: "req:req_2" before rendering StreamView, ensure
useStreamDetailsMock returns the same observability config used here, render
StreamView (createRoot + root.render), and assert that
getNavigationStateValue("streamObserveParam") is already "req:req_2",
useStreamObserveRequestMock was called with the same lookup ({ kind:
"requestId", value: "req_2" }) and that the
'[data-testid="stream-observe-sheet"]' is present on mount to cover the
deep-link/remount path.
In `@ui/studio/views/stream/StreamView.tsx`:
- Around line 884-900: The observe state (streamObserveParam / observeLookup) is
not scoped to the currently selected stream, causing stale lookups when the user
switches streams; update the logic in StreamView to either include the selected
stream identity in the serialized observe state (use serializeStreamObserveParam
/ parseStreamObserveParam to add/read a stream identifier) or clear the param
whenever selectedStreamIdentity changes after mount (call
setStreamObserveParam(null) in a useEffect watching selectedStreamIdentity);
ensure StreamObserveSheetHost receives a lookup tied to the current
selectedStreamIdentity so lookups always resolve against the correct
eventsStream/tracesStream.
---
Outside diff comments:
In `@Architecture/stream-event-view.md`:
- Around line 35-42: The canonical list of stream-view building blocks is
missing the new observe hook; add `use-stream-observe-request.ts` to the
enumerated list and mention the corresponding hook name
`useStreamObserveRequest` so the document matches the required fetch boundary
referenced elsewhere; update the list alongside the other hooks
(`use-stream-events.ts`, `use-stream-details.ts`, `use-stream-aggregations.ts`,
`use-ui-state.ts`, `use-navigation.tsx`, `use-stream-observe-request.ts`) and
ensure the StreamView/StreamAggregationsPanel/context references remain
consistent with `useStreamObserveRequest`.
🪄 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: ff6444df-e3c3-4c60-b379-8e37eb082050
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (42)
.changeset/busy-onions-obey.mdAGENTS.mdArchitecture/navigation-url-state.mdArchitecture/non-standard-ui.mdArchitecture/request-observability.mdArchitecture/stream-event-view.mdArchitecture/streams.mdFEATURES.mdPRODUCT.mdREADME.mddata/mysql-core/adapter.test.tsdata/mysql-core/dml.test.tsdata/mysql-core/introspection.test.tsdemo/ppg-dev/runtime.test.tsdemo/ppg-dev/runtime.tsdemo/ppg-dev/seed-streams-scale.test.tsdemo/ppg-dev/seed-streams-scale.tsdemo/ppg-dev/seed-streams.test.tsdemo/ppg-dev/seed-streams.tseslint.config.mjspackage.jsonscripts/release/check-exports.mjsscripts/release/check-exports.test.tsui/hooks/nuqs.tsui/hooks/react-query.tsui/hooks/use-navigation.tsxui/hooks/use-stream-details.test.tsxui/hooks/use-stream-details.tsui/hooks/use-stream-events.test.tsxui/hooks/use-stream-observe-request.test.tsxui/hooks/use-stream-observe-request.tsui/hooks/use-streams.test.tsxui/hooks/use-streams.tsui/studio/views/stream/StreamObserveEventSection.tsxui/studio/views/stream/StreamObserveShared.tsxui/studio/views/stream/StreamObserveSheet.test.tsxui/studio/views/stream/StreamObserveSheet.tsxui/studio/views/stream/StreamObserveTimelineSection.tsxui/studio/views/stream/StreamObserveTraceSection.tsxui/studio/views/stream/StreamView.test.tsxui/studio/views/stream/StreamView.tsxvitest.config.ts
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 (2)
demo/ppg-dev/runtime.ts (1)
143-151:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExtend startup cleanup to client/executor creation failures.
The new catch only covers seeding/ticker setup. If
createPostgresClient()orcreateExecutor()throws here,startDemoRuntime()still exits with a live local Prisma Dev server because the registeredcleanupCallbacksare never drained. Please keep the entire post-startPrismaDevServerImpl()initialization under the same cleanup-on-failure path, and add a regression indemo/ppg-dev/runtime.test.tsthat makescreatePostgresExecutorthrow and assertsclose()still runs.🤖 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 `@demo/ppg-dev/runtime.ts` around lines 143 - 151, Wrap the entire initialization after startPrismaDevServerImpl() — including createPostgresClient(...) and createExecutor(...) calls that currently live outside the try/catch — in the same failure-handling block so that on any throw you run the registered cleanupCallbacks (e.g., ensure cleanupCallbacks are drained and invoked when createPostgresClient or createExecutor throws); specifically update startDemoRuntime to register cleanupCallbacks before these calls and move those calls into the existing try/catch (or extend the catch) so postgresClient.end is invoked on error. Also add a regression test in demo/ppg-dev/runtime.test.ts that stubs/mocks createPostgresExecutor to throw, calls startDemoRuntime, and asserts that the server close/cleanup callback was executed (verify close() invoked) to prevent regressions.demo/ppg-dev/seed-streams-scale.ts (1)
30-46:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't silently default invalid negative numeric options.
This helper now treats any dash-prefixed token as "missing", so
parseScaleSeedArgs(["--streams-url", "http://x", "--batches", "-1"])falls back toDEFAULT_BATCHESinstead of raising--batches must be a positive integer. That turns an explicit invalid CLI input into a silent default and can append far more seed data than the caller intended. Restrict the dash-token rejection to flag-like options such as--streams-url, or let numeric callers validate-1themselves and add a regression indemo/ppg-dev/seed-streams-scale.test.tsfor--batches -1.🤖 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 `@demo/ppg-dev/seed-streams-scale.ts` around lines 30 - 46, The current readOptionValue implementation treats any dash-prefixed next token as missing, causing negative numbers like "-1" to be ignored; update readOptionValue (used by parseScaleSeedArgs) to only consider the next token "missing" when it is a flag (e.g., startsWith("--") or matches a flag pattern like /^-\w/), or alternatively remove the dash-prefix rejection entirely and let numeric option parsers in parseScaleSeedArgs validate values (so parseScaleSeedArgs can reject negative integers). Modify readOptionValue accordingly and adjust parseScaleSeedArgs validation to ensure an explicit "-1" triggers the intended "must be a positive integer" error.
♻️ Duplicate comments (1)
scripts/release/check-exports.test.ts (1)
106-126:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis still doesn’t reproduce the strict-Corepack path.
Setting
process.env.COREPACK_ENABLE_STRICT = "1"here has no effect on thenpm packsubprocess, becausepackPackage()overwrites that env var to"0"before spawning it. The test therefore still passes on the normal path and won’t catch a regression in the strict-Corepack workaround.Proposed direction
- const previousCorepackStrict = process.env.COREPACK_ENABLE_STRICT; - process.env.COREPACK_ENABLE_STRICT = "1"; - - try { - const tarballPath = packPackage(directory); + const tarballPath = packPackage(directory); expect(tarballPath).toBe( join(directory, "release-fixture-pnpm-7.8.9.tgz"), ); expect(existsSync(tarballPath)).toBe(true); cleanupPackedTarball(tarballPath); expect(existsSync(tarballPath)).toBe(false); - } finally { - if (previousCorepackStrict === undefined) { - delete process.env.COREPACK_ENABLE_STRICT; - } else { - process.env.COREPACK_ENABLE_STRICT = previousCorepackStrict; - } - }If the intent is to prove the override, make this a unit test around the env passed to
execFileSync(for example by mockingnode:child_process) instead of mutatingprocess.envin the parent test.🤖 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 `@scripts/release/check-exports.test.ts` around lines 106 - 126, The test currently sets process.env.COREPACK_ENABLE_STRICT but packPackage overwrites that env before spawning so the subprocess never sees the strict path; replace this integration-style check with a unit test that mocks node:child_process.execFileSync (or similar spawn helper used by packPackage) and calls packPackage(directory) while capturing the options.env passed to execFileSync; assert that when the parent env had COREPACK_ENABLE_STRICT="1" packPackage supplies the expected override value (e.g., "0") in the env passed to execFileSync, and remove the current filesystem assertions that rely on the actual spawned process.
🤖 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 `@demo/ppg-dev/runtime.ts`:
- Around line 143-151: Wrap the entire initialization after
startPrismaDevServerImpl() — including createPostgresClient(...) and
createExecutor(...) calls that currently live outside the try/catch — in the
same failure-handling block so that on any throw you run the registered
cleanupCallbacks (e.g., ensure cleanupCallbacks are drained and invoked when
createPostgresClient or createExecutor throws); specifically update
startDemoRuntime to register cleanupCallbacks before these calls and move those
calls into the existing try/catch (or extend the catch) so postgresClient.end is
invoked on error. Also add a regression test in demo/ppg-dev/runtime.test.ts
that stubs/mocks createPostgresExecutor to throw, calls startDemoRuntime, and
asserts that the server close/cleanup callback was executed (verify close()
invoked) to prevent regressions.
In `@demo/ppg-dev/seed-streams-scale.ts`:
- Around line 30-46: The current readOptionValue implementation treats any
dash-prefixed next token as missing, causing negative numbers like "-1" to be
ignored; update readOptionValue (used by parseScaleSeedArgs) to only consider
the next token "missing" when it is a flag (e.g., startsWith("--") or matches a
flag pattern like /^-\w/), or alternatively remove the dash-prefix rejection
entirely and let numeric option parsers in parseScaleSeedArgs validate values
(so parseScaleSeedArgs can reject negative integers). Modify readOptionValue
accordingly and adjust parseScaleSeedArgs validation to ensure an explicit "-1"
triggers the intended "must be a positive integer" error.
---
Duplicate comments:
In `@scripts/release/check-exports.test.ts`:
- Around line 106-126: The test currently sets
process.env.COREPACK_ENABLE_STRICT but packPackage overwrites that env before
spawning so the subprocess never sees the strict path; replace this
integration-style check with a unit test that mocks
node:child_process.execFileSync (or similar spawn helper used by packPackage)
and calls packPackage(directory) while capturing the options.env passed to
execFileSync; assert that when the parent env had COREPACK_ENABLE_STRICT="1"
packPackage supplies the expected override value (e.g., "0") in the env passed
to execFileSync, and remove the current filesystem assertions that rely on the
actual spawned process.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 85ff3c40-56d9-4f68-b23e-8caf9aa830ec
📒 Files selected for processing (13)
Architecture/stream-event-view.mddemo/ppg-dev/runtime.test.tsdemo/ppg-dev/runtime.tsdemo/ppg-dev/seed-streams-scale.test.tsdemo/ppg-dev/seed-streams-scale.tsdemo/ppg-dev/seed-streams.test.tsdemo/ppg-dev/seed-streams.tsscripts/release/check-exports.test.tsui/hooks/use-stream-details.test.tsxui/studio/views/stream/StreamObserveSheet.test.tsxui/studio/views/stream/StreamObserveSheet.tsxui/studio/views/stream/StreamView.test.tsxui/studio/views/stream/StreamView.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a Studio-owned stream request observability experience for streams that support request-level profiles.
streamObserveand a detail sheet for event payloads, timelines, and trace spans./v1/observe/requestdata hook with profile gating forevlogandotel-traces, stream descriptor resolution, coverage warnings, and error handling.@prisma/devfor the demo runtime.@prisma/studio-core.Review Notes
I reviewed the branch against the request-observability architecture docs and the existing streams/navigation patterns. The implementation keeps the request-observability transport isolated behind the new hook, preserves profile gating, and keeps the selected request state in URL parameters as documented. The only issue I found during review was the missing changeset for the user-facing feature; I added that in the final commit.
Validation
pnpm test ui/hooks/use-stream-observe-request.test.tsx ui/studio/views/stream/StreamObserveSheet.test.tsx ui/studio/views/stream/StreamView.test.tsx ui/hooks/use-streams.test.tsx ui/hooks/use-stream-details.test.tsxpnpm test demo/ppg-dev/seed-streams.test.ts demo/ppg-dev/seed-streams-scale.test.ts demo/ppg-dev/runtime.test.tspnpm typecheckpnpm lint(passes; existing warning-level lint output remains)pnpm exec vitest --project releasepnpm changeset status --since origin/mainpnpm test