Proxy browser OTLP traces through the server#1739
Conversation
- Add client OTLP tracing configuration and distributed trace headers - Proxy browser OTLP spans to upstream collectors and local trace sink - Co-authored-by: codex <codex@users.noreply.github.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
size:XL
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| afterEach(() => { | ||
| globalThis.WebSocket = originalWebSocket; | ||
| globalThis.fetch = originalFetch; | ||
| void __resetClientTracingForTests(); | ||
| vi.restoreAllMocks(); | ||
| }); |
There was a problem hiding this comment.
🟡 Medium src/wsTransport.test.ts:114
void __resetClientTracingForTests() in afterEach discards the cleanup promise, so the hook returns before tracing state is fully reset. When tests run in quick succession, the next test's configureClientTracing may run against partially-reset state, causing flaky failures. Consider making afterEach async and awaiting the reset.
-afterEach(() => {
+afterEach(async () => {
globalThis.WebSocket = originalWebSocket;
globalThis.fetch = originalFetch;
- void __resetClientTracingForTests();
+ await __resetClientTracingForTests();
vi.restoreAllMocks();
});🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/wsTransport.test.ts around lines 114-119:
`void __resetClientTracingForTests()` in `afterEach` discards the cleanup promise, so the hook returns before tracing state is fully reset. When tests run in quick succession, the next test's `configureClientTracing` may run against partially-reset state, causing flaky failures. Consider making `afterEach` async and awaiting the reset.
Evidence trail:
apps/web/src/wsTransport.test.ts:114-119 (REVIEWED_COMMIT) - shows `void __resetClientTracingForTests()` in non-async afterEach
apps/web/src/observability/clientTracing.ts:131-144 (REVIEWED_COMMIT) - shows `async function __resetClientTracingForTests()` that awaits `disposeTracerRuntime`
apps/web/src/rpc/client.ts:41 (REVIEWED_COMMIT) - shows correct pattern: `await __resetClientTracingForTests()`
![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.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Request body consumed twice may silently break local recording
- Replaced the second HttpServerRequest.schemaBodyJson call with Schema.decodeEffect(Schema.fromJsonString(OtlpTracePayloadSchema)) parsing from the already-read body bytes, preventing the stream double-read; also fixed the upstream forwarding content-type.
Or push these changes by commenting:
@cursor push a5f6c7f0b7
Preview (a5f6c7f0b7)
diff --git a/apps/server/src/http.ts b/apps/server/src/http.ts
--- a/apps/server/src/http.ts
+++ b/apps/server/src/http.ts
@@ -1,5 +1,5 @@
import Mime from "@effect/platform-node/Mime";
-import { Effect, FileSystem, Layer, Option, Path } from "effect";
+import { Effect, FileSystem, Layer, Option, Path, Schema } from "effect";
import {
HttpBody,
HttpClient,
@@ -39,8 +39,10 @@
Effect.map((buffer) => new Uint8Array(buffer)),
);
- // Collect traces to local trace sink
- yield* HttpServerRequest.schemaBodyJson(OtlpTracePayloadSchema).pipe(
+ // Collect traces to local trace sink (parse from already-read body bytes)
+ yield* Schema.decodeEffect(Schema.fromJsonString(OtlpTracePayloadSchema))(
+ new TextDecoder().decode(body),
+ ).pipe(
Effect.map(decodeOtlpTraceRecords),
Effect.flatMap(browserTraceCollector.record),
Effect.catch((cause) => Effect.logWarning("Failed to record browser OTLP traces", { cause })),
@@ -53,7 +55,7 @@
// Forward request to remote OTLP traces endpoint
return yield* httpClient
.post(otlpTracesUrl, {
- body: HttpBody.uint8Array(body),
+ body: HttpBody.uint8Array(body, "application/json"),
})
.pipe(
Effect.flatMap(HttpClientResponse.filterStatusOk),You can send follow-ups to the cloud agent here.
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This PR introduces a new feature for proxying browser OTLP traces through the server, adding a new HTTP endpoint, new services, and client-side tracing infrastructure. The scope includes new runtime behavior and integrations across client and server. Additionally, there's an unresolved medium-severity comment about a potential bug where the promise chain could permanently break after a single failure. You can customize Macroscope's approvability policy. Learn more. |
- Decode Effect OTLP payloads and proxy them to the backend - Disable client tracing on the export HTTP client Co-authored-by: codex <codex@users.noreply.github.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Promise chain permanently breaks after single rejection
- Replaced
.finally(() => apply())with.then(() => apply(), () => apply())so the promise chain recovers from rejections instead of staying permanently poisoned.
- Replaced
- ✅ Fixed: Unvalidated top-level payload crashes on malformed input
- Added
isRecord(payload)andasArray(payload.resourceSpans)guards at the top-level entry point so malformed payloads return an empty array instead of throwing a TypeError.
- Added
- ✅ Fixed: Duplicate error formatting function across two files
- Extracted the shared
formatErrorMessagefunction intoapps/web/src/lib/utils.tsand imported it from bothclientTracing.tsandwsTransport.ts, removing the duplicate definitions.
- Extracted the shared
Or push these changes by commenting:
@cursor push cda3825f94
Preview (cda3825f94)
diff --git a/apps/server/src/observability/TraceRecord.ts b/apps/server/src/observability/TraceRecord.ts
--- a/apps/server/src/observability/TraceRecord.ts
+++ b/apps/server/src/observability/TraceRecord.ts
@@ -137,11 +137,20 @@
payload: OtlpTracer.TraceData,
): ReadonlyArray<OtlpTraceRecord> {
const records: Array<OtlpTraceRecord> = [];
+ const resourceSpans = isRecord(payload) ? asArray(payload.resourceSpans) : [];
- for (const resourceSpan of payload.resourceSpans) {
+ for (const rs of resourceSpans) {
+ if (!isRecord(rs)) {
+ continue;
+ }
+ const resourceSpan = rs as unknown as OtlpTracer.ResourceSpan;
const resourceAttributes = decodeAttributes(resourceSpan.resource?.attributes ?? []);
- for (const scopeSpan of resourceSpan.scopeSpans) {
+ for (const ss of asArray(resourceSpan.scopeSpans)) {
+ if (!isRecord(ss)) {
+ continue;
+ }
+ const scopeSpan = ss as unknown as OtlpTracer.ScopeSpan;
const scopeAttributes = decodeScopeAttributes(scopeSpan);
for (const span of scopeSpan.spans) {
diff --git a/apps/web/src/lib/utils.ts b/apps/web/src/lib/utils.ts
--- a/apps/web/src/lib/utils.ts
+++ b/apps/web/src/lib/utils.ts
@@ -46,6 +46,13 @@
throw new Error("No non-empty string provided");
};
+export function formatErrorMessage(error: unknown): string {
+ if (error instanceof Error && error.message.trim().length > 0) {
+ return error.message;
+ }
+ return globalThis.String(error);
+}
+
export const resolveServerUrl = (options?: {
url?: string | undefined;
protocol?: "http" | "https" | "ws" | "wss" | undefined;
diff --git a/apps/web/src/observability/clientTracing.ts b/apps/web/src/observability/clientTracing.ts
--- a/apps/web/src/observability/clientTracing.ts
+++ b/apps/web/src/observability/clientTracing.ts
@@ -3,7 +3,7 @@
import { OtlpSerialization, OtlpTracer } from "effect/unstable/observability";
import { isElectron } from "../env";
-import { resolveServerUrl } from "../lib/utils";
+import { formatErrorMessage, resolveServerUrl } from "../lib/utils";
import { APP_VERSION } from "~/branding";
const DEFAULT_EXPORT_INTERVAL_MS = 1_000;
@@ -46,7 +46,10 @@
if (config.exportIntervalMs === undefined && activeConfigKey !== null) {
return pendingConfiguration;
}
- pendingConfiguration = pendingConfiguration.finally(() => applyClientTracingConfig(config));
+ pendingConfiguration = pendingConfiguration.then(
+ () => applyClientTracingConfig(config),
+ () => applyClientTracingConfig(config),
+ );
return pendingConfiguration;
}
@@ -101,7 +104,7 @@
if (generation === configurationGeneration) {
console.warn("Failed to configure client tracing exporter", {
- error: formatError(error),
+ error: formatErrorMessage(error),
otlpTracesUrl,
});
}
@@ -124,14 +127,6 @@
});
}
-function formatError(error: unknown): string {
- if (error instanceof Error && error.message.trim().length > 0) {
- return error.message;
- }
-
- return String(error);
-}
-
export async function __resetClientTracingForTests() {
configurationGeneration++;
activeConfigKey = null;
diff --git a/apps/web/src/wsTransport.ts b/apps/web/src/wsTransport.ts
--- a/apps/web/src/wsTransport.ts
+++ b/apps/web/src/wsTransport.ts
@@ -7,6 +7,7 @@
} from "./rpc/protocol";
import { RpcClient } from "effect/unstable/rpc";
import { ClientTracingLive, configureClientTracing } from "./observability/clientTracing";
+import { formatErrorMessage } from "./lib/utils";
interface SubscribeOptions {
readonly retryDelay?: Duration.Input;
@@ -18,13 +19,6 @@
const DEFAULT_SUBSCRIPTION_RETRY_DELAY_MS = Duration.millis(250);
-function formatErrorMessage(error: unknown): string {
- if (error instanceof Error && error.message.trim().length > 0) {
- return error.message;
- }
- return String(error);
-}
-
export class WsTransport {
private readonly runtime: ManagedRuntime.ManagedRuntime<RpcClient.Protocol, never>;
private readonly clientScope: Scope.Closeable;You can send follow-ups to the cloud agent here.
| return pendingConfiguration; | ||
| } | ||
| pendingConfiguration = pendingConfiguration.finally(() => applyClientTracingConfig(config)); | ||
| return pendingConfiguration; |
There was a problem hiding this comment.
Promise chain permanently breaks after single rejection
Medium Severity
Using pendingConfiguration.finally() for sequencing is problematic because finally preserves the original promise's rejection even when the callback resolves successfully. If applyClientTracingConfig ever rejects (e.g., disposeTracerRuntime failing outside the try/catch on line 75), pendingConfiguration becomes permanently rejected. Every subsequent .finally() chain inherits that rejection, making all future configureClientTracing calls return rejected promises. Since runRpc and WsTransport.request both await this promise, all RPC and WebSocket operations would permanently stop working after a single failure.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit c2ab995. Configure here.
| } | ||
|
|
||
| return String(error); | ||
| } |
There was a problem hiding this comment.
Duplicate error formatting function across two files
Low Severity
formatError in clientTracing.ts and formatErrorMessage in wsTransport.ts have identical implementations — both check instanceof Error with a non-empty trimmed message, falling back to String(error). This duplication could be extracted into a shared utility.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c2ab995. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed:
decodeLinksusesflatMapinstead ofmap- Replaced
flatMapwithmapindecodeLinkssince the callback always returns a single object, not an array.
- Replaced
Or push these changes by commenting:
@cursor push 613a5755d9
Preview (613a5755d9)
diff --git a/apps/server/src/observability/TraceRecord.ts b/apps/server/src/observability/TraceRecord.ts
--- a/apps/server/src/observability/TraceRecord.ts
+++ b/apps/server/src/observability/TraceRecord.ts
@@ -223,7 +223,7 @@
}
function decodeLinks(input: ReadonlyArray<OtlpSpanLink>): ReadonlyArray<TraceRecordLink> {
- return input.flatMap((current) => {
+ return input.map((current) => {
const traceId = current.traceId;
const spanId = current.spanId;
return {You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 0c7d79b. Configure here.
| attributes: decodeAttributes(current.attributes), | ||
| }; | ||
| }); | ||
| } |
There was a problem hiding this comment.
decodeLinks uses flatMap instead of map
Low Severity
decodeLinks uses Array.prototype.flatMap but the callback returns a single object, not an array. While JavaScript's flatMap happens to work with non-array returns (treating them as single elements), map is the semantically correct method here. Using flatMap implies intent to return variable-length results per element, which is misleading.
Reviewed by Cursor Bugbot for commit 0c7d79b. Configure here.
- Generate a real browser OTLP payload in the server test - Assert exported span metadata matches the server trace record
|
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
Or push these changes by commenting: Preview (13b5cc2dac)diff --git a/apps/server/src/http.ts b/apps/server/src/http.ts
--- a/apps/server/src/http.ts
+++ b/apps/server/src/http.ts
@@ -39,12 +39,15 @@
yield* Effect.try({
try: () => decodeOtlpTraceRecords(bodyJson),
- catch: (cause) =>
+ catch: (cause) => ({ _tag: "DecodeError" as const, cause }),
+ }).pipe(
+ Effect.flatMap((records) => browserTraceCollector.record(records)),
+ Effect.catch(() =>
Effect.logWarning("Failed to decode browser OTLP traces", {
- cause,
bodyJson,
}),
- }).pipe(Effect.flatMap((records) => browserTraceCollector.record(records)));
+ ),
+ );
if (otlpTracesUrl === undefined) {
return HttpServerResponse.empty({ status: 204 });
diff --git a/apps/server/src/observability/TraceRecord.ts b/apps/server/src/observability/TraceRecord.ts
--- a/apps/server/src/observability/TraceRecord.ts
+++ b/apps/server/src/observability/TraceRecord.ts
@@ -147,13 +147,18 @@
const resourceAttributes = decodeAttributes(resourceSpan.resource?.attributes ?? []);
for (const scopeSpan of resourceSpan.scopeSpans) {
+ const scope = scopeSpan.scope as {
+ name?: string;
+ version?: string;
+ attributes?: ReadonlyArray<OtlpResource.KeyValue>;
+ };
for (const span of scopeSpan.spans) {
records.push(
otlpSpanToTraceRecord({
resourceAttributes,
- scopeAttributes: {},
- scopeName: scopeSpan.scope.name,
- scopeVersion: undefined,
+ scopeAttributes: decodeAttributes(scope?.attributes ?? []),
+ scopeName: scope?.name,
+ scopeVersion: scope?.version,
span,
}),
);
@@ -184,20 +189,23 @@
durationMs:
Number(parseBigInt(input.span.endTimeUnixNano) - parseBigInt(input.span.startTimeUnixNano)) /
1_000_000,
- attributes: decodeAttributes(input.span.attributes),
+ attributes: decodeAttributes(input.span.attributes ?? []),
resourceAttributes: input.resourceAttributes,
scope: {
...(input.scopeName ? { name: input.scopeName } : {}),
...(input.scopeVersion ? { version: input.scopeVersion } : {}),
attributes: input.scopeAttributes,
},
- events: decodeEvents(input.span.events),
- links: decodeLinks(input.span.links),
+ events: decodeEvents(input.span.events ?? []),
+ links: decodeLinks(input.span.links ?? []),
status: decodeStatus(input.span.status),
};
}
-function decodeStatus(input: OtlpSpanStatus): OtlpTraceRecord["status"] {
+function decodeStatus(input: OtlpSpanStatus | undefined): OtlpTraceRecord["status"] {
+ if (input == null) {
+ return undefined;
+ }
const code = String(input.code);
const message = input.message;
@@ -250,7 +258,7 @@
return input.boolValue;
}
if ("intValue" in input) {
- return input.intValue;
+ return Number(input.intValue);
}
if ("doubleValue" in input) {
return input.doubleValue;You can send follow-ups to the cloud agent here. |
Co-authored-by: codex <codex@users.noreply.github.com>
This commit depends on clientTracing from upstream pingdotgg#1739 (OTLP trace proxy), which is in a separate PR (#48). This stub provides the required exports so this PR compiles independently. The real implementation from PR #48 will replace this stub when merged.
…ce-proxy Merge upstream: Proxy browser OTLP traces through the server (pingdotgg#1739)
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
1 participant
Summary
/api/observability/v1/tracesendpoint that accepts browser OTLP exports, records them locally, and forwards them to an upstream collector when configured.Testing
LocalFileTracertests, WebSocket transport tests, and RPC client tracing tests.Note
Medium Risk
Adds a new public HTTP endpoint that accepts and forwards OTLP trace payloads, plus new client-side tracing runtime setup; mistakes could impact observability data integrity or introduce noisy/failed network calls, but changes are scoped and covered by tests.
Overview
Adds a new server route
POST /api/observability/v1/tracesthat accepts browser OTLP JSON payloads, decodes them into normalized trace records, records them via a newBrowserTraceCollectorservice, and (whenServerConfig.otlpTracesUrlis set) forwards the raw payload upstream; includes CORS support and returns204on success or502on upstream failure.Refactors server tracing to share a single local trace sink between server spans and ingested browser spans (
makeLocalFileTracernow accepts an optional externalTraceSink), and expandsTraceRecordto include OTLP-derived span records plus OTLP decoding logic.On the web side, introduces
configureClientTracing/ClientTracingLiveto enable OTLP exporting to the server endpoint and updates RPC/WebSocket runtimes to initialize tracing before issuing requests so distributedtraceId/spanIdpropagate; adds/updates tests for proxying, preflight CORS, local recording, and trace context propagation.Reviewed by Cursor Bugbot for commit c2e0cd0. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Proxy browser OTLP traces through the server to an upstream collector
POST /api/observability/v1/tracesendpoint that accepts OTLP JSON trace data from browsers, decodes it into internalOtlpTraceRecordobjects, stores them via a newBrowserTraceCollectorservice, and optionally proxies the payload to an upstream OTLP collector.decodeOtlpTraceRecordsin TraceRecord.ts to mapOtlpTracer.TraceDatainto typedOtlpTraceRecordobjects, splitting the previousTraceRecordtype intoEffectTraceRecordandOtlpTraceRecord.ObservabilityLivelayer now providesBrowserTraceCollector, writing received records into the same file-backed trace sink used by the local tracer.configureClientTracingin clientTracing.ts initializes an OTLP exporter pointed at the proxy endpoint;WsTransportand the RPC client await tracing readiness before executing requests, and inject trace/span IDs into WS messages.Macroscope summarized c2e0cd0.