Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThis PR introduces comprehensive end-to-end OTel tracing for the mothership copilot pipeline, linking Sim (Node.js) and Go spans into a single trace via W3C trace-context propagation. Key additions include a new Confidence Score: 5/5Safe to merge — all findings are P2 suggestions with no functional impact on production tracing correctness. The implementation is well-structured with careful attention to ALS boundary issues, span lifecycle correctness (no leaked spans), and explicit context propagation. All three findings are P2: the serviceName override is a documentation/configurability concern for forks, the tracer name inconsistency is cosmetic, and the non-null assertions don't change runtime behavior. apps/sim/instrumentation-node.ts — the serviceName: 'mothership' override and its interaction with the documented user-configurable telemetry.config.ts field. Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser
participant Next as Next.js (sim)
participant OTel as OTel SDK
participant Go as Go Orchestrator
participant Tools as Tool Executor
Browser->>Next: POST /api/copilot/chat
Next->>OTel: startCopilotOtelRoot() → gen_ai.agent.execute (ROOT_CONTEXT)
Note over Next,OTel: requestId = traceId
Next->>Go: fetchGo() + W3C traceparent injected
Go->>Go: child spans under Sim trace
Go-->>Next: SSE stream
loop Stream events
Next->>Next: withCopilotSpan(copilot.*)
Next->>Tools: executeToolAndReport()
Tools->>OTel: withCopilotToolSpan() → tool.execute span
Tools-->>Go: tool result
end
Next->>OTel: finalizeStream() → copilot.finalize.stream
Next->>OTel: otelRoot.finish() → ends gen_ai.agent.execute
Browser->>Next: GET /api/copilot/chat/stream (reconnect)
Next->>OTel: startSpan(copilot.resume.request) parented via traceparent echo
Next->>Next: ReadableStream SSE poll loop
Next->>OTel: rootSpan.end() in finally block
Reviews (1): Last reviewed commit: "Fix lint" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| serviceName: 'mothership', | ||
| } |
There was a problem hiding this comment.
serviceName override silently bypasses user-configurable field
telemetry.config.ts explicitly documents serviceName as configurable for forks of the repo ("You can change this for your fork"), but the code always overwrites it with 'mothership' on this line. Any fork that has customized the service name in telemetry.config.ts will silently get 'mothership' exported in their spans, breaking their own trace backends. The config file's documentation and the override should be reconciled — either drop the config field or conditionally apply this default.
| import type { OrchestratorResult } from '@/lib/copilot/request/types' | ||
|
|
||
| const logger = createLogger('CopilotStreamFinalize') | ||
| const getTracer = () => trace.getTracer('sim-copilot-finalize', '1.0.0') |
There was a problem hiding this comment.
getTracer() here returns trace.getTracer('sim-copilot-finalize', '1.0.0'), while every other mothership span uses getCopilotTracer() (tracer 'sim-ai-platform'). The tracer name is metadata and doesn't affect routing, but this creates an inconsistent otel.library.name dimension in the OTLP backend. The span name copilot.finalize.stream is still covered by ALLOWED_SPAN_PREFIXES so sampling is unaffected, but aligning with the shared helper would improve maintainability.
| [TraceAttr.CopilotOutputFileOutcome]: CopilotOutputFileOutcome.Uploaded, | ||
| }) |
There was a problem hiding this comment.
context.workspaceId is typed as string | undefined in ToolExecutionContext. The added ! suppresses the TypeScript error at compile time but provides no runtime guard — if workspaceId is actually undefined, uploadWorkspaceFile receives undefined silently. An explicit guard or a fallback default would be safer than a blanket assertion.
PR SummaryMedium Risk Overview Updates chat lifecycle to return Fixes a few behavior edges while instrumenting: Reworks Node OTel bootstrap to export only allowlisted spans, add Reviewed by Cursor Bugbot for commit fad203d. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fad203d. Configure here.
| } | ||
| } | ||
| return 1.0 | ||
| } |
There was a problem hiding this comment.
Default sampling ratio increased from 10% to 100%
Medium Severity
resolveSamplingRatio defaults to 1.0 (100%) when no env var is set, whereas the previous code hardcoded TraceIdRatioBasedSampler(0.1) (10%). This is a 10× increase in default trace export volume. Combined with the removal of ParentBasedSampler, business-root spans that were previously sampled at 10% are now always exported, and spans whose Go-side parent was unsampled are now independently re-sampled at 100% — potentially creating broken/orphan traces in backends. Deployments that relied on the old 10% default will see a significant cost and throughput increase without any configuration change.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fad203d. Configure here.
2 participants
Summary
Brief description of what this PR does and why.
Fixes #(issue)
Type of Change
Testing
How has this been tested? What should reviewers focus on?
Checklist