From 52602ff2e16727571d3b77b1eb25fa67785aa063 Mon Sep 17 00:00:00 2001 From: mnoah1 Date: Wed, 14 Oct 2026 15:18:25 +0000 Subject: [PATCH] refactor(web): keep instrumentation in reference hosts --- AGENTS.md | 2 +- web/pnpm-lock.yaml | 12 -- web/service/shared/BUILD.bazel | 1 + web/service/shared/instrumentation.test.ts | 70 ++++++++ web/service/shared/instrumentation.ts | 54 ++++++ web/service/shared/package.json | 4 + web/service/stovepipe/BUILD.bazel | 2 + web/service/stovepipe/src/server/host.ts | 13 +- .../src/server/instrumentation.test.ts | 62 +++++++ .../stovepipe/src/server/instrumentation.ts | 35 ++++ web/service/submitqueue/BUILD.bazel | 2 + web/service/submitqueue/src/server/gateway.ts | 4 + web/service/submitqueue/src/server/host.ts | 8 +- .../src/server/instrumentation.test.ts | 88 ++++++++++ .../submitqueue/src/server/instrumentation.ts | 61 +++++++ .../submitqueue/src/server/observability.ts | 4 +- web/stovepipe/BUILD.bazel | 1 - web/stovepipe/README.md | 4 +- web/stovepipe/module.ts | 157 ++++++------------ web/stovepipe/package.json | 2 - web/submitqueue/BUILD.bazel | 1 - web/submitqueue/README.md | 6 +- web/submitqueue/controller/gateway-call.ts | 31 +--- web/submitqueue/core/observability.test.ts | 17 -- web/submitqueue/core/observability.ts | 38 ----- web/submitqueue/module.test.ts | 18 +- web/submitqueue/module.ts | 31 +--- web/submitqueue/package.json | 2 - web/test/package/BUILD.bazel | 2 - web/test/package/pack_contents_test.py | 4 +- web/test/package/react18/BUILD.bazel | 1 - web/tool/library.bzl | 1 - 32 files changed, 480 insertions(+), 258 deletions(-) create mode 100644 web/service/shared/instrumentation.test.ts create mode 100644 web/service/shared/instrumentation.ts create mode 100644 web/service/stovepipe/src/server/instrumentation.test.ts create mode 100644 web/service/stovepipe/src/server/instrumentation.ts create mode 100644 web/service/submitqueue/src/server/instrumentation.test.ts create mode 100644 web/service/submitqueue/src/server/instrumentation.ts delete mode 100644 web/submitqueue/core/observability.test.ts delete mode 100644 web/submitqueue/core/observability.ts diff --git a/AGENTS.md b/AGENTS.md index e85d949cb..54d1c70ec 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -194,7 +194,7 @@ Reviews send changes back for these more often than for anything else. Get them - **Build only for consumers in scope.** Do not add a package, interface, adapter, option, or second supported path for a consumer the change does not ship, such as a second host, a framework that "can't" import CSS, or a caller nobody has asked for. Extract shared code when the second real consumer arrives. When two ways of doing something would both be supported, pick one and enforce it. - **Every file, dependency, and call has a job you can name.** Skip work the behavior does not need: an upfront validation pass when each item is validated before use, an RPC whose result does not reach the output, a config file that restates defaults. If nothing breaks when you delete it, delete it. -- **Use ecosystem types directly.** Library code takes `*zap.SugaredLogger` and tally in Go, and OpenTelemetry meter and tracer in web code, with no-op defaults. Web libraries emit logs through a synchronous, request-scoped callback; the host chooses the logger and its request context. Pino belongs in the reference hosts. Do not define a homegrown `Logger` interface. Wrap a dependency only where the repository actually swaps implementations, which is what an extension contract is for. +- **Use ecosystem types directly.** Library code takes `*zap.SugaredLogger` and tally in Go. Web libraries emit logs through a synchronous, request-scoped callback; hosts instrument page loads and transport clients with their native telemetry APIs. Pino and OpenTelemetry belong in the reference hosts, not the reusable web libraries. Do not define replacement logger, meter, or tracer APIs. Wrap a dependency only where the repository actually swaps implementations, which is what an extension contract is for. - **The library owns behavior; the host owns integration.** `platform/` and domain library code are framework-neutral and ship the complete behavior. In web code that includes layout, styling, pages, and API calls. A host supplies the server, auth, logging, metrics, transport, and configuration, and stays thin. Framework adapters (Next.js and the like) live in the host that uses them. Library code depends on the structural shape of a client, not on generated bindings or a transport. Only the host chooses implementations. - **Enforce boundaries; do not just document them.** Use Bazel `visibility` and tests that fail on forbidden imports. Generated bindings shared by in-repo consumers (such as a host and its e2e test) stay with the contract and are private to those consumers. Consumers outside the repository generate their own from the `.proto` sources. - **Mirror the repository layout in every language.** Go code uses the root tree. A workspace for another language repeats that layout inside itself: `web/platform`, `web/service`, `web/test/e2e`, and `web/tool`, not the repo-level `test/` or `tool/`. Do not substitute a framework's default layout (`web/e2e`), and do not call one host's wiring `platform`. diff --git a/web/pnpm-lock.yaml b/web/pnpm-lock.yaml index 0c3f8f0a6..e35ba401c 100644 --- a/web/pnpm-lock.yaml +++ b/web/pnpm-lock.yaml @@ -285,9 +285,6 @@ importers: specifier: 0.0.0 version: link:../platform devDependencies: - '@opentelemetry/api': - specifier: ^1.9.0 - version: 1.9.0 '@testing-library/jest-dom': specifier: ^6.6.3 version: 6.10.0(@testing-library/dom@10.4.2) @@ -328,9 +325,6 @@ importers: '@connectrpc/connect': specifier: ^2.1.1 version: 2.2.0(@bufbuild/protobuf@2.15.0) - '@opentelemetry/api': - specifier: ^1.9.0 - version: 1.9.1 '@testing-library/jest-dom': specifier: ^6.6.3 version: 6.10.0(@testing-library/dom@10.4.2) @@ -1140,10 +1134,6 @@ packages: resolution: {integrity: sha512-3giAOQvZiH5F9bMlMiv8+GSPMeqg0dbaeo58/0SlA9sxSqZhnUtxzX9/2FzyhS9sWQf5S0GJE0AKBrFqjpeYcg==} engines: {node: '>=8.0.0'} - '@opentelemetry/api@1.9.1': - resolution: {integrity: sha512-gLyJlPHPZYdAk1JENA9LeHejZe1Ti77/pTeFm/nMXmQH/HFZlcS/O2XJB+L8fkbrNSqhdtlvjBVjxwUYanNH5Q==} - engines: {node: '>=8.0.0'} - '@opentelemetry/core@2.12.0': resolution: {integrity: sha512-1HsSAuvT4/my0QrXWsyXFdWjKaPeCHsismKZgEEIb8NK13LP6DrMRlsUbvD/0F9mDWK3xDom9AMv3IscxgOnOQ==} engines: {node: ^18.19.0 || >=20.6.0} @@ -3812,8 +3802,6 @@ snapshots: '@opentelemetry/api@1.9.0': {} - '@opentelemetry/api@1.9.1': {} - '@opentelemetry/core@2.12.0(@opentelemetry/api@1.9.0)': dependencies: '@opentelemetry/api': 1.9.0 diff --git a/web/service/shared/BUILD.bazel b/web/service/shared/BUILD.bazel index 3c802b563..d9633e73b 100644 --- a/web/service/shared/BUILD.bazel +++ b/web/service/shared/BUILD.bazel @@ -16,6 +16,7 @@ web_library( deps = [ ":node_modules/@connectrpc/connect", ":node_modules/@connectrpc/connect-node", + ":node_modules/@opentelemetry/api", ":node_modules/@submitqueue/web-platform", ":node_modules/@types/node", ":node_modules/@types/react", diff --git a/web/service/shared/instrumentation.test.ts b/web/service/shared/instrumentation.test.ts new file mode 100644 index 000000000..cca0bf106 --- /dev/null +++ b/web/service/shared/instrumentation.test.ts @@ -0,0 +1,70 @@ +import { AsyncLocalStorage } from "node:async_hooks"; +import { context, trace, ROOT_CONTEXT, SpanStatusCode, type Context, type ContextManager, type Span, type Tracer } from "@opentelemetry/api"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { observeTelemetry, withSpan } from "./instrumentation.js"; + +function createSpanFixture() { + return { setAttribute: vi.fn(), setStatus: vi.fn(), recordException: vi.fn(), end: vi.fn() }; +} + +afterEach(() => { context.disable(); }); + +describe("reference host OpenTelemetry spans", () => { + it("preserves results and errors while ending each span once", async () => { + const span = createSpanFixture(); + const tracer = { startSpan: vi.fn(() => span) } as unknown as Tracer; + const value = { page: "queue" }; + const execute = vi.fn(async () => value); + expect(await withSpan(tracer, "page", execute)).toBe(value); + expect(execute).toHaveBeenCalledOnce(); + expect(span.end).toHaveBeenCalledOnce(); + + const error = new Error("original failure"); + await expect(withSpan(tracer, "page", async () => { throw error; })).rejects.toBe(error); + expect(span.recordException).toHaveBeenCalledWith(error); + expect(span.setStatus).toHaveBeenCalledWith({ code: SpanStatusCode.ERROR }); + expect(span.end).toHaveBeenCalledTimes(2); + }); + + it("continues exactly once when starting or finishing telemetry fails", async () => { + const failure = () => { throw new Error("telemetry unavailable"); }; + const span = createSpanFixture(); + span.end.mockImplementation(failure); + span.recordException.mockImplementation(failure); + span.setStatus.mockImplementation(failure); + const tracer = { startSpan: vi.fn(() => span).mockImplementationOnce(failure) } as unknown as Tracer; + const execute = vi.fn(async () => "page"); + expect(await withSpan(tracer, "page", execute)).toBe("page"); + expect(await withSpan(tracer, "page", execute)).toBe("page"); + expect(execute).toHaveBeenCalledTimes(2); + const error = new Error("business failure"); + await expect(withSpan(tracer, "page", async () => { throw error; })).rejects.toBe(error); + expect(() => observeTelemetry(failure)).not.toThrow(); + }); + + it("keeps overlapping requests in their native active span across awaits", async () => { + const storage = new AsyncLocalStorage(); + const manager: ContextManager = { + active: () => storage.getStore() ?? ROOT_CONTEXT, + with: (ctx, fn, thisArg, ...args) => storage.run(ctx, () => fn.call(thisArg, ...args)), + bind: (_ctx, target) => target, + enable: () => manager, + disable: () => { storage.disable(); return manager; }, + }; + context.setGlobalContextManager(manager); + const spans = [createSpanFixture(), createSpanFixture()]; + const tracer = { startSpan: vi.fn().mockReturnValueOnce(spans[0]).mockReturnValueOnce(spans[1]) } as unknown as Tracer; + let release!: () => void; + const ready = new Promise(resolve => { release = resolve; }); + const pending = spans.map((span, index) => withSpan(tracer, "page", async active => { + expect(active).toBe(span); + expect(trace.getSpan(context.active())).toBe(span as unknown as Span); + await ready; + expect(trace.getSpan(context.active())).toBe(span as unknown as Span); + return index; + })); + release(); + expect(await Promise.all(pending)).toEqual([0, 1]); + expect(trace.getSpan(context.active())).toBeUndefined(); + }); +}); diff --git a/web/service/shared/instrumentation.ts b/web/service/shared/instrumentation.ts new file mode 100644 index 000000000..feba9fbb1 --- /dev/null +++ b/web/service/shared/instrumentation.ts @@ -0,0 +1,54 @@ +import { context, propagation, trace, SpanKind, SpanStatusCode, type Span, type SpanOptions, type Tracer } from "@opentelemetry/api"; +import type { Interceptor } from "@connectrpc/connect"; + +export const LATENCY_BUCKETS_MS = [5, 10, 25, 50, 100, 250, 500, 1_000, 2_500, 5_000, 10_000]; + +export function observeTelemetry(deliver: () => void): void { + try { + deliver(); + } catch { + // A telemetry failure must not change the business result. + } +} + +/** Uses the host's provider and active context; never repeats work on telemetry failure. */ +export async function withSpan( + tracer: Tracer, + name: string, + execute: (span: Span | undefined) => Promise, + options: SpanOptions = {}, +): Promise { + let span: Span | undefined; + observeTelemetry(() => { span = tracer.startSpan(name, options); }); + let execution: Promise | undefined; + const run = () => execution ??= Promise.resolve().then(() => execute(span)); + try { + const activeSpan = span; + if (activeSpan) { + observeTelemetry(() => { context.with(trace.setSpan(context.active(), activeSpan), run); }); + } + return await run(); + } catch (error) { + observeTelemetry(() => span?.recordException(error instanceof Error ? error : String(error))); + observeTelemetry(() => span?.setStatus({ code: SpanStatusCode.ERROR })); + throw error; + } finally { + observeTelemetry(() => span?.end()); + } +} + +export function traceConnectRequests(tracer: Tracer): Interceptor { + return next => request => withSpan(tracer, `rpc.${request.service.typeName}.${request.method.name}`, async () => { + observeTelemetry(() => propagation.inject(context.active(), request.header, { + set: (headers, key, value) => headers.set(key, value), + })); + return next(request); + }, { + kind: SpanKind.CLIENT, + attributes: { + "rpc.system": "grpc", + "rpc.service": request.service.typeName, + "rpc.method": request.method.name, + }, + }); +} diff --git a/web/service/shared/package.json b/web/service/shared/package.json index 8f97076b0..4065709a1 100644 --- a/web/service/shared/package.json +++ b/web/service/shared/package.json @@ -50,6 +50,10 @@ "types": "./dist/logging.d.ts", "import": "./dist/logging.js" }, + "./instrumentation": { + "types": "./dist/instrumentation.d.ts", + "import": "./dist/instrumentation.js" + }, "./health": { "types": "./dist/health.d.ts", "import": "./dist/health.js" diff --git a/web/service/stovepipe/BUILD.bazel b/web/service/stovepipe/BUILD.bazel index c4fd51774..f77d2b852 100644 --- a/web/service/stovepipe/BUILD.bazel +++ b/web/service/stovepipe/BUILD.bazel @@ -87,6 +87,8 @@ vitest.vitest_test( ":node_modules", ":node_modules/@submitqueue/web-service-shared", ":node_modules/vitest", + ":node_modules/@submitqueue/web-stovepipe", + "//web/stovepipe:lib", "//web:node_modules/@connectrpc/connect", "//web:node_modules/@connectrpc/connect-node", "//web:node_modules/@opentelemetry/api", diff --git a/web/service/stovepipe/src/server/host.ts b/web/service/stovepipe/src/server/host.ts index 03af5eb76..1e13e2d66 100644 --- a/web/service/stovepipe/src/server/host.ts +++ b/web/service/stovepipe/src/server/host.ts @@ -1,9 +1,12 @@ import "server-only"; import { createClient } from "@connectrpc/connect"; +import { metrics, trace } from "@opentelemetry/api"; import { Stovepipe } from "@submitqueue/api/stovepipe"; import { createStovepipeWeb } from "@submitqueue/web-stovepipe/server"; import { pino } from "pino"; import { createPinoLogCallback } from "@submitqueue/web-service-shared/logging"; +import { traceConnectRequests } from "@submitqueue/web-service-shared/instrumentation"; +import { instrumentStovepipeWeb } from "./instrumentation"; import { loadConfiguration } from "./config"; import { newConnectTransport } from "@submitqueue/web-service-shared/transport"; const logger = pino({ @@ -18,12 +21,16 @@ export function resolveHost() { const configuration = loadConfiguration(); const service = createClient( Stovepipe, - newConnectTransport({ ...configuration, defaultTimeoutMs: 5_000 }), + newConnectTransport({ + ...configuration, + defaultTimeoutMs: 5_000, + interceptors: [traceConnectRequests(trace.getTracer("stovepipe-web"))], + }), ); - host = createStovepipeWeb({ + host = instrumentStovepipeWeb(createStovepipeWeb({ service, queues: configuration.queues, - }); + }), metrics.getMeter("stovepipe-web"), trace.getTracer("stovepipe-web")); } return host; } diff --git a/web/service/stovepipe/src/server/instrumentation.test.ts b/web/service/stovepipe/src/server/instrumentation.test.ts new file mode 100644 index 000000000..c19e42598 --- /dev/null +++ b/web/service/stovepipe/src/server/instrumentation.test.ts @@ -0,0 +1,62 @@ +import { SpanStatusCode, type Meter, type Tracer } from "@opentelemetry/api"; +import { createStovepipeWeb } from "@submitqueue/web-stovepipe/server"; +import { describe, expect, it, vi } from "vitest"; +import { instrumentStovepipeWeb } from "./instrumentation"; + +function createHostFixture() { + const list = vi.fn().mockResolvedValue({ requests: [], nextPageToken: "" }); + const getRequestHistoryByID = vi.fn().mockRejectedValue(new Error("private failure")); + const web = createStovepipeWeb({ queues: [{ name: "demo" }], service: { list, getRequestHistoryByID } }); + const record = vi.fn(); + const add = vi.fn(); + const span = { setAttribute: vi.fn(), setStatus: vi.fn(), recordException: vi.fn(), end: vi.fn() }; + const createHistogram = vi.fn(() => ({ record })); + const createCounter = vi.fn(() => ({ add })); + const host = instrumentStovepipeWeb(web, + { createHistogram, createCounter } as unknown as Meter, + { startSpan: () => span } as unknown as Tracer); + return { host, list, web, record, add, span, createHistogram, createCounter }; +} + +describe("Stovepipe host instrumentation", () => { + it("records successful page loads under the existing metric names", async () => { + const fixture = createHostFixture(); + const result = await fixture.host.handle({ path: "/demo", search: new URLSearchParams() }); + expect(result).toMatchObject({ kind: "render", model: { requests: { ok: true } } }); + expect(fixture.createHistogram).toHaveBeenCalledWith("stovepipe.web.page_load_duration", expect.objectContaining({ unit: "ms" })); + expect(fixture.createCounter).toHaveBeenCalledWith("stovepipe.web.page_loads"); + expect(fixture.record).toHaveBeenCalledWith(expect.any(Number), { outcome: "render" }); + expect(fixture.add).toHaveBeenCalledWith(1, { outcome: "render" }); + expect(fixture.span.end).toHaveBeenCalledOnce(); + }); + + it.each(["/demo", "/demo/request/1"])("marks handled section failures at %s as errors", async path => { + const fixture = createHostFixture(); + fixture.list.mockRejectedValue(new Error("private failure")); + const result = await fixture.host.handle({ path, search: new URLSearchParams() }); + expect(result.kind).toBe("render"); + expect(fixture.span.setStatus).toHaveBeenCalledWith({ code: SpanStatusCode.ERROR }); + expect(fixture.record).toHaveBeenCalledWith(expect.any(Number), { outcome: "error" }); + expect(JSON.stringify(result)).not.toContain("private failure"); + }); + + it("does not repeat a load or replace its result when metrics fail", async () => { + const fixture = createHostFixture(); + fixture.record.mockImplementation(() => { throw new Error("metrics unavailable"); }); + fixture.add.mockImplementation(() => { throw new Error("metrics unavailable"); }); + const result = await fixture.host.handle({ path: "/demo", search: new URLSearchParams() }); + expect(result.kind).toBe("render"); + expect(fixture.list).toHaveBeenCalledOnce(); + expect(fixture.span.end).toHaveBeenCalledOnce(); + }); + + it("preserves unexpected failures even when recording fails", async () => { + const fixture = createHostFixture(); + const error = new Error("business failure"); + const handle = vi.spyOn(fixture.web, "handle").mockRejectedValue(error); + fixture.span.recordException.mockImplementation(() => { throw new Error("tracing unavailable"); }); + await expect(fixture.host.handle({ path: "/demo", search: new URLSearchParams() })).rejects.toBe(error); + expect(handle).toHaveBeenCalledOnce(); + expect(fixture.record).toHaveBeenCalledWith(expect.any(Number), { outcome: "exception" }); + }); +}); diff --git a/web/service/stovepipe/src/server/instrumentation.ts b/web/service/stovepipe/src/server/instrumentation.ts new file mode 100644 index 000000000..f9c6b96cb --- /dev/null +++ b/web/service/stovepipe/src/server/instrumentation.ts @@ -0,0 +1,35 @@ +import { SpanStatusCode, type Meter, type Tracer } from "@opentelemetry/api"; +import type { createStovepipeWeb } from "@submitqueue/web-stovepipe/server"; +import { LATENCY_BUCKETS_MS, observeTelemetry, withSpan } from "@submitqueue/web-service-shared/instrumentation"; + +export function instrumentStovepipeWeb( + web: ReturnType, + meter: Meter, + tracer: Tracer, +): ReturnType { + return { + handle: request => withSpan(tracer, "stovepipe.web.page_load", async span => { + const started = performance.now(); + let outcome: string = "exception"; + try { + const result = await web.handle(request); + outcome = result.kind; + if (result.kind === "render") { + const section = result.model.kind === "queue" ? result.model.requests : result.model.history; + if (!section.ok) { + outcome = "error"; + observeTelemetry(() => span?.setStatus({ code: SpanStatusCode.ERROR })); + } + } + return result; + } finally { + observeTelemetry(() => span?.setAttribute("web.outcome", outcome)); + observeTelemetry(() => meter.createCounter("stovepipe.web.page_loads").add(1, { outcome })); + observeTelemetry(() => meter.createHistogram("stovepipe.web.page_load_duration", { + unit: "ms", + advice: { explicitBucketBoundaries: LATENCY_BUCKETS_MS }, + }).record(Math.max(0, performance.now() - started), { outcome })); + } + }), + }; +} diff --git a/web/service/submitqueue/BUILD.bazel b/web/service/submitqueue/BUILD.bazel index 7ab617a0c..5e6494b31 100644 --- a/web/service/submitqueue/BUILD.bazel +++ b/web/service/submitqueue/BUILD.bazel @@ -87,6 +87,8 @@ vitest.vitest_test( data = [ ":node_modules", ":node_modules/vitest", + ":node_modules/@submitqueue/web-submitqueue", + "//web/submitqueue:lib", "//web:node_modules/@connectrpc/connect", "//web:node_modules/@connectrpc/connect-node", "//web:node_modules/@opentelemetry/api", diff --git a/web/service/submitqueue/src/server/gateway.ts b/web/service/submitqueue/src/server/gateway.ts index ccf31e2a7..e1e0d4a10 100644 --- a/web/service/submitqueue/src/server/gateway.ts +++ b/web/service/submitqueue/src/server/gateway.ts @@ -3,6 +3,9 @@ import "server-only"; import { createClient, type Client } from "@connectrpc/connect"; import { SubmitQueueGateway } from "@submitqueue/api/submitqueue/gateway"; import { newConnectTransport } from "@submitqueue/web-service-shared/transport"; +import { traceConnectRequests } from "@submitqueue/web-service-shared/instrumentation"; +import { measureGatewayRequests } from "./instrumentation"; +import { referenceTracer, resolveReferenceMeter } from "./observability"; import { GATEWAY_DEADLINE_MS, GATEWAY_HEALTH_DEADLINE_MS, loadGatewayConfiguration } from "./config"; let client: Client | undefined; @@ -12,6 +15,7 @@ export function resolveReferenceGateway(): Client { client ??= createClient(SubmitQueueGateway, newConnectTransport({ ...loadGatewayConfiguration(), defaultTimeoutMs: GATEWAY_DEADLINE_MS, + interceptors: [traceConnectRequests(referenceTracer), measureGatewayRequests(resolveReferenceMeter())], })); return client; } diff --git a/web/service/submitqueue/src/server/host.ts b/web/service/submitqueue/src/server/host.ts index 36593d63d..44825cd9d 100644 --- a/web/service/submitqueue/src/server/host.ts +++ b/web/service/submitqueue/src/server/host.ts @@ -4,7 +4,8 @@ import { newHmacCursorCodec } from "@submitqueue/web-submitqueue/extension/curso import { createSubmitQueueWeb, type SubmitQueueWeb } from "@submitqueue/web-submitqueue/server"; import { loadCursorSecret } from "./config"; import { resolveReferenceGateway } from "./gateway"; -import { resolveReferenceMeter } from "./observability"; +import { instrumentSubmitQueueWeb } from "./instrumentation"; +import { referenceTracer, resolveReferenceMeter } from "./observability"; let web: SubmitQueueWeb | undefined; @@ -14,10 +15,9 @@ let web: SubmitQueueWeb | undefined; * metrics. Frameworks adapt this composition; they do not build their own. */ export function resolveReferenceWeb(): SubmitQueueWeb { - web ??= createSubmitQueueWeb({ + web ??= instrumentSubmitQueueWeb(createSubmitQueueWeb({ gateway: resolveReferenceGateway, cursors: newHmacCursorCodec(loadCursorSecret()), - meter: resolveReferenceMeter(), - }); + }), resolveReferenceMeter(), referenceTracer); return web; } diff --git a/web/service/submitqueue/src/server/instrumentation.test.ts b/web/service/submitqueue/src/server/instrumentation.test.ts new file mode 100644 index 000000000..0255999a4 --- /dev/null +++ b/web/service/submitqueue/src/server/instrumentation.test.ts @@ -0,0 +1,88 @@ +import { Code, ConnectError, type UnaryRequest, type UnaryResponse } from "@connectrpc/connect"; +import { SpanStatusCode, type Meter, type Tracer } from "@opentelemetry/api"; +import { createSubmitQueueWeb } from "@submitqueue/web-submitqueue/server"; +import { createFakeGatewayReader } from "@submitqueue/web-submitqueue/extension/gateway/mock"; +import { newHmacCursorCodec } from "@submitqueue/web-submitqueue/extension/cursor/hmac"; +import { describe, expect, it, vi } from "vitest"; +import { instrumentSubmitQueueWeb, measureGatewayRequests } from "./instrumentation"; + +function createHostFixture() { + const gateway = createFakeGatewayReader(); + const web = createSubmitQueueWeb({ gateway, cursors: newHmacCursorCodec("test-only-secret") }); + const span = { setAttribute: vi.fn(), setStatus: vi.fn(), recordException: vi.fn(), end: vi.fn() }; + const record = vi.fn(); + const createHistogram = vi.fn(() => ({ record })); + const host = instrumentSubmitQueueWeb(web, { createHistogram } as unknown as Meter, { startSpan: () => span } as unknown as Tracer); + return { gateway, web, host, span, record, createHistogram }; +} + +describe("SubmitQueue host instrumentation", () => { + it("records page outcomes with the existing metric name, units and page tag", async () => { + const fixture = createHostFixture(); + const result = await fixture.host.handle({ path: "/", search: {} }); + expect(result.kind).toBe("render"); + expect(fixture.createHistogram).toHaveBeenCalledWith("web.page_load.duration", expect.objectContaining({ unit: "ms" })); + expect(fixture.record).toHaveBeenCalledWith(expect.any(Number), { page: "queues", outcome: "render" }); + expect(fixture.span.setAttribute).toHaveBeenCalledWith("web.page", "queues"); + expect(fixture.span.end).toHaveBeenCalledOnce(); + }); + + it("marks handled gateway failures without exposing their raw cause in the model", async () => { + const fixture = createHostFixture(); + vi.spyOn(fixture.gateway, "listQueues").mockRejectedValue(new ConnectError("private failure", Code.Unavailable)); + const result = await fixture.host.handle({ path: "/", search: {} }); + expect(result).toMatchObject({ kind: "render", model: { result: { ok: false } } }); + expect(fixture.record).toHaveBeenCalledWith(expect.any(Number), { page: "queues", outcome: "transient" }); + expect(fixture.span.setStatus).toHaveBeenCalledWith({ code: SpanStatusCode.ERROR }); + expect(JSON.stringify(result)).not.toContain("private failure"); + }); + + it("preserves the result and executes once when telemetry delivery fails", async () => { + const fixture = createHostFixture(); + fixture.record.mockImplementation(() => { throw new Error("metrics unavailable"); }); + fixture.span.setAttribute.mockImplementation(() => { throw new Error("tracing unavailable"); }); + const handle = vi.spyOn(fixture.web, "handle"); + expect((await fixture.host.handle({ path: "/", search: {} })).kind).toBe("render"); + expect(handle).toHaveBeenCalledOnce(); + expect(fixture.span.end).toHaveBeenCalledOnce(); + }); +}); + +describe("SubmitQueue transport metrics", () => { + it.each([ + ["List", "list"], + ["ListQueues", "queues"], + ["GetRequestSummaryByID", "summary"], + ["GetRequestHistoryByID", "history"], + ["GetRequestHistoryByChangeURI", "change"], + ])("measures %s at the transport boundary", async (method, operation) => { + const record = vi.fn(); + const meter = { createHistogram: vi.fn(() => ({ record })) } as unknown as Meter; + const request = { method: { name: method } } as unknown as UnaryRequest; + const response = {} as UnaryResponse; + const next = vi.fn(async () => response); + expect(await measureGatewayRequests(meter)(next)(request)).toBe(response); + expect(next).toHaveBeenCalledExactlyOnceWith(request); + expect(record).toHaveBeenCalledWith(expect.any(Number), { operation, outcome: "success" }); + }); + + it("preserves RPC errors and records their classification even if metrics fail", async () => { + const error = new ConnectError("private failure", Code.Unavailable); + const record = vi.fn(() => { throw new Error("metrics unavailable"); }); + const meter = { createHistogram: () => ({ record }) } as unknown as Meter; + const next = vi.fn(async () => { throw error; }); + const request = { method: { name: "List" } } as unknown as UnaryRequest; + await expect(measureGatewayRequests(meter)(next)(request)).rejects.toBe(error); + expect(next).toHaveBeenCalledOnce(); + expect(record).toHaveBeenCalledWith(expect.any(Number), { operation: "list", outcome: "unavailable" }); + }); + + it("does not mix readiness probes into the read-operation metrics", async () => { + const createHistogram = vi.fn(); + const meter = { createHistogram } as unknown as Meter; + const request = { method: { name: "Ping" } } as unknown as UnaryRequest; + const response = {} as UnaryResponse; + expect(await measureGatewayRequests(meter)(async () => response)(request)).toBe(response); + expect(createHistogram).not.toHaveBeenCalled(); + }); +}); diff --git a/web/service/submitqueue/src/server/instrumentation.ts b/web/service/submitqueue/src/server/instrumentation.ts new file mode 100644 index 000000000..6e0bfd459 --- /dev/null +++ b/web/service/submitqueue/src/server/instrumentation.ts @@ -0,0 +1,61 @@ +import { SpanStatusCode, type Meter, type Tracer } from "@opentelemetry/api"; +import type { Interceptor } from "@connectrpc/connect"; +import { grpcStatusErrorCode, type SubmitQueueWeb } from "@submitqueue/web-submitqueue/server"; +import { LATENCY_BUCKETS_MS, observeTelemetry, withSpan } from "@submitqueue/web-service-shared/instrumentation"; + +export function instrumentSubmitQueueWeb(web: SubmitQueueWeb, meter: Meter, tracer: Tracer): SubmitQueueWeb { + return { + handle: request => withSpan(tracer, "web.page_load", async span => { + const started = performance.now(); + let outcome: string = "exception"; + let page = "unknown"; + try { + const result = await web.handle(request); + outcome = result.kind; + if (result.kind === "render") { + page = result.model.kind; + if (!result.model.result.ok) { + outcome = result.model.result.error.kind; + observeTelemetry(() => span?.setStatus({ code: SpanStatusCode.ERROR })); + } + } + return result; + } finally { + observeTelemetry(() => span?.setAttribute("web.page", page)); + observeTelemetry(() => span?.setAttribute("web.outcome", outcome)); + observeTelemetry(() => meter.createHistogram("web.page_load.duration", { + unit: "ms", + advice: { explicitBucketBoundaries: LATENCY_BUCKETS_MS }, + }).record(Math.max(0, performance.now() - started), { page, outcome })); + } + }), + }; +} + +const gatewayOperations: Readonly> = { + List: "list", + ListQueues: "queues", + GetRequestSummaryByID: "summary", + GetRequestHistoryByID: "history", + GetRequestHistoryByChangeURI: "change", +}; + +export function measureGatewayRequests(meter: Meter): Interceptor { + return next => async request => { + const operation = gatewayOperations[request.method.name]; + if (!operation) return next(request); + const started = performance.now(); + let outcome = "success"; + try { + return await next(request); + } catch (error) { + outcome = grpcStatusErrorCode(error); + throw error; + } finally { + observeTelemetry(() => meter.createHistogram("submitqueue_web.gateway.duration", { + unit: "ms", + advice: { explicitBucketBoundaries: LATENCY_BUCKETS_MS }, + }).record(Math.max(0, performance.now() - started), { operation, outcome })); + } + }; +} diff --git a/web/service/submitqueue/src/server/observability.ts b/web/service/submitqueue/src/server/observability.ts index d3ab512fd..e7545bcba 100644 --- a/web/service/submitqueue/src/server/observability.ts +++ b/web/service/submitqueue/src/server/observability.ts @@ -1,6 +1,6 @@ import "server-only"; -import { metrics, type Meter } from "@opentelemetry/api"; +import { metrics, trace, type Meter } from "@opentelemetry/api"; import { PrometheusExporter } from "@opentelemetry/exporter-prometheus"; import { MeterProvider } from "@opentelemetry/sdk-metrics"; import { pino, type Logger } from "pino"; @@ -19,6 +19,8 @@ export function resolveReferenceLogCallback() { return createPinoLogCallback(resolveReferenceLogger()); } +export const referenceTracer = trace.getTracer("submitqueue-web"); + let meter: Meter | undefined; /** diff --git a/web/stovepipe/BUILD.bazel b/web/stovepipe/BUILD.bazel index 8e53b7db2..05bd54fdd 100644 --- a/web/stovepipe/BUILD.bazel +++ b/web/stovepipe/BUILD.bazel @@ -24,7 +24,6 @@ web_library( ], vitest = vitest, deps = [ - ":node_modules/@opentelemetry/api", ":node_modules/@submitqueue/web-platform", ":node_modules/@types/node", ":node_modules/@types/react", diff --git a/web/stovepipe/README.md b/web/stovepipe/README.md index 723edc7c2..c8f868e8e 100644 --- a/web/stovepipe/README.md +++ b/web/stovepipe/README.md @@ -1,11 +1,13 @@ # Stovepipe web UI -A framework-neutral React UI with a queue request list and a request page containing the current lifecycle summary and retained history. The host supplies queue configuration, a structural `StovepipeService` client, authentication, transport, logging and OpenTelemetry providers. +A framework-neutral React UI with a queue request list and a request page containing the current lifecycle summary and retained history. The host supplies queue configuration, a structural `StovepipeService` client, authentication, transport and optional logging. Logging uses a synchronous `onLog` callback passed on each `handle` request. Bind it to the host's request-scoped logger; it receives severity, message, and structured metadata containing the original `error`, correlation `requestId`, and page `path`. Omitting it is silent, and callback exceptions do not change page results. The library has no logging-library dependency. ## Integration +Instrumentation belongs in the host: wrap `handle` to observe page results and instrument the concrete service client for RPC traces. A `render` result can contain a failed section, so check `requests.ok` or `history.ok` before reporting success. The reference host uses native OpenTelemetry spans, a page-load counter and a duration metric; other hosts can use their own telemetry without an adapter. The reusable module has no telemetry dependency or instrumentation API. + Use the server entry's `createStovepipeWeb` to load a page and render its model with `StovepipeApp`. Mount beneath `WebNavigationProvider` when the host supplies client navigation and refresh. Without it, links and refresh use ordinary document navigation. Import `@submitqueue/web-stovepipe/styles.css`, which includes the common native styles from `@submitqueue/web-platform`. ## Routes and data diff --git a/web/stovepipe/module.ts b/web/stovepipe/module.ts index f2ee5d2ea..4027ac70a 100644 --- a/web/stovepipe/module.ts +++ b/web/stovepipe/module.ts @@ -1,4 +1,3 @@ -import { metrics, trace, SpanStatusCode, type Meter, type Tracer } from "@opentelemetry/api"; import { createLogCallback, type LogCallback } from "@submitqueue/web-platform/logging"; import { parseStovepipeRoute } from "./core/route.js"; import type { StovepipeService } from "./extension/service/index.js"; @@ -25,121 +24,73 @@ export type StovepipeWebResult = | { kind: "forbidden" } | { kind: "redirect"; href: string }; export interface StovepipeWebOptions { - service: - | StovepipeService - | ((request: StovepipeWebRequest) => StovepipeService); + service: StovepipeService | ((request: StovepipeWebRequest) => StovepipeService); queues: readonly Queue[]; paths?: StovepipePaths; authorize?(principal: unknown, queue: string): boolean | Promise; - meter?: Meter; - tracer?: Tracer; } export function createStovepipeWeb(options: StovepipeWebOptions) { const paths = options.paths ?? new StovepipePaths(); - const meter = options.meter ?? metrics.getMeter("stovepipe-web"); - const tracer = options.tracer ?? trace.getTracer("stovepipe-web"); - const loads = meter.createCounter("stovepipe.web.page_loads"); - const latency = meter.createHistogram("stovepipe.web.page_load_duration", { - unit: "ms", - }); return { async handle(request: StovepipeWebRequest): Promise { - const started = Date.now(); const onLog = createLogCallback(request.onLog, { requestId: request.requestId ?? globalThis.crypto.randomUUID(), path: request.path, }); - return tracer.startActiveSpan( - "stovepipe.web.page_load", - async (span): Promise => { - try { - if (request.path === (paths.basePath || "/")) { - const first = options.queues[0]; - return first - ? { kind: "redirect", href: paths.queue(first.name) } - : { kind: "not-found" }; - } - const route = parseStovepipeRoute( - request.path, - paths, - options.queues, - ); - if (!route) return { kind: "not-found" }; - const { queue, requestId } = route; - if ( - options.authorize && - !(await options.authorize(request.principal, queue.name)) - ) - return { kind: "forbidden" }; - const service = - typeof options.service === "function" - ? options.service(request) - : options.service; - const common = { - key: `${request.path}?${request.search}`, - queue: queue.name, - queues: options.queues.map((queue) => ({ - name: queue.name, - href: paths.queue(queue.name), - })), - queueHref: paths.queue(queue.name), - latestHref: paths.queue(queue.name), - showLatestLink: Boolean( - !requestId && request.search.get("page"), + try { + if (request.path === (paths.basePath || "/")) { + const first = options.queues[0]; + return first + ? { kind: "redirect", href: paths.queue(first.name) } + : { kind: "not-found" }; + } + const route = parseStovepipeRoute(request.path, paths, options.queues); + if (!route) return { kind: "not-found" }; + const { queue, requestId } = route; + if (options.authorize && !(await options.authorize(request.principal, queue.name))) + return { kind: "forbidden" }; + const service = + typeof options.service === "function" ? options.service(request) : options.service; + const common = { + key: `${request.path}?${request.search}`, + queue: queue.name, + queues: options.queues.map((queue) => ({ + name: queue.name, + href: paths.queue(queue.name), + })), + queueHref: paths.queue(queue.name), + latestHref: paths.queue(queue.name), + showLatestLink: Boolean(!requestId && request.search.get("page")), + loadedAtMs: Date.now(), + }; + if (!requestId) { + const requests = await readSection( + () => + loadQueueRequests( + service, + paths, + queue.name, + request.search.get("page") ?? "", + request.signal ), - loadedAtMs: Date.now(), - }; - if (!requestId) { - const requests = await readSection( - () => - loadQueueRequests( - service, - paths, - queue.name, - request.search.get("page") ?? "", - request.signal, - ), - onLog, - "queue requests", - ); - return { - kind: "render", - model: { ...common, kind: "queue", requests }, - }; - } - const history = await readSection( - () => - loadRequestHistory( - service, - paths, - queue.name, - requestId, - request.signal, - ), - onLog, - "request history", - ); - return { - kind: "render", - model: { - ...common, - kind: "request", - requestId, - history, - }, - }; - } catch (error) { - onLog("error", "Web page load failed", { error }); - span.recordException(error instanceof Error ? error : String(error)); - span.setStatus({ code: SpanStatusCode.ERROR }); - throw error; - } finally { - loads.add(1); - latency.record(Date.now() - started); - span.end(); - } - }, - ); + onLog, + "queue requests" + ); + return { kind: "render", model: { ...common, kind: "queue", requests } }; + } + const history = await readSection( + () => loadRequestHistory(service, paths, queue.name, requestId, request.signal), + onLog, + "request history" + ); + return { + kind: "render", + model: { ...common, kind: "request", requestId, history }, + }; + } catch (error) { + onLog("error", "Web page load failed", { error }); + throw error; + } }, }; } diff --git a/web/stovepipe/package.json b/web/stovepipe/package.json index 981449d42..cd69ed8f8 100644 --- a/web/stovepipe/package.json +++ b/web/stovepipe/package.json @@ -28,11 +28,9 @@ "typecheck": "tsc --noEmit" }, "peerDependencies": { - "@opentelemetry/api": ">=1.9 <2", "react": ">=18 <20" }, "devDependencies": { - "@opentelemetry/api": "^1.9.0", "@testing-library/jest-dom": "^6.6.3", "@testing-library/react": "^16.3.0", "@types/node": "22.18.6", diff --git a/web/submitqueue/BUILD.bazel b/web/submitqueue/BUILD.bazel index 5db0fd3f9..78761503f 100644 --- a/web/submitqueue/BUILD.bazel +++ b/web/submitqueue/BUILD.bazel @@ -25,7 +25,6 @@ web_library( ], vitest = vitest, deps = [ - ":node_modules/@opentelemetry/api", ":node_modules/@submitqueue/web-platform", ":node_modules/@types/node", ":node_modules/@types/react", diff --git a/web/submitqueue/README.md b/web/submitqueue/README.md index ba95102ca..230f7bf1e 100644 --- a/web/submitqueue/README.md +++ b/web/submitqueue/README.md @@ -5,7 +5,7 @@ The complete read-only SubmitQueue UI as a mountable module. Folders follow the | Folder | Holds | |---|---| | [`entity/`](entity/) | Serializable view models (requests, history, queues, changes, pages) and load results | -| [`core/`](core/) | Paths and `parseRoute`, change references, status table, receipt windows, observability helpers | +| [`core/`](core/) | Paths and `parseRoute`, change references, status table, receipt windows | | [`extension/gateway/`](extension/gateway/) | The structural gateway contract, the gRPC-status classifier, and test fakes in `mock/` | | [`extension/cursor/`](extension/cursor/) | The page-cursor contract and its HMAC implementation in `hmac/` | | [`controller/`](controller/) | One loader per page: directory, queue, request, change | @@ -14,10 +14,12 @@ The complete read-only SubmitQueue UI as a mountable module. Folders follow the Entry points: the root is client-safe (`SubmitQueueApp`, page frames, `WebNavigationProvider`, model types); `./server` has `createSubmitQueueWeb`, the gateway and cursor contracts, and `WebPaths`; `./extension/cursor/hmac` signs cursors; `./extension/gateway/mock` has test fakes; `./styles.css` is the full stylesheet. Controllers and helpers stay internal. -**Hosting it.** Give `createSubmitQueueWeb` a gateway client (any client matching the structural contract, so the module ships no generated code), a `CursorCodec`, and optionally a mount path, authorization check, meter, tracer, error classifier, or cached queue catalog. Route every page path to `handle`, render `SubmitQueueApp` with each page model, and wrap it in a `WebNavigationProvider` for client-side navigation. Hosts must load `./styles.css`, either by importing it or by serving it and linking to it; theme with the `--sq-*` properties. +**Hosting it.** Give `createSubmitQueueWeb` a gateway client (any client matching the structural contract, so the module ships no generated code), a `CursorCodec`, and optionally a mount path, authorization check, error classifier, or cached queue catalog. Route every page path to `handle`, render `SubmitQueueApp` with each page model, and wrap it in a `WebNavigationProvider` for client-side navigation. Hosts must load `./styles.css`, either by importing it or by serving it and linking to it; theme with the `--sq-*` properties. Pass `onLog` on each `handle` request to deliver severity, message, and structured metadata to the host's request-scoped logger. Errors retain their original value under `error`, and logs include the correlation `requestId` and page `path`. The callback must be synchronous; omitting it is silent, and callback exceptions do not change page results. The module has no logging-library dependency. +Instrumentation belongs in the host: wrap `handle` to observe page results and instrument the concrete gateway client for RPC traces and timings. A `render` result can contain a failed load, so check `model.result.ok` before reporting success. The reference host uses native OpenTelemetry APIs; other hosts can use their own telemetry without an adapter. The reusable module has no telemetry dependency or instrumentation API. + **Behavior worth knowing.** Expected outcomes (missing resources, stale cursors) are results, never exceptions. Raw gateway errors are logged, never rendered. Views keep the last good data on transient failures. Request IDs stay opaque in URLs; dot-only segments get a visible `~` escape. ```bash diff --git a/web/submitqueue/controller/gateway-call.ts b/web/submitqueue/controller/gateway-call.ts index e2a2d6c91..2de48fd22 100644 --- a/web/submitqueue/controller/gateway-call.ts +++ b/web/submitqueue/controller/gateway-call.ts @@ -1,6 +1,4 @@ -import { SpanStatusCode, type Meter, type Span, type Tracer } from "@opentelemetry/api"; import type { WebError } from "../entity/index.js"; -import { beginOperation, defaultMeter, defaultTracer } from "../core/observability.js"; import { createLogCallback, type LogCallback } from "@submitqueue/web-platform/logging"; import type { GatewayErrorClassifier, GatewayErrorCode } from "../extension/gateway/index.js"; import { grpcStatusErrorCode } from "../extension/gateway/grpcstatus/index.js"; @@ -13,8 +11,6 @@ export interface GatewayLoadOptions { classifyError?: GatewayErrorClassifier; /** Receives one warning per failed call, with the raw cause. */ onLog?: LogCallback; - meter?: Meter; - tracer?: Tracer; } export type GatewayCall = { ok: true; value: T } | { ok: false; error: WebError }; @@ -29,8 +25,7 @@ function classifyGatewayError(classify: GatewayErrorClassifier, cause: unknown): } /** - * Performs one gateway read inside a span and a `submitqueue_web.gateway` - * duration metric. A failure is classified, logged with its raw cause, and + * Performs one gateway read. A failure is classified, logged with its raw cause, and * returned as a viewer-safe error; this never rejects. */ export async function callGateway( @@ -39,21 +34,11 @@ export async function callGateway( target: { queue: string; sqid?: string }, call: () => Promise, ): Promise> { - const timing = beginOperation(options.meter ?? defaultMeter(), "submitqueue_web.gateway", { operation }); - const tracer = options.tracer ?? defaultTracer(); - return tracer.startActiveSpan(`submitqueue_web.gateway.${operation}`, async (span: Span): Promise> => { - try { - const value = await call(); - timing.end("success"); - return { ok: true, value }; - } catch (cause) { - const code = classifyGatewayError(options.classifyError ?? grpcStatusErrorCode, cause); - timing.end(code); - span.setStatus({ code: SpanStatusCode.ERROR, message: code }); - createLogCallback(options.onLog)("warn", "SubmitQueue gateway call failed", { operation, ...target, code, error: cause }); - return { ok: false, error: webErrorForGatewayCode(code) }; - } finally { - span.end(); - } - }); + try { + return { ok: true, value: await call() }; + } catch (cause) { + const code = classifyGatewayError(options.classifyError ?? grpcStatusErrorCode, cause); + createLogCallback(options.onLog)("warn", "SubmitQueue gateway call failed", { operation, ...target, code, error: cause }); + return { ok: false, error: webErrorForGatewayCode(code) }; + } } diff --git a/web/submitqueue/core/observability.test.ts b/web/submitqueue/core/observability.test.ts deleted file mode 100644 index af935c574..000000000 --- a/web/submitqueue/core/observability.test.ts +++ /dev/null @@ -1,17 +0,0 @@ -import type { Meter } from "@opentelemetry/api"; -import { describe, expect, it, vi } from "vitest"; - -import { beginOperation } from "./observability.js"; - -describe("beginOperation", () => { - it("records one duration tagged with the start attributes and outcome", () => { - const record = vi.fn(); - const createHistogram = vi.fn(() => ({ record })); - const operation = beginOperation({ createHistogram } as unknown as Meter, "web.page_load", { page: "queue" }); - - operation.end("success", { module: "submitqueue" }); - - expect(createHistogram).toHaveBeenCalledWith("web.page_load.duration", expect.objectContaining({ unit: "ms" })); - expect(record).toHaveBeenCalledWith(expect.any(Number), { page: "queue", module: "submitqueue", outcome: "success" }); - }); -}); diff --git a/web/submitqueue/core/observability.ts b/web/submitqueue/core/observability.ts deleted file mode 100644 index 069d4ee42..000000000 --- a/web/submitqueue/core/observability.ts +++ /dev/null @@ -1,38 +0,0 @@ -import { metrics, trace, type Attributes, type Meter, type Tracer } from "@opentelemetry/api"; - -/** Instrumentation scope for meters and tracers the web packages create by default. */ -export const INSTRUMENTATION_SCOPE = "submitqueue-web"; - -/** Latency histogram bucket boundaries in milliseconds, for gateway reads and page loads. */ -export const LATENCY_BUCKETS_MS = [5, 10, 25, 50, 100, 250, 500, 1_000, 2_500, 5_000, 10_000]; - -export function defaultMeter(): Meter { - return metrics.getMeter(INSTRUMENTATION_SCOPE); -} - -export function defaultTracer(): Tracer { - return trace.getTracer(INSTRUMENTATION_SCOPE); -} - -export interface Operation { - /** Records the operation's latency and outcome; call exactly once. */ - end(outcome: string, attributes?: Attributes): void; -} - -/** - * Starts timing one operation. `end` records `.duration` (milliseconds) - * tagged with the start attributes plus `outcome`, the counterpart of the Go - * services' `metrics.Begin`. - */ -export function beginOperation(meter: Meter, name: string, attributes: Attributes = {}): Operation { - const startedAt = performance.now(); - const duration = meter.createHistogram(`${name}.duration`, { - unit: "ms", - advice: { explicitBucketBoundaries: LATENCY_BUCKETS_MS }, - }); - return { - end(outcome, extra = {}) { - duration.record(Math.max(0, performance.now() - startedAt), { ...attributes, ...extra, outcome }); - }, - }; -} diff --git a/web/submitqueue/module.test.ts b/web/submitqueue/module.test.ts index 37241c8c2..20067e9b7 100644 --- a/web/submitqueue/module.test.ts +++ b/web/submitqueue/module.test.ts @@ -1,5 +1,4 @@ import { Code, ConnectError } from "@connectrpc/connect"; -import type { Meter } from "@opentelemetry/api"; import { describe, expect, it, vi } from "vitest"; import { createSubmitQueueWeb, type SubmitQueueWebOptions, type SubmitQueueWebRequest } from "./module.js"; import { createFakeGatewayReader, gatewayHistoryFixture, gatewayRequestFixture } from "./extension/gateway/mock/index.js"; @@ -22,11 +21,6 @@ function newWeb(options: Partial = {}) { web.handle({ path, search, ...extra }); } -function recordingMeter() { - const record = vi.fn(); - return { meter: { createHistogram: () => ({ record }) } as unknown as Meter, record }; -} - function nextPageToken(result: Awaited>>): string { if (result.kind !== "render" || result.model.kind !== "queue" || !result.model.result.ok) { throw new Error("Expected a queue page"); @@ -216,12 +210,6 @@ describe("createSubmitQueueWeb", () => { expect(onLog).toHaveBeenCalledWith("warn", expect.any(String), expect.objectContaining({ requestId: "request-7", operation: "queues", error: cause, code: "unavailable" })); }); - it("records each page load's outcome", async () => { - const { meter, record } = recordingMeter(); - await newWeb({ meter })("/sq/demo-queue", { page: "tampered" }); - expect(record).toHaveBeenCalledWith(expect.any(Number), { page: "queue", outcome: "redirect" }); - }); - it("keeps overlapping loads bound to their own host logging callbacks", async () => { let release!: () => void; const ready = new Promise(resolve => { release = resolve; }); @@ -255,14 +243,12 @@ describe("createSubmitQueueWeb", () => { expect(onLog).toHaveBeenCalledWith("info", "Web page request was not authorized", { requestId: "denied", path: "/sq" }); }); - it("logs, records, and rethrows an unexpected failure", async () => { + it("logs and rethrows an unexpected failure", async () => { const failure = new Error("bug"); const onLog = vi.fn(); - const { meter, record } = recordingMeter(); - const get = newWeb({ gateway: () => { throw failure; }, meter }); + const get = newWeb({ gateway: () => { throw failure; } }); await expect(get("/sq", {}, { onLog })).rejects.toBe(failure); expect(onLog).toHaveBeenCalledWith("error", "Web page load failed", expect.objectContaining({ error: failure })); - expect(record).toHaveBeenCalledWith(expect.any(Number), { page: "queues", outcome: "exception" }); }); it("caches successful catalogs per key until they expire", async () => { diff --git a/web/submitqueue/module.ts b/web/submitqueue/module.ts index 936dc8e8a..24aeca5c2 100644 --- a/web/submitqueue/module.ts +++ b/web/submitqueue/module.ts @@ -1,6 +1,4 @@ -import { SpanStatusCode, type Meter, type Tracer } from "@opentelemetry/api"; import { createLogCallback, type LogCallback } from "@submitqueue/web-platform/logging"; -import { beginOperation, defaultMeter, defaultTracer } from "./core/observability.js"; import { WebPaths } from "./core/paths.js"; import { parseRoute, type SubmitQueueWebRoute, type WebSearchParams } from "./core/route.js"; import { loadChangePage } from "./controller/change.js"; @@ -59,8 +57,6 @@ export interface SubmitQueueWebOptions { classifyError?: GatewayErrorClassifier; /** Caches the queue catalog (`ListQueues`) across loads; absent means every load reads it. */ queuesCache?: QueueCatalogCache; - meter?: Meter; - tracer?: Tracer; now?: () => number; newRequestId?: () => string; } @@ -109,13 +105,9 @@ export function createSubmitQueueWeb(options: SubmitQueueWebOptions): SubmitQueu const paths = options.paths ?? new WebPaths(); const mount = paths.basePath.split("/").filter(Boolean); const now = () => options.now?.() ?? Date.now(); - const meter = options.meter ?? defaultMeter(); - const tracer = options.tracer ?? defaultTracer(); const newRequestId = options.newRequestId ?? (() => globalThis.crypto.randomUUID()); const baseGatewayOptions: GatewayLoadOptions = { ...(options.classifyError ? { classifyError: options.classifyError } : {}), - meter, - tracer, }; const readQueues: QueueCatalogLoader = (gateway, context) => loadQueueDirectory(gateway, { @@ -171,23 +163,12 @@ export function createSubmitQueueWeb(options: SubmitQueueWebOptions): SubmitQueu ...(request.principal === undefined ? {} : { principal: request.principal }), ...(request.signal ? { signal: request.signal } : {}), }; - const operation = beginOperation(meter, "web.page_load", { page: route.kind }); - return tracer.startActiveSpan("web.page_load", { attributes: { "web.page": route.kind } }, async (span): Promise => { - try { - const result = await load(route, context); - operation.end(result.kind); - span.setAttribute("web.outcome", result.kind); - return result; - } catch (cause) { - operation.end("exception"); - span.recordException(cause instanceof Error ? cause : String(cause)); - span.setStatus({ code: SpanStatusCode.ERROR }); - onLog("error", "Web page load failed", { error: cause }); - throw cause; - } finally { - span.end(); - } - }); + try { + return await load(route, context); + } catch (cause) { + onLog("error", "Web page load failed", { error: cause }); + throw cause; + } }, }; } diff --git a/web/submitqueue/package.json b/web/submitqueue/package.json index 9c40f6d60..9e693cc7e 100644 --- a/web/submitqueue/package.json +++ b/web/submitqueue/package.json @@ -36,12 +36,10 @@ "typecheck": "tsc --noEmit" }, "peerDependencies": { - "@opentelemetry/api": ">=1.9 <2", "react": ">=18 <20" }, "devDependencies": { "@connectrpc/connect": "^2.1.1", - "@opentelemetry/api": "^1.9.0", "@testing-library/jest-dom": "^6.6.3", "@testing-library/react": "^16.3.0", "@types/node": "22.18.6", diff --git a/web/test/package/BUILD.bazel b/web/test/package/BUILD.bazel index 5ad2fbcda..5a9785e31 100644 --- a/web/test/package/BUILD.bazel +++ b/web/test/package/BUILD.bazel @@ -31,7 +31,6 @@ ts_project( ":node_modules/@submitqueue/web-platform", "//web:node_modules/@bufbuild/protobuf", "//web:node_modules/@connectrpc/connect", - "//web:node_modules/@opentelemetry/api", "//web:node_modules/@types/node", "//web:node_modules/react", "//web:node_modules/react-dom", @@ -56,7 +55,6 @@ js_test( "//web/submitqueue:node_modules", "//web:node_modules/@bufbuild/protobuf", "//web:node_modules/@connectrpc/connect", - "//web:node_modules/@opentelemetry/api", "//web:node_modules/react", "//web:node_modules/react-dom", ], diff --git a/web/test/package/pack_contents_test.py b/web/test/package/pack_contents_test.py index 48bb09232..940b27156 100644 --- a/web/test/package/pack_contents_test.py +++ b/web/test/package/pack_contents_test.py @@ -8,7 +8,7 @@ # The module reads the gateway through a structural contract; the transport, # generated protos, and web framework belong to the host. -_FORBIDDEN_IMPORTS = ("@submitqueue/api", "@submitqueue/web-submitqueue", "@connectrpc/", "@bufbuild/", "next", "pino", "fusion-core", "fusion-tokens") +_FORBIDDEN_IMPORTS = ("@submitqueue/api", "@submitqueue/web-submitqueue", "@connectrpc/", "@bufbuild/", "@opentelemetry/", "next", "pino", "fusion-core", "fusion-tokens") # Modules the client-safe root entry must never reach, directly or transitively. _CLIENT_FORBIDDEN_MODULES = ("package/dist/controller/", "package/dist/module.js", "package/dist/server.js") @@ -81,7 +81,7 @@ def main() -> int: for group in ("peerDependencies", "dependencies", "optionalDependencies") for name in package.get(group, {}) ): - print("The core module must not require framework, transport, or logging dependencies.", file=sys.stderr) + print("The core module must not require framework, transport, logging, or telemetry-backend dependencies.", file=sys.stderr) return 1 stylesheet = package.get("exports", {}).get("./styles.css") if stylesheet != "./view/styles.css" or stylesheet not in package.get("sideEffects", []): diff --git a/web/test/package/react18/BUILD.bazel b/web/test/package/react18/BUILD.bazel index c0a3e920d..e02e51008 100644 --- a/web/test/package/react18/BUILD.bazel +++ b/web/test/package/react18/BUILD.bazel @@ -27,7 +27,6 @@ js_test( ":node_modules/@submitqueue/web-submitqueue", "//web:node_modules/@bufbuild/protobuf", "//web:node_modules/@connectrpc/connect", - "//web:node_modules/@opentelemetry/api", ], entry_point = "react18-runtime.mjs", ) diff --git a/web/tool/library.bzl b/web/tool/library.bzl index 4e876f5bd..a91bc7621 100644 --- a/web/tool/library.bzl +++ b/web/tool/library.bzl @@ -63,7 +63,6 @@ def web_library(package, vitest, eslint, deps, test_deps = [], assets = []): "//web:node_modules/react-dom", # jest-dom resolves its vitest peer from the workspace root. "//web:node_modules/vitest", - "//web:node_modules/@opentelemetry/api", ] + deps, ) eslint.eslint_test(