fix: sync service otel and CI - #5390
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes add document IDs to individual document load spans. Error normalization now handles native errors, strings, message-bearing objects, serializable objects, serialization failures, and other values. Telemetry tests cover structured non-Error values. Sync-service configuration now forces worker-build installation and uses 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
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: 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/observability/src/span.ts`:
- Around line 104-122: Update normalizeError so every operation on an unknown
thrown value is guarded: safely read the object’s message property, use the
"Unserializable thrown object" fallback when JSON.stringify returns undefined or
throws, and protect the final String(error) coercion with a fallback that cannot
throw. Ensure normalizeError always returns an Error, including for throwing
getters, unserializable objects, and values with failing string conversion.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: da3d3cee-1491-4606-91d0-85dfc25a79ac
📒 Files selected for processing (5)
apps/web/src/features/block-md/definition.tspackages/observability/src/span.tspackages/observability/src/telemetry.test.tsservices/sync-service/wrangler.docker.tomlservices/sync-service/wrangler.toml
| if ( | ||
| typeof error === "object" && | ||
| error !== null && | ||
| "message" in error && | ||
| typeof error.message === "string" | ||
| ) { | ||
| return new Error(error.message); | ||
| } | ||
|
|
||
| if (typeof error === "object" && error !== null) { | ||
| try { | ||
| const message = JSON.stringify(error); | ||
| if (message !== undefined) return new Error(message); | ||
| } catch { | ||
| return new Error("Unserializable thrown object"); | ||
| } | ||
| } | ||
|
|
||
| return new Error(String(error)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make normalizeError safe for every unknown value.
JSON.stringify(error) can return undefined without throwing. Line 122 then falls through to String(error) instead of returning the intended "Unserializable thrown object" fallback. A custom message getter or string conversion can also throw. This can make SpanImpl.error throw while it is handling the original error.
Keep message access and final coercion inside guarded fallbacks.
Proposed fix
private static normalizeError(error: unknown): Error | string {
- if (error instanceof Error || typeof error === "string") return error;
-
- if (
- typeof error === "object" &&
- error !== null &&
- "message" in error &&
- typeof error.message === "string"
- ) {
- return new Error(error.message);
- }
-
- if (typeof error === "object" && error !== null) {
- try {
- const message = JSON.stringify(error);
- if (message !== undefined) return new Error(message);
- } catch {
- return new Error("Unserializable thrown object");
+ try {
+ if (error instanceof Error || typeof error === "string") return error;
+ if (typeof error === "object" && error !== null) {
+ const message = "message" in error ? error.message : undefined;
+ if (typeof message === "string") return new Error(message);
+ try {
+ const serialized = JSON.stringify(error);
+ return new Error(serialized ?? "Unserializable thrown object");
+ } catch {
+ return new Error("Unserializable thrown object");
+ }
}
+ return new Error(String(error));
+ } catch {
+ return new Error("Unserializable thrown value");
}
-
- return new Error(String(error));
}🤖 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/observability/src/span.ts` around lines 104 - 122, Update
normalizeError so every operation on an unknown thrown value is guarded: safely
read the object’s message property, use the "Unserializable thrown object"
fallback when JSON.stringify returns undefined or throws, and protect the final
String(error) coercion with a fallback that cannot throw. Ensure normalizeError
always returns an Error, including for throwing getters, unserializable objects,
and values with failing string conversion.
There was a problem hiding this comment.
@whutchinson98, reply received.
You are interacting with an AI system.
There was a problem hiding this comment.
@whutchinson98, reply received.
You are interacting with an AI system.
2 participants
No description provided.