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..5e25815dcf 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 "./sentry"; 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..acbe34f8f9 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 "./sentry"; 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..84507f6f4c 100644 --- a/src/selfhost/sentry.ts +++ b/src/selfhost/sentry.ts @@ -2,7 +2,7 @@ // env-gated, dynamically-imported selfhost-integration pattern (Redis/Qdrant/embed-provider in server.ts). // @sentry/node is NEVER imported at module top level — it loads lazily inside initSentry(), so it never enters // the Worker bundle (src/index.ts) and cloudflare:* stubbing stays clean. All helpers are safe to call when off. -import { currentOtelTraceIds } from "./otel"; +import { currentOtelTraceIds, withOtelSpan } from "./otel"; type SentryNs = typeof import("@sentry/node"); type SentryMonitorConfig = NonNullable[1]>; @@ -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,54 @@ 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 shared review-pipeline span wrapper: open ONE boundary that feeds BOTH tracers — an OpenTelemetry span and a + * Sentry span — so instrumentation is consistent and the same stage shows up in whichever backend is enabled. Each + * side independently no-ops when its backend is off, so this reduces to `fn()` when neither is configured. (#1734) */ +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); +} + // 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 +424,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..87977fdfbc 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 "./sentry"; 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/test/unit/selfhost-sentry.test.ts b/test/unit/selfhost-sentry.test.ts index a027241de5..c817af40d2 100644 --- a/test/unit/selfhost-sentry.test.ts +++ b/test/unit/selfhost-sentry.test.ts @@ -11,10 +11,14 @@ 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(() => ({ currentOtelTraceIds: vi.fn(), + // withOtelSpan no-ops to its callback here (OTEL off) — so withReviewSpan exercises only the Sentry side. + withOtelSpan: vi.fn((_name: string, _attrs: unknown, fn: () => T): T => fn()), })); vi.mock("@sentry/node", () => ({ init: mocks.init, @@ -23,9 +27,11 @@ vi.mock("@sentry/node", () => ({ captureMessage: mocks.captureMessage, captureCheckIn: mocks.captureCheckIn, flush: mocks.flush, + startSpan: mocks.startSpan, })); vi.mock("../../src/selfhost/otel", () => ({ currentOtelTraceIds: otelMocks.currentOtelTraceIds, + withOtelSpan: otelMocks.withOtelSpan, })); import { @@ -37,9 +43,14 @@ import { installStructuredLogForwarding, resolveSentryRelease, resolveSentryMonitorSlug, + resolveTracesSampleRate, scrubEvent, resetSentryForTest, + sentryTracingEnabled, + sentrySpanAttributes, + withReviewSpan, withSentryMonitor, + withSentrySpan, } from "../../src/selfhost/sentry"; beforeEach(() => { @@ -698,3 +709,93 @@ 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, the span helpers run fn but start NO span and report tracing disabled", async () => { + expect(sentryTracingEnabled()).toBe(false); + expect(await withSentrySpan("s", { a: 1 }, async () => "r")).toBe("r"); + expect(await withReviewSpan("s", { a: 1 }, async () => "r2")).toBe("r2"); + 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"); + await withReviewSpan("s", undefined, async () => "r"); + expect(mocks.startSpan).not.toHaveBeenCalled(); + // withReviewSpan still runs the OTEL side (its own no-op here), so the boundary is preserved. + expect(otelMocks.withOtelSpan).toHaveBeenCalledTimes(1); + }); +}); + +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("withReviewSpan drives BOTH the OTEL and the Sentry span for one boundary", async () => { + const result = await withReviewSpan("selfhost.queue.job", { "job.type": "github-webhook" }, async () => "ok"); + expect(result).toBe("ok"); + expect(otelMocks.withOtelSpan).toHaveBeenCalledTimes(1); + expect(mocks.startSpan).toHaveBeenCalledTimes(1); + }); + + it("propagates an error thrown by fn so the span is recorded as failed", async () => { + await expect( + withSentrySpan("selfhost.queue.job", undefined, async () => { + throw new Error("boom"); + }), + ).rejects.toThrow("boom"); + }); +});