diff --git a/.env.example b/.env.example index 748b79164c..39e570ee4d 100644 --- a/.env.example +++ b/.env.example @@ -230,7 +230,13 @@ REDIS_URL=redis://redis:6379 # REQUIRED for the self-host review # --- Sentry error tracking (optional) --- # SENTRY_DSN= # enables self-host Sentry capture; unset = complete no-op # SENTRY_ENVIRONMENT=production -# SENTRY_TRACES_SAMPLE_RATE=0 # traces are off by default; errors still report +# SENTRY_TRACES_SAMPLE_RATE=0 # traces/spans are off by default; errors still report. Set a LOW rate +# # (e.g. 0.05) with SENTRY_DSN to sample review tracing: each sampled +# # review emits a connected trace — the queue-job span (whole-review +# # latency) with the AI-provider span nested — so you can filter slow +# # or failed STAGES in Sentry without reading scattered logs. Spans +# # carry only safe dimensions (repo, job type, provider/model); never +# # prompts, diffs, tokens, or bodies. Leave 0 to keep tracing a no-op. # SENTRY_RELEASE= # custom images only: set this ONLY when you uploaded source maps for # # the exact built bundle under this exact release id. Future official # # images bake GITTENSORY_VERSION=gittensory-selfhost@, so do diff --git a/src/selfhost/ai.ts b/src/selfhost/ai.ts index 77c67b0068..062bbbad56 100644 --- a/src/selfhost/ai.ts +++ b/src/selfhost/ai.ts @@ -10,7 +10,7 @@ import type { CombineStrategy, OnMerge } from "../services/ai-review"; import { isConfiguredSelfHostProvider, resolveConfiguredProviderNames } from "./ai-config"; export { assertNoLegacySharedAiEnv } from "./ai-config"; import { incr } from "./metrics"; -import { withOtelSpan } from "./otel"; +import { withReviewSpan } from "./tracing"; import { delimiter } from "node:path"; interface AiRunOptions { @@ -627,7 +627,7 @@ function runProviderWithOtel( model: string, options: AiRunOptions, ): Promise { - return withOtelSpan( + return withReviewSpan( "selfhost.ai.provider", { "ai.provider": provider.name, "ai.model": model || "default", "ai.request_kind": requestKind(options) }, () => provider.ai.run(model, options), diff --git a/src/selfhost/pg-queue.ts b/src/selfhost/pg-queue.ts index a467a1ca20..3106a5974d 100644 --- a/src/selfhost/pg-queue.ts +++ b/src/selfhost/pg-queue.ts @@ -5,7 +5,7 @@ import type { Pool } from "pg"; import { logAudit, extractPayloadType } from "./audit"; import { incr } from "./metrics"; -import { withOtelSpan } from "./otel"; +import { withReviewSpan } from "./tracing"; import { captureError } from "./sentry"; import { consumingRetryDelayMs, @@ -370,7 +370,7 @@ export function createPgQueue( return true; } try { - await withOtelSpan( + await withReviewSpan( "selfhost.queue.job", { "job.type": message.type, "queue.backend": "postgres", "job.attempt": Number(job.attempts) + 1 }, () => consume(message), diff --git a/src/selfhost/sentry.ts b/src/selfhost/sentry.ts index ecd5957e55..e6fa484b8b 100644 --- a/src/selfhost/sentry.ts +++ b/src/selfhost/sentry.ts @@ -14,6 +14,9 @@ type SentryScope = { let Sentry: SentryNs | undefined; let active = false; let sentryEnvironment = "production"; +// The resolved tracing sample rate. Tracing stays a complete no-op (no spans started, no trace traffic) until this +// is configured above 0 — distinct from error capture, which is on whenever the DSN is set. (#1734) +let tracesSampleRate = 0; const SECRET_KEY = /(token|secret|key|password|passwd|authorization|auth|dsn|cookie|bearer|credential|private)/i; @@ -104,6 +107,14 @@ export function resolveSentryRelease( return nonBlank(env.SENTRY_RELEASE) ?? nonBlank(env.GITTENSORY_VERSION); } +/** Resolve the trace sample rate, clamped to [0, 1]. Defaults to 0 (tracing off) — a malformed value is treated as + * off rather than full sampling, so a typo can never accidentally flood the tracer. (#1734) */ +export function resolveTracesSampleRate(env: NodeJS.ProcessEnv): number { + const parsed = Number(env.SENTRY_TRACES_SAMPLE_RATE ?? "0"); + if (!Number.isFinite(parsed)) return 0; + return Math.min(1, Math.max(0, parsed)); +} + /** beforeSend scrubber — redact anything token/secret-like before an event leaves the box (privacy boundary). */ export function scrubEvent(event: T): T { const redact = (obj: unknown, depth: number): void => { @@ -135,11 +146,12 @@ export async function initSentry(env: NodeJS.ProcessEnv): Promise { Sentry = await import("@sentry/node"); const release = resolveSentryRelease(env); sentryEnvironment = nonBlank(env.SENTRY_ENVIRONMENT) ?? "production"; + tracesSampleRate = resolveTracesSampleRate(env); Sentry.init({ dsn: env.SENTRY_DSN, environment: sentryEnvironment, ...(release ? { release } : {}), - tracesSampleRate: Number(env.SENTRY_TRACES_SAMPLE_RATE ?? "0"), + tracesSampleRate, serverName: env.PUBLIC_API_ORIGIN, beforeSend: (e) => scrubEvent(e), }); @@ -186,6 +198,42 @@ export function captureReviewFailure( }); } +/** True only when error capture is active AND trace sampling is configured above 0. When false, every span helper + * is a complete no-op — no span is started and no trace traffic is emitted (the #1734 "sampling off" guarantee). */ +export function sentryTracingEnabled(): boolean { + return active && Sentry !== undefined && tracesSampleRate > 0; +} + +/** Project an attribute bag onto the safe, low-cardinality subset allowed on a span: drop secret-keyed keys and + * null/undefined, keep finite numbers + booleans, and truncate strings — never a prompt/diff/token/body. */ +export function sentrySpanAttributes( + input: Record | undefined, +): Record { + const out: Record = {}; + if (!input) return out; + for (const [key, value] of Object.entries(input)) { + if (SECRET_KEY.test(key) || value === null || value === undefined) continue; + if (typeof value === "string") out[key] = value.length > 160 ? `${value.slice(0, 157)}...` : value; + else if (typeof value === "number" && Number.isFinite(value)) out[key] = value; + else if (typeof value === "boolean") out[key] = value; + } + return out; +} + +/** Run `fn` inside a Sentry span named `name`, tagged with the safe attributes. The span auto-closes and is marked + * errored if `fn` throws (so slow/failed stages are filterable). A pure pass-through to `fn` when tracing is off. */ +export async function withSentrySpan( + name: string, + attributes: Record | undefined, + fn: () => T | Promise, +): Promise { + if (!sentryTracingEnabled()) return fn(); + return Sentry!.startSpan( + { name, op: name, attributes: sentrySpanAttributes(attributes) }, + () => fn(), + ); +} + // The structured-log fields worth indexing as Sentry tags — the dimensions operators filter + group by. Only // string|number values are tagged; everything else stays in the full "log" context. const SENTRY_LOG_TAG_KEYS = ["repo", "repository", "installationId", "installation_id", "pull", "pullNumber", "pr", "project", "kind", "deliveryId", "provider", "model", "effort", "timeoutMs", "trace_id", "span_id"] as const; @@ -364,6 +412,7 @@ export function resetSentryForTest(): void { Sentry = undefined; active = false; sentryEnvironment = "production"; + tracesSampleRate = 0; } interface StructuredLogConsole { diff --git a/src/selfhost/sqlite-queue.ts b/src/selfhost/sqlite-queue.ts index 008c10b818..779ec44d86 100644 --- a/src/selfhost/sqlite-queue.ts +++ b/src/selfhost/sqlite-queue.ts @@ -6,7 +6,7 @@ import type { SqliteDriver } from "./d1-adapter"; import { logAudit, extractPayloadType } from "./audit"; import { incr } from "./metrics"; -import { withOtelSpan } from "./otel"; +import { withReviewSpan } from "./tracing"; import { captureError } from "./sentry"; import { consumingRetryDelayMs, @@ -313,7 +313,7 @@ export function createSqliteQueue( return true; } try { - await withOtelSpan( + await withReviewSpan( "selfhost.queue.job", { "job.type": message.type, "queue.backend": "sqlite", "job.attempt": job.attempts + 1 }, () => consume(message), diff --git a/src/selfhost/tracing.ts b/src/selfhost/tracing.ts new file mode 100644 index 0000000000..53ec2c64f2 --- /dev/null +++ b/src/selfhost/tracing.ts @@ -0,0 +1,16 @@ +// Shared review-pipeline span wrapper (#1734). Opens ONE boundary that feeds BOTH tracers — an OpenTelemetry span +// and a Sentry span — so a stage is instrumented once and shows up in whichever backend is enabled. Kept in this +// neutral module (rather than inside otel.ts or sentry.ts) so a caller wires the normal review boundary without +// importing from, or coupling to, either tracer's module. Each side independently no-ops when its backend is off, +// so this reduces to `fn()` when neither is configured. +import { withOtelSpan } from "./otel"; +import { withSentrySpan } from "./sentry"; + +export async function withReviewSpan( + name: string, + attributes: Record | undefined, + fn: () => T | Promise, + options?: { parentTraceParent?: string | undefined }, +): Promise { + return withOtelSpan(name, attributes, () => withSentrySpan(name, attributes, fn), options); +} diff --git a/test/unit/selfhost-sentry.test.ts b/test/unit/selfhost-sentry.test.ts index a027241de5..93949885fe 100644 --- a/test/unit/selfhost-sentry.test.ts +++ b/test/unit/selfhost-sentry.test.ts @@ -11,6 +11,8 @@ const mocks = vi.hoisted(() => { captureMessage: vi.fn(), captureCheckIn: vi.fn((checkIn: { checkInId?: string }) => checkIn.checkInId ?? "check-in-id"), flush: vi.fn().mockResolvedValue(true), + // Mirror @sentry/node's startSpan contract: invoke the callback inside the span and return its value. + startSpan: vi.fn((_opts: unknown, cb: () => T): T => cb()), }; }); const otelMocks = vi.hoisted(() => ({ @@ -23,6 +25,7 @@ vi.mock("@sentry/node", () => ({ captureMessage: mocks.captureMessage, captureCheckIn: mocks.captureCheckIn, flush: mocks.flush, + startSpan: mocks.startSpan, })); vi.mock("../../src/selfhost/otel", () => ({ currentOtelTraceIds: otelMocks.currentOtelTraceIds, @@ -37,9 +40,13 @@ import { installStructuredLogForwarding, resolveSentryRelease, resolveSentryMonitorSlug, + resolveTracesSampleRate, scrubEvent, resetSentryForTest, + sentryTracingEnabled, + sentrySpanAttributes, withSentryMonitor, + withSentrySpan, } from "../../src/selfhost/sentry"; beforeEach(() => { @@ -698,3 +705,82 @@ describe("installStructuredLogForwarding — central console sink instrumentatio expect(base.error).toHaveBeenCalledTimes(2); }); }); + +const DSN = "https://k@o.ingest/1"; +const asEnv = (e: Record) => e as unknown as NodeJS.ProcessEnv; + +describe("resolveTracesSampleRate — opt-in, clamped, safe default (#1734)", () => { + it("defaults to 0, parses a valid rate, clamps to [0,1], and treats a non-finite value as 0", () => { + expect(resolveTracesSampleRate(asEnv({}))).toBe(0); + expect(resolveTracesSampleRate(asEnv({ SENTRY_TRACES_SAMPLE_RATE: "0.25" }))).toBe(0.25); + expect(resolveTracesSampleRate(asEnv({ SENTRY_TRACES_SAMPLE_RATE: "5" }))).toBe(1); + expect(resolveTracesSampleRate(asEnv({ SENTRY_TRACES_SAMPLE_RATE: "-2" }))).toBe(0); + expect(resolveTracesSampleRate(asEnv({ SENTRY_TRACES_SAMPLE_RATE: "abc" }))).toBe(0); + }); +}); + +describe("sentrySpanAttributes — safe, low-cardinality only", () => { + it("drops secret-keyed and null/undefined keys, keeps scalars, truncates long strings", () => { + const out = sentrySpanAttributes({ + "ai.model": "gpt", + "job.attempt": 2, + ok: true, + apiKey: "shh", + token: "x", + missing: null, + undef: undefined, + nan: Number.NaN, // a non-finite number is dropped, never tagged + nested: { a: 1 }, // a non-scalar is dropped (no unbounded blobs on a span) + long: "z".repeat(200), + }); + expect(out).toEqual({ + "ai.model": "gpt", + "job.attempt": 2, + ok: true, + long: `${"z".repeat(157)}...`, + }); + }); + + it("returns an empty object for undefined input", () => { + expect(sentrySpanAttributes(undefined)).toEqual({}); + }); +}); + +describe("tracing is a complete no-op unless sampling is configured > 0 (#1734)", () => { + it("with Sentry off, withSentrySpan runs fn but starts NO span and reports tracing disabled", async () => { + expect(sentryTracingEnabled()).toBe(false); + expect(await withSentrySpan("s", { a: 1 }, async () => "r")).toBe("r"); + expect(mocks.startSpan).not.toHaveBeenCalled(); + }); + + it("with the DSN set but sample rate 0 (default), tracing stays off and starts no span", async () => { + await initSentry(asEnv({ SENTRY_DSN: DSN })); // no SENTRY_TRACES_SAMPLE_RATE → 0 + expect(sentryTracingEnabled()).toBe(false); + await withSentrySpan("s", undefined, async () => "r"); + expect(mocks.startSpan).not.toHaveBeenCalled(); + }); +}); + +describe("tracing emits spans when sampling is enabled (#1734)", () => { + beforeEach(async () => { + await initSentry(asEnv({ SENTRY_DSN: DSN, SENTRY_TRACES_SAMPLE_RATE: "1" })); + }); + + it("starts a named span tagged with safe attributes and returns fn's value", async () => { + expect(sentryTracingEnabled()).toBe(true); + const result = await withSentrySpan("selfhost.ai.provider", { "ai.model": "gpt", apiKey: "shh" }, async () => 42); + expect(result).toBe(42); + expect(mocks.startSpan).toHaveBeenCalledTimes(1); + const [opts] = mocks.startSpan.mock.calls[0]!; + expect(opts).toMatchObject({ name: "selfhost.ai.provider", op: "selfhost.ai.provider" }); + expect((opts as { attributes: Record }).attributes).toEqual({ "ai.model": "gpt" }); // secret dropped + }); + + it("propagates an error thrown by fn (the caller's error is never swallowed by the span wrapper)", async () => { + await expect( + withSentrySpan("selfhost.queue.job", undefined, async () => { + throw new Error("boom"); + }), + ).rejects.toThrow("boom"); + }); +}); diff --git a/test/unit/selfhost-tracing.test.ts b/test/unit/selfhost-tracing.test.ts new file mode 100644 index 0000000000..6b7bfee4f6 --- /dev/null +++ b/test/unit/selfhost-tracing.test.ts @@ -0,0 +1,41 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +// withReviewSpan composes the two tracer wrappers; both are mocked here as passthroughs so we assert the +// composition (one boundary → both tracers, option forwarding, value pass-through) without any real SDK. +const otel = vi.hoisted(() => ({ + withOtelSpan: vi.fn( + (_name: string, _attrs: unknown, fn: () => T | Promise, _options?: unknown): T | Promise => fn(), + ), +})); +const sentry = vi.hoisted(() => ({ + withSentrySpan: vi.fn( + (_name: string, _attrs: unknown, fn: () => T | Promise): T | Promise => fn(), + ), +})); +vi.mock("../../src/selfhost/otel", () => ({ withOtelSpan: otel.withOtelSpan })); +vi.mock("../../src/selfhost/sentry", () => ({ withSentrySpan: sentry.withSentrySpan })); + +import { withReviewSpan } from "../../src/selfhost/tracing"; + +beforeEach(() => vi.clearAllMocks()); + +describe("withReviewSpan — one boundary feeds both tracers (#1734)", () => { + it("runs fn through the OTEL span wrapping the Sentry span, and returns fn's value", async () => { + const result = await withReviewSpan("selfhost.queue.job", { "job.type": "github-webhook" }, async () => "ok"); + expect(result).toBe("ok"); + expect(otel.withOtelSpan).toHaveBeenCalledTimes(1); + expect(sentry.withSentrySpan).toHaveBeenCalledTimes(1); + // Same span name + attributes are handed to both tracers. + expect(otel.withOtelSpan.mock.calls[0]![0]).toBe("selfhost.queue.job"); + expect(sentry.withSentrySpan.mock.calls[0]![0]).toBe("selfhost.queue.job"); + }); + + it("forwards the parentTraceParent option to the OTEL span (cross-job trace continuity)", async () => { + await withReviewSpan("n", undefined, async () => 1, { parentTraceParent: "00-trace-span-01" }); + expect(otel.withOtelSpan.mock.calls[0]![3]).toEqual({ parentTraceParent: "00-trace-span-01" }); + }); + + it("reduces to fn() when both tracer wrappers no-op (neither backend configured)", async () => { + expect(await withReviewSpan("n", undefined, async () => 42)).toBe(42); + }); +});