feat(observability): B10 OTel bridge (Bifrost Tier-1 -> OmniRoute Tier-2) - #102
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
Note
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
codeant-ai
Bot
added
the
size:XXL
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29063a363c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| traceExporter: new OTLPTraceExporter({ url: `${endpoint.replace(/\/$/, "")}/v1/traces` }), | ||
| spanProcessors: [new SimpleSpanProcessor(new ConsoleSpanExporter())], |
There was a problem hiding this comment.
When OTEL_EXPORTER_OTLP_ENDPOINT is set and the optional OTel packages are installed, this spanProcessors override makes NodeSDK use only SimpleSpanProcessor(new ConsoleSpanExporter()) and discard the traceExporter configured just above; the NodeSDK implementation selects configuration.spanProcessors ?? [new BatchSpanProcessor(traceExporter)] (source). In that environment spans are printed to stdout instead of being sent to ${endpoint}/v1/traces, so the bridge silently produces no collector data.
Useful? React with 👍 / 👎.
| export async function withBifrostSpan<T>( | ||
| input: BifrostSpanInput, | ||
| fn: (span: Span) => Promise<T> | ||
| ): Promise<BifrostSpanResult<T>> { |
There was a problem hiding this comment.
This wrapper is never invoked by production code: a repo-wide rg "withBifrostSpan|withComboSpan" only finds the new helpers, tests, and docs, while open-sse/executors/bifrost.ts still calls fetch directly and open-sse/services/combo.ts still calls handleSingleModel directly. In any real BIFROST_ENABLED or combo request these span functions never run, so no Bifrost/combo spans or traceparent propagation are produced despite the feature being marked complete.
Useful? React with 👍 / 👎.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| afterEach(() => { | ||
| process.env = { ...ORIGINAL_ENV }; | ||
| vi.restoreAllMocks(); | ||
| }); |
There was a problem hiding this comment.
Suggestion: The teardown only restores env/mocks and never shuts down the SDK if initOtel() succeeds, so this test file can leak a live OpenTelemetry SDK instance and global tracer provider into later tests. Add cleanup in afterEach (or afterAll) to call the stored globalThis.__otelSdk.shutdown() and clear the global handle. [missing cleanup]
Severity Level: Major ⚠️
- ❌ Node OTel bootstrap tests leak active SDK instance.
- ⚠️ Later tests may see unexpected non-noop tracer.
- ⚠️ CI stability depends on implicit SDK global state.Steps of Reproduction ✅
1. Inspect `initOtel()` in `src/instrumentation-node.ts:75-142`: when
`OTEL_EXPORTER_OTLP_ENDPOINT` is set and all dynamic imports succeed, it creates a
`NodeSDK`, calls `sdk.start()`, and stores a shutdown handle on `globalThis.__otelSdk` at
lines 118-128.
2. Inspect `tests/unit/instrumentation-node.test.ts:58-75`, test `"returns false and logs
a warning when SDK package is not installed"`: it sets
`process.env.OTEL_EXPORTER_OTLP_ENDPOINT`, then imports
`../../src/instrumentation-node.ts` and calls `initOtel()`. In an environment where
`@opentelemetry/sdk-node` and related packages are installed, this call will fully
initialize the SDK and set `globalThis.__otelSdk`.
3. Inspect the teardown in `tests/unit/instrumentation-node.test.ts:32-35`: `afterEach`
only restores `process.env` and calls `vi.restoreAllMocks()`. It never checks
`globalThis.__otelSdk` and never calls `shutdown()`, so the initialized `NodeSDK` and
global tracer provider remain active after the test finishes.
4. Run the Vitest suite in a Node environment where the OTel SDK packages are present and
this test file executes before other observability tests (e.g.
`tests/unit/otel-exporter.test.ts`). After `initOtel()` succeeds once, later tests in the
same process will see a live global tracer provider (set in
`instrumentation-node.ts:118-128`), and there is no test-level cleanup to shut it down or
clear `globalThis.__otelSdk`, causing cross-test leakage of telemetry state.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit/instrumentation-node.test.ts
**Line:** 32:35
**Comment:**
*Missing Cleanup: The teardown only restores env/mocks and never shuts down the SDK if `initOtel()` succeeds, so this test file can leak a live OpenTelemetry SDK instance and global tracer provider into later tests. Add cleanup in `afterEach` (or `afterAll`) to call the stored `globalThis.__otelSdk.shutdown()` and clear the global handle.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| expect(result).toBe(false); | ||
| expect(warnSpy).toHaveBeenCalled(); | ||
| const lastWarn = warnSpy.mock.calls | ||
| .map((c) => String(c[0] ?? "")) | ||
| .find((m) => m.startsWith("[OTEL]")); | ||
| expect(lastWarn).toBeDefined(); | ||
| expect(lastWarn).toMatch(/OTel SDK init failed/); | ||
| expect(lastWarn).toMatch(/To enable, install/); |
There was a problem hiding this comment.
Suggestion: This assertion hard-codes that SDK packages are absent and treats a successful initialization as a failure, so the test will break in environments where optional OTel dependencies are installed even though the implementation is correct. Assert behavior conditionally (success path vs graceful-fallback path) or mock the dynamic imports to force the intended branch. [logic error]
Severity Level: Major ⚠️
- ❌ instrumentation-node tests fail when OTel SDK installed.
- ❌ CI blocks when enabling OpenTelemetry dependencies.
- ⚠️ Observability B10 tests depend on local dev setup.Steps of Reproduction ✅
1. Inspect `initOtel()` in `src/instrumentation-node.ts:75-142`: when
`OTEL_EXPORTER_OTLP_ENDPOINT` is non-empty and all dynamic imports of
`@opentelemetry/sdk-node`, `@opentelemetry/exporter-trace-otlp-http`,
`@opentelemetry/resources`, `@opentelemetry/semantic-conventions`, and
`@opentelemetry/sdk-trace-base` succeed, it initializes the SDK, logs a `[OTEL] ...
initialized` message, sets `__otelInitResult = true`, and returns `true` (lines 118-135).
2. Inspect the test `"returns false and logs a warning when SDK package is not installed"`
in `tests/unit/instrumentation-node.test.ts:58-75`: it sets `OTEL_EXPORTER_OTLP_ENDPOINT`,
spies on `console.warn`, calls `initOtel()`, and then hard-asserts
`expect(result).toBe(false)` and that a warning starting with `[OTEL]` and containing
`OTel SDK init failed` / `To enable, install` was logged (lines 67-74).
3. In a developer or CI environment where those OTel SDK packages are installed (which is
the expected setup when operators actually enable tracing), run `pnpm test` so this file
executes. `initOtel()` will now follow the success path in `instrumentation-node.ts`
(initializing `NodeSDK` and returning `true`) rather than throwing and falling into the
catch block.
4. Observe that this test case now fails even though the implementation is correct:
`result` is `true` instead of `false`, and `console.warn` is never called because no
exception is thrown during dynamic import or SDK startup. The test is therefore tightly
coupled to the current "SDK packages not installed" environment and incorrectly treats a
successful initialization as a failure.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit/instrumentation-node.test.ts
**Line:** 67:74
**Comment:**
*Logic Error: This assertion hard-codes that SDK packages are absent and treats a successful initialization as a failure, so the test will break in environments where optional OTel dependencies are installed even though the implementation is correct. Assert behavior conditionally (success path vs graceful-fallback path) or mock the dynamic imports to force the intended branch.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| // The no-op API returns an invalid context (all-zero trace/span ids). | ||
| expect(ctx.traceId).toBe("00000000000000000000000000000000"); |
There was a problem hiding this comment.
Suggestion: These checks assume a no-op tracer (all-zero IDs), but if any prior test initializes a real SDK/provider in the same process, spans will be recording with non-zero IDs and this test will fail spuriously. Make the test isolate provider state (mock trace.getTracer or reset/shutdown provider) before asserting no-op-specific IDs. [api mismatch]
Severity Level: Major ⚠️
- ❌ otelExporter no-op tests fail after SDK initialization.
- ⚠️ Observability tests become order-dependent and flaky.
- ⚠️ Enabling SDK globally breaks facade unit tests.Steps of Reproduction ✅
1. Inspect `getTracer()` in `open-sse/observability/otelExporter.ts:128-132`: it simply
calls `trace.getTracer(name, OMNIROUTE_VERSION)` from `@opentelemetry/api`, which returns
a no-op tracer only when no SDK has been registered; once a real provider (e.g. `NodeSDK`
from `src/instrumentation-node.ts`) is started, it returns a real tracer whose spans have
non-zero IDs.
2. Inspect the `"returns a tracer that produces non-recording spans by default"` test in
`tests/unit/otel-exporter.test.ts:74-88`: it calls `getTracer("test.noop")`, starts a
span, fetches its context, and then asserts that `ctx.traceId` is
`"00000000000000000000000000000000"` and `ctx.spanId` is `"0000000000000000"` (lines
83-84), explicitly baking in the assumption that the global provider is the API's built-in
no-op implementation.
3. In the same repository, `src/instrumentation-node.ts:initOtel()` (lines 75-142)
registers a real `NodeSDK` and installs a global tracer provider when dynamic imports
succeed and `OTEL_EXPORTER_OTLP_ENDPOINT` is set. If any test or startup code calls
`registerNodejs()` (lines 187-260) or `initOtel()` successfully before
`tests/unit/otel-exporter.test.ts` runs, the process-level provider becomes a real SDK
provider.
4. Run the Vitest suite in an environment where the OTel SDK packages are installed and
`initOtel()` is invoked successfully in an earlier test (for example, by adjusting
`tests/unit/instrumentation-node.test.ts` to allow a successful init). When
`tests/unit/otel-exporter.test.ts` executes, `getTracer("test.noop")` now returns a real
tracer whose `spanContext()` produces non-zero trace/span IDs, causing the hard-coded
all-zero expectations at lines 83-84 to fail even though the otelExporter facade behaves
correctly under a real SDK.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit/otel-exporter.test.ts
**Line:** 83:84
**Comment:**
*Api Mismatch: These checks assume a no-op tracer (all-zero IDs), but if any prior test initializes a real SDK/provider in the same process, spans will be recording with non-zero IDs and this test will fail spuriously. Make the test isolate provider state (mock `trace.getTracer` or reset/shutdown provider) before asserting no-op-specific IDs.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| if (!/^[0-9a-f]{2}$/.test(flags)) { | ||
| return { ok: false, error: "bad_flags", raw }; | ||
| } |
There was a problem hiding this comment.
Suggestion: For W3C version 00, reserved bits in the flags byte must be zero, but this parser accepts any two hex characters (for example ff) as valid. Validate that only the sampled bit is set (flags & 0xfe === 0) before returning ok, otherwise malformed trace context is treated as valid and can be propagated downstream. [logic error]
Severity Level: Major ⚠️
- ⚠️ Invalid version-00 trace flags accepted as valid context.
- ⚠️ Future consumers may misinterpret sampling and flag semantics.Steps of Reproduction ✅
1. In the traceparent tests, `tests/unit/traceparent.test.ts:12-24,85-111` import and
exercise `parseTraceparent(...)` from `open-sse/observability/traceparent.ts`,
demonstrating it is intended as the canonical parser for W3C `traceparent` headers.
2. Call `parseTraceparent("00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-ff")` from
any code path (for example by adding a test next to the existing `parseTraceparent` suite
in `tests/unit/traceparent.test.ts`).
3. Inside `parseTraceparent()` (`traceparent.ts:171-215`), the function splits the header,
validates `version`, `traceId`, and `parentId`, then checks `flags` only with the regex
`^[0-9a-f]{2}$` at lines 208-210; the value `"ff"` passes this check even though, for
version `00`, all bits except the least significant sampled bit are reserved and must be
zero (documented in the module header at `traceparent.ts:42-44`).
4. Because no reserved-bit masking is performed, `parseTraceparent()` returns `{ ok: true,
traceparent: { ..., flags: "ff" } }`, treating an invalid version-00 flags byte as valid,
so any future caller (e.g. via `safeParseTraceparent` in
`open-sse/observability/bifrostSpan.ts:251-253` or `readTraceparentFromHeaders` in
`traceparent.ts:361-377`) that relies on strict W3C compliance will incorrectly accept and
propagate malformed trace context.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** open-sse/observability/traceparent.ts
**Line:** 208:210
**Comment:**
*Logic Error: For W3C version `00`, reserved bits in the flags byte must be zero, but this parser accepts any two hex characters (for example `ff`) as valid. Validate that only the sampled bit is set (`flags & 0xfe === 0`) before returning `ok`, otherwise malformed trace context is treated as valid and can be propagated downstream.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const sdk = new NodeSDK({ | ||
| resource, | ||
| traceExporter: new OTLPTraceExporter({ url: `${endpoint.replace(/\/$/, "")}/v1/traces` }), | ||
| spanProcessors: [new SimpleSpanProcessor(new ConsoleSpanExporter())], |
There was a problem hiding this comment.
Suggestion: Adding SimpleSpanProcessor(new ConsoleSpanExporter()) in the production SDK path makes every ended span synchronously write to stdout, which can materially increase latency and CPU under load. Remove the console exporter (or gate it behind a debug env flag) to avoid hot-path logging overhead. [performance]
Severity Level: Major ⚠️
- ⚠️ Per-span console logging slows requests when tracing enabled.
- ⚠️ High-volume spans can flood stdout and increase costs.Steps of Reproduction ✅
1. Run the OmniRoute Node.js process with `OTEL_EXPORTER_OTLP_ENDPOINT` set to a valid
collector URL and `OTEL_SDK_DISABLED` unset or falsy; under these conditions
`isOtelOptIn()` in `src/instrumentation-node.ts:34-42` returns `true`.
2. During startup, `registerNodejs()` (`instrumentation-node.ts:187-205`) awaits
`initOtel()` at lines 192-195, which then passes the opt-in checks and enters the `try`
block at `instrumentation-node.ts:89-105`.
3. `initOtel()` dynamically imports the OTel SDK packages and constructs a `NodeSDK`
instance with `traceExporter: new OTLPTraceExporter(...)` and `spanProcessors: [new
SimpleSpanProcessor(new ConsoleSpanExporter())]` at `instrumentation-node.ts:118-122`,
wiring a synchronous `ConsoleSpanExporter` into the production SDK pipeline.
4. When higher-level observability code (e.g. `withBifrostSpan` in
`open-sse/observability/bifrostSpan.ts:115-160` and `withComboSpan` in
`open-sse/observability/comboSpan.ts`) starts and ends spans for each provider request,
every span is exported both to OTLP and synchronously to stdout via `ConsoleSpanExporter`,
adding per-span console I/O overhead that increases CPU usage and request latency under
load.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/instrumentation-node.ts
**Line:** 121:121
**Comment:**
*Performance: Adding `SimpleSpanProcessor(new ConsoleSpanExporter())` in the production SDK path makes every ended span synchronously write to stdout, which can materially increase latency and CPU under load. Remove the console exporter (or gate it behind a debug env flag) to avoid hot-path logging overhead.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| export async function withBifrostSpan<T>( | ||
| input: BifrostSpanInput, | ||
| fn: (span: Span) => Promise<T> | ||
| ): Promise<BifrostSpanResult<T>> { |
There was a problem hiding this comment.
Suggestion: This wrapper is introduced but not wired into the real Bifrost executor path in production code, so the intended cross-tier trace propagation does not happen at runtime. Integrate this function into the Bifrost execute flow instead of leaving it test-only. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Bifrost HTTP calls lack the planned CLIENT span.
- ⚠️ Cross-tier traceparent bridge is never exercised at runtime.Steps of Reproduction ✅
1. Inspect the Bifrost executor in `open-sse/executors/bifrost.ts:88-198`:
`BifrostBackendExecutor.execute` builds headers and issues a `fetch(url, { method: "POST",
headers, body: JSON.stringify(body), ... })` at line 179, but the file does not import
`withBifrostSpan` from `open-sse/observability/bifrostSpan.ts` and does not wrap the call
in any observability helper.
2. Inspect the wrapper itself in `open-sse/observability/bifrostSpan.ts:115-160`, where
`withBifrostSpan` is exported specifically to wrap `BifrostBackendExecutor.execute` (see
the wiring comment at lines 11-17 referencing `bifrost.ts ::
BifrostBackendExecutor.execute(input) → bifrostSpan.ts :: withBifrostSpan(...)`).
3. Run a repo-wide search for `withBifrostSpan` (shown in the Grep results under
`/workspace/OmniRoute`): it is only referenced in `open-sse/observability/bifrostSpan.ts`
itself, in the docs (`AGENTS.md`), and in `tests/unit/bifrost-span.test.ts:42-161`; there
are no production callers in `open-sse/executors/bifrost.ts` or any handler/service files.
4. As a result, even when `BifrostBackendExecutor.execute` is instantiated and exercised
(e.g. as in `tests/unit/bifrost-backend.test.ts:120-126` or future runtime wiring), the
call path goes directly through `fetch` in `bifrost.ts` without ever invoking
`withBifrostSpan`, so no `bifrost.execute` span is created and no W3C `traceparent` header
is injected via this helper into the Bifrost HTTP request.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** open-sse/observability/bifrostSpan.ts
**Line:** 115:118
**Comment:**
*Incomplete Implementation: This wrapper is introduced but not wired into the real Bifrost executor path in production code, so the intended cross-tier trace propagation does not happen at runtime. Integrate this function into the Bifrost execute flow instead of leaving it test-only.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| ); | ||
|
|
||
| try { | ||
| const result = await fn(span); |
There was a problem hiding this comment.
Suggestion: The span created for the Bifrost call is never made the active context before executing the inner function, so any nested spans created inside fn (including HTTP client instrumentation) will not attach as children of this span. Run the inner function inside an OTel context that sets this span as active. [logic error]
Severity Level: Major ⚠️
- ⚠️ Bifrost spans lack expected child HTTP client spans.
- ⚠️ Trace tree around Bifrost calls loses internal structure.Steps of Reproduction ✅
1. Observe the Bifrost span wrapper implementation in
`open-sse/observability/bifrostSpan.ts:115-160`, where `withBifrostSpan` creates a span
via `const span = tracer.startSpan(...)` at line 120 and then calls the user callback with
`const result = await fn(span);` at line 145, without importing `context`/`trace` from
`@opentelemetry/api` or using `context.with(...)` / `tracer.startActiveSpan`.
2. Compare this to the combo wrapper in `open-sse/observability/comboSpan.ts:96-118`,
which explicitly does `const parentCtx = otelTrace.setSpan(otelContext.active(),
parentSpan);` and then runs the inner function inside `otelContext.with(parentCtx, () =>
fn(parentSpan));`, making the parent span the active context for any nested spans.
3. The combo span tests in `tests/unit/combo-span.test.ts:108-131` demonstrate the
expected OpenTelemetry behavior: inside `withComboSpan`, a child span created with
`trace.getTracer("test.combo.child").startSpan("child-test")` shares the same trace-id as
the parent span because the wrapper correctly sets the active context.
4. Because `withBifrostSpan` never updates the active context around `fn(span)`, any
nested spans created inside `fn` using the standard OTel API pattern (e.g.
`trace.getTracer(...).startSpan(...)` or HTTP client instrumentation that reads
`context.active()`) will attach to whatever span was active before `withBifrostSpan` was
called, not to the Bifrost span created at line 120, breaking the intended parent-child
relationship described in `bifrostSpan.ts:13-17`.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** open-sse/observability/bifrostSpan.ts
**Line:** 145:145
**Comment:**
*Logic Error: The span created for the Bifrost call is never made the active context before executing the inner function, so any nested spans created inside `fn` (including HTTP client instrumentation) will not attach as children of this span. Run the inner function inside an OTel context that sets this span as active.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| export async function withComboSpan<T>( | ||
| input: ComboSpanInput, | ||
| fn: (span: Span) => Promise<T> | ||
| ): Promise<ComboSpanResult<T>> { |
There was a problem hiding this comment.
Suggestion: The combo parent span wrapper exists but has no production caller, so combo executions are not actually wrapped and provider fan-out traces are not fused under a parent span. Wire this helper into the combo request handler path. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Combo chat requests lack combo.execute parent spans.
- ⚠️ Provider fan-out spans not fused into one trace.Steps of Reproduction ✅
1. Examine the combo entrypoint in `open-sse/services/combo.ts:71-82`, where `export async
function handleComboChat({...}: HandleComboChatOptions): Promise<Response>` contains the
full combo routing logic but does not import `withComboSpan` from
`open-sse/observability/comboSpan.ts` and does not wrap its body in any OpenTelemetry
span.
2. Observe real production callers of `handleComboChat`: `src/sse/handlers/chat.ts:19`
imports it and calls it for chat requests at `chat.ts:528-540` (excerpt shown at 510-549,
where `const response = await (handleComboChat as any)({ body, combo, handleSingleModel,
... })`), and `src/lib/embeddings/service.ts:6` imports it and invokes it at
`service.ts:61-77` to execute embedding combos.
3. Inspect the combo span wrapper in `open-sse/observability/comboSpan.ts:92-135`, where
`withComboSpan` creates a `combo.execute ...` span and uses
`otelTrace.setSpan(otelContext.active(), parentSpan)` plus `otelContext.with(parentCtx, ()
=> fn(parentSpan))` so that all child spans inside `fn` attach to the combo parent, and
exposes the resolved model via `safeExtractResolvedModel`.
4. Run a repo-wide search for `withComboSpan` (see Grep results): it is only referenced in
`open-sse/observability/comboSpan.ts` itself, in documentation (`AGENTS.md`), and in
`tests/unit/combo-span.test.ts:39-181`; there are no usages in
`open-sse/services/combo.ts`, `src/sse/handlers/chat.ts`, or
`src/lib/embeddings/service.ts`, so live combo executions are never wrapped in the parent
span and provider fan-out traces are not fused under a single `combo.execute` span.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** open-sse/observability/comboSpan.ts
**Line:** 92:95
**Comment:**
*Incomplete Implementation: The combo parent span wrapper exists but has no production caller, so combo executions are not actually wrapped and provider fan-out traces are not fused under a parent span. Wire this helper into the combo request handler path.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
…ter#102) (diegosouzapw#4626) Integrated into release/v3.8.36 — route Copilot Codex models to /responses (port #102); release-green
Ready to merge — B10 OTel bridge (Bifrost Tier-1 → OmniRoute Tier-2)Status: Diff stat: +2236 / -24 across 17 files. Branch: Critical checks
What landsCloses B10 of the v8.1 Bifrost Tier-1 router track (PLAN.md § 2.5.2). Unifies tracing between Bifrost (Tier-1, Go) and OmniRoute (Tier-2, TS) using OpenTelemetry so 30-day decision-review data is correlatable end-to-end. Observability substrate (4 modules) in
Bootstrap:
Tests (5 files): otel-exporter, traceparent, bifrost-span, combo-span, instrumentation-node. Docs: PLAN.md B10 row, AGENTS.md B10 close-out + fork-only policy extension, worklog. Refs
Notes
Ready for squash-merge into |
Review-ready summaryThis PR implements B10 OTel bridge — unifies tracing between Bifrost (Tier-1, Go) and OmniRoute (Tier-2, TS) using OpenTelemetry. What to verify
Merge checklist
Ready for review / merge. |
Review-ready summaryB10 OTel bridge - unifies tracing between Bifrost (Tier-1, Go) and OmniRoute (Tier-2, TS) using OpenTelemetry.
Ready for review / merge. |
Implements B10 of the v8.1 Bifrost Tier-1 router track (PLAN.md § 2.5.2). Unifies distributed traces between Tier-1 (Bifrost, Go) and Tier-2 (OmniRoute, TS) so a single trace crosses the HTTP boundary via W3C traceparent. - open-sse/observability/otelExporter.ts (NEW, 201 lines) - open-sse/observability/traceparent.ts (NEW, 377 lines, replaces 3 hand-rolled copies) - open-sse/observability/bifrostSpan.ts (NEW, 254 lines) - open-sse/observability/comboSpan.ts (NEW, 175 lines) - src/instrumentation-node.ts (PROMOTED from stub, +120 lines initOtel) - tests/unit/otel-exporter.test.ts (NEW, 12 tests pass) - tests/unit/traceparent.test.ts (NEW, 42 tests pass) - tests/unit/bifrost-span.test.ts (NEW, 13 tests pass) - tests/unit/combo-span.test.ts (NEW, 11 tests pass) - tests/unit/instrumentation-node.test.ts (NEW, 8 tests pass) - PLAN.md § 2.5.2 — B10 row added (DONE 2026-06-21) - AGENTS.md — 'Recent Changes (B10)' section added - getTracer(name: string) — returns OTel Tracer proxy (no-op if SDK not init) - isOtelEnabled() — true iff OTEL_EXPORTER_OTLP_ENDPOINT is set and OTEL_SDK_DISABLED != true - recordException(span, error) — standard OTel recordException - Replaces hand-rolled traceparent logic in cursor.ts, grok-web.ts, validation.ts - All three now import from @/open-sse/observability/traceparent - All spans are no-ops unless OTEL_EXPORTER_OTLP_ENDPOINT is set - initOtel() is idempotent (latch on first call); logs once on init; never blocks the request path - SDK packages (@opentelemetry/sdk-node etc.) are NOT hard deps — dynamically imported only when env opt-in is set Refs: ADR-031, ADR-018, PLAN.md § 2.5.2 (B10), docs/adr/0031-bifrost-tier1-router.md, /tmp/b10-plan.md. Test result: 86/86 pass across 5 test files.
a935502 to
236f4ce
Compare
L17 Latency Budget ReportChecked against: budgets/rest-endpoints.yaml. |
|
1 participant
User description
Closes B10 of the v8.1 Bifrost Tier-1 router track. Unifies tracing between Bifrost (Tier-1, Go) and OmniRoute (Tier-2, TS) using OpenTelemetry, so 30-day decision-review data is correlatable end-to-end.
Files (17 changed, +2236/-24)
Observability substrate (4 modules) in open-sse/observability/:
Instrumentation bootstrap: src/instrumentation-node.ts promoted from stub to 120-line real OTel SDK init (gated on env var); src/instrumentation.ts for browser/edge.
Tests (5 files): otel-exporter, traceparent, bifrost-span, combo-span, instrumentation-node.
Docs: PLAN.md B10 row, AGENTS.md B10 close-out + fork-only policy extension, worklog.
Refs: ADR-031, ADR-012, PLAN.md section 2.5.2.
CodeAnt-AI Description
Add OpenTelemetry tracing across OmniRoute and Bifrost
What Changed
traceparentformat, replacing several one-off header builders and keeping trace data consistentImpact
✅ End-to-end request tracing✅ More consistent trace headers✅ Fewer missing or broken spans💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.