Mark D1 batch statement spans as errors when the batch fails

[ayaangazali]

instrumentD1Fn creates one span per statement in a batch() call (packages/otel-cf-workers/src/instrumentation/d1.ts:116), but the catch only sets ERROR on the parent span. The statement spans are ended in the finally with their status still UNSET, so a failed batch renders as a failed parent with a set of apparently successful queries underneath it, which reads exactly backwards from what happened.

This sets the error status on the statement spans too. I left the exception itself on the parent rather than recording it once per statement, since it is one failure rather than N, but say the word if you would rather each statement span carried it.

There was no test file for the D1 bindings, so I added one covering a successful batch and a rejected one. The rejection test fails on current main with expected +0 to be 2, that being UNSET where ERROR was expected. Ran the repo preflight and pnpm run check, both green. Patch changeset for @pydantic/otel-cf-workers included.

Worth noting this one hid from the sweep I used for #201, #203 and #204: d1.ts pairs recordException and setStatus three times each, so it looked balanced, and only reading the batch branch showed the sub-spans were missed.

quick disclosure: built with Claude Code's help and I read the final diff myself. Freshman still learning, so corrections welcome :)

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[coderabbitai]

📝 Walkthrough

Walkthrough

D1 batch instrumentation now marks every query span with SpanStatusCode.ERROR when the batch fails. The exception remains recorded only on the parent batch span. Tests cover successful batches, query span creation, query text attributes, error propagation, and error statuses. A patch changeset documents the behavior.

Possibly related PRs

• pydantic/logfire-js#204: Updates Cloudflare Workers instrumentation to mark failed operation spans with SpanStatusCode.ERROR and adds rejection tests for a different handler.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main change: marking D1 batch statement spans as errors when the batch fails.
Description check	✅ Passed	The description explains the instrumentation change, test coverage, parent-span behavior, and changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

packages/otel-cf-workers/test/instrumentation/d1.test.ts (1)

72-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert exception event placement.

The test verifies statuses but not the contract that only the parent span records the exception. Assert one exception event on parent and zero such events on each query span. This prevents a future change from duplicating exception events.

Proposed test addition

     expect(parent?.status.code).toBe(SpanStatusCode.ERROR)
+    expect(parent?.events.filter((event) => event.name === 'exception')).toHaveLength(1)
     expect(querySpans).toHaveLength(2)
     for (const span of querySpans) {
       expect(span.status.code).toBe(SpanStatusCode.ERROR)
+      expect(span.events.filter((event) => event.name === 'exception')).toHaveLength(0)
     }

🤖 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/otel-cf-workers/test/instrumentation/d1.test.ts` around lines 72 -
76, Update the assertions in the D1 instrumentation test around parent and query
span status checks to verify exception event placement: assert that parent
contains exactly one exception event, and each span in querySpans contains none.
Keep the existing status and span-count assertions unchanged.

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.

Nitpick comments:
In `@packages/otel-cf-workers/test/instrumentation/d1.test.ts`:
- Around line 72-76: Update the assertions in the D1 instrumentation test around
parent and query span status checks to verify exception event placement: assert
that parent contains exactly one exception event, and each span in querySpans
contains none. Keep the existing status and span-count assertions unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ea6e9a59-ce0f-4b92-b990-d1189b7c125f

📥 Commits

Reviewing files that changed from the base of the PR and between 7f25681 and b7958f1.

📒 Files selected for processing (3)

• .changeset/d1-batch-subspan-status.md
• packages/otel-cf-workers/src/instrumentation/d1.ts
• packages/otel-cf-workers/test/instrumentation/d1.test.ts

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• pydantic/logfire (manual)
• pydantic/pydantic-ai (manual)

[petyosi]

Reviewed the full diff and relevant surrounding code. No significant correctness, failure-mode, compatibility, or test-coverage issues found.

[ayaangazali]

Rebased onto current main (79bcac0) and re-verified there. Approved and clean, no conflicts.

Recap since it has been a couple of weeks: instrumentD1Fn opens one sub-span per statement in a batch, but the catch sets ERROR only on the parent, so a failed batch rendered as a failed parent over N apparently successful queries. The rejection test fails on main with expected +0 to be 2.

Happy to close it if you would rather not take this one.