fix(webapp,clickhouse): stop invalid customer queries alerting, and isolate Sentry scope per request

Three fixes to how query failures are reported.

Invalid TSQL is a caller mistake, not ours: executeTSQL now logs
ExposedTSQLError at warn and reserves error for InternalTSQLError and
unanticipated exceptions, so a bad column name no longer raises an alert.
The route above it already returned 400 and logged at warn; the layer
below was overriding that decision.

ClickHouse rejections that come from a query asking for too much (memory
ceiling, timeout, row/byte caps) drop to warn as well. Those are decided
in the client, which is the only place holding the parsed ClickHouseError
type, and queryWithStats gained a logFields option so a failing query is
recorded with the TSQL that generated it rather than the generated SQL
alone.

Sentry.init runs with skipOpenTelemetrySetup because we register our own
OTel pipeline, which also skipped installing SentryContextManager.
withIsolationScope only marks the context and relies on that manager to
fork, so without it every request shared one global isolation scope and
events were attributed to whichever request wrote last. The tracer now
registers it, including on the path where tracing is disabled and
register() was never called.

Author: ericallam

.server-changes/sentry-request-attribution.md

@@ -0,0 +1,6 @@
+---
+area: webapp
+type: fix
+---
+
+Fixed error reports being attributed to the wrong request when several requests were in flight at once.

.server-changes/tsql-query-error-log-levels.md

@@ -0,0 +1,6 @@
+---
+area: webapp
+type: fix
+---
+
+Invalid queries sent to the query API are no longer treated as internal errors, and a query that does fail is now recorded together with the query text that produced it.

apps/webapp/app/v3/tracer.server.ts

@@ -1,6 +1,7 @@
 import {
   type Attributes,
   type Context,
+  context as otelContext,
   createContextKey,
   DiagConsoleLogger,
   DiagLogLevel,
@@ -14,6 +15,7 @@ import {
   metrics,
   type Meter,
 } from "@opentelemetry/api";
+import { SentryContextManager } from "@sentry/remix";
 import { logs, SeverityNumber } from "@opentelemetry/api-logs";
 import { OTLPTraceExporter } from "@opentelemetry/exporter-trace-otlp-http";
 import { OTLPLogExporter } from "@opentelemetry/exporter-logs-otlp-http";
@@ -209,10 +211,28 @@ function getResource() {
   return baseResource.merge(detectedResource);
 }
 
+/**
+ * Sentry's `withIsolationScope` only marks the OTel context; the fork itself is
+ * done by Sentry's context manager. We pass `skipOpenTelemetrySetup: true` to
+ * `Sentry.init` because we run our own OTel pipeline, which also skips the
+ * `setGlobalContextManager(new SentryContextManager())` that Sentry would
+ * otherwise do. Registering it here is what keeps per-request scopes (and so
+ * the request attributed to each Sentry event) from leaking between concurrent
+ * requests. It extends `AsyncLocalStorageContextManager`, so OTel behaviour is
+ * unchanged.
+ */
+function createContextManager() {
+  return new SentryContextManager();
+}
+
 function setupTelemetry() {
   if (env.INTERNAL_OTEL_TRACE_DISABLED === "1") {
     console.log(`🔦 Tracer disabled, returning a noop tracer`);
 
+    const contextManager = createContextManager();
+    contextManager.enable();
+    otelContext.setGlobalContextManager(contextManager);
+
     return {
       tracer: trace.getTracer("trigger.dev", "3.3.12"),
       logger: logs.getLogger("trigger.dev", "3.3.12"),
@@ -300,7 +320,7 @@ function setupTelemetry() {
     );
   }
 
-  provider.register();
+  provider.register({ contextManager: createContextManager() });
 
   let instrumentations: Instrumentation[] = [
     new AwsSdkInstrumentation({

apps/webapp/test/sentryRequestIsolation.test.ts

@@ -0,0 +1,51 @@
+import { context } from "@opentelemetry/api";
+import { NodeTracerProvider } from "@opentelemetry/sdk-trace-node";
+import * as Sentry from "@sentry/remix";
+import { SentryContextManager } from "@sentry/remix";
+import { afterEach, beforeAll, describe, expect, it } from "vitest";
+
+/**
+ * Two overlapping requests, each tagging its own isolation scope, mirroring what
+ * `SentryHttpInstrumentation` does per incoming request. Returns what each one
+ * reads back after the other has started.
+ */
+async function raceTwoRequests(): Promise<Record<string, unknown>> {
+  const observed: Record<string, unknown> = {};
+
+  const handleRequest = (name: string, holdMs: number) =>
+    Sentry.withIsolationScope(async () => {
+      Sentry.getIsolationScope().setTag("request", name);
+      await new Promise((resolve) => setTimeout(resolve, holdMs));
+      observed[name] = Sentry.getIsolationScope().getScopeData().tags.request;
+    });
+
+  await Promise.all([handleRequest("slow", 30), handleRequest("fast", 5)]);
+
+  return observed;
+}
+
+describe("Sentry request isolation", () => {
+  beforeAll(() => {
+    Sentry.init({ dsn: undefined, defaultIntegrations: false, skipOpenTelemetrySetup: true });
+  });
+
+  afterEach(() => {
+    context.disable();
+  });
+
+  it("leaks the isolation scope between concurrent requests without SentryContextManager", async () => {
+    new NodeTracerProvider().register();
+
+    const observed = await raceTwoRequests();
+
+    expect(observed).toEqual({ slow: "fast", fast: "fast" });
+  });
+
+  it("keeps each request's isolation scope separate with SentryContextManager", async () => {
+    new NodeTracerProvider().register({ contextManager: new SentryContextManager() });
+
+    const observed = await raceTwoRequests();
+
+    expect(observed).toEqual({ slow: "slow", fast: "fast" });
+  });
+});

internal-packages/clickhouse/src/client/client.ts

@@ -171,13 +171,19 @@ export class ClickhouseClient implements ClickhouseReader, ClickhouseWriter {
         );
 
         if (clickhouseError) {
-          this.logger.error("Error querying clickhouse", {
+          const errorLogFields = {
             name: req.name,
             error: clickhouseError,
             query: req.query,
             params,
             queryId,
-          });
+          };
+
+          if (isClickhouseQuotaError(clickhouseError)) {
+            this.logger.warn("Query exceeded a ClickHouse limit", errorLogFields);
+          } else {
+            this.logger.error("Error querying clickhouse", errorLogFields);
+          }
 
           recordClickhouseError(span, clickhouseError);
 
@@ -260,6 +266,11 @@ export class ClickhouseClient implements ClickhouseReader, ClickhouseWriter {
      * These will be merged with the default settings.
      */
     settings?: ClickHouseSettings;
+    /**
+     * Extra fields to attach to the error log if the query fails. Use this to
+     * record what produced the SQL, e.g. the TSQL a caller actually wrote.
+     */
+    logFields?: Record<string, unknown>;
   }): ClickhouseQueryWithStatsFunction<z.input<TIn>, z.output<TOut>> {
     return async (params, options) => {
       const queryId = randomUUID();
@@ -320,13 +331,20 @@ export class ClickhouseClient implements ClickhouseReader, ClickhouseWriter {
         );
 
         if (clickhouseError) {
-          this.logger.error("Error querying clickhouse", {
+          const errorLogFields = {
             name: req.name,
             error: clickhouseError,
             query: req.query,
             params,
             queryId,
-          });
+            ...req.logFields,
+          };
+
+          if (isClickhouseQuotaError(clickhouseError)) {
+            this.logger.warn("Query exceeded a ClickHouse limit", errorLogFields);
+          } else {
+            this.logger.error("Error querying clickhouse", errorLogFields);
+          }
 
           recordClickhouseError(span, clickhouseError);
 
@@ -453,13 +471,19 @@ export class ClickhouseClient implements ClickhouseReader, ClickhouseWriter {
         );
 
         if (clickhouseError) {
-          this.logger.error("Error querying clickhouse", {
+          const errorLogFields = {
             name: req.name,
             error: clickhouseError,
             query: req.query,
             params,
             queryId,
-          });
+          };
+
+          if (isClickhouseQuotaError(clickhouseError)) {
+            this.logger.warn("Query exceeded a ClickHouse limit", errorLogFields);
+          } else {
+            this.logger.error("Error querying clickhouse", errorLogFields);
+          }
 
           recordClickhouseError(span, clickhouseError);
 
@@ -1001,6 +1025,29 @@ export class ClickhouseClient implements ClickhouseReader, ClickhouseWriter {
   }
 }
 
+/**
+ * ClickHouse error types raised by a query that is valid but asks for more than
+ * the caller is allowed to spend. The caller gets a 4xx and there is nothing on
+ * our side to fix, so these are logged at warn rather than error.
+ */
+const CLICKHOUSE_QUOTA_ERROR_TYPES = new Set([
+  "MEMORY_LIMIT_EXCEEDED",
+  "TIMEOUT_EXCEEDED",
+  "TOO_SLOW",
+  "TOO_MANY_ROWS",
+  "TOO_MANY_BYTES",
+  "TOO_MANY_ROWS_OR_BYTES",
+  "QUERY_WAS_CANCELLED",
+]);
+
+function isClickhouseQuotaError(error: Error): boolean {
+  return (
+    error instanceof ClickHouseError &&
+    error.type !== undefined &&
+    CLICKHOUSE_QUOTA_ERROR_TYPES.has(error.type)
+  );
+}
+
 function recordClickhouseError(span: Span, error: Error): void {
   if (error instanceof ClickHouseError) {
     span.setAttributes({

internal-packages/clickhouse/src/client/tsql.ts

@@ -8,6 +8,7 @@
 import type { ClickHouseSettings } from "@clickhouse/client";
 import {
   compileTSQL,
+  ExposedTSQLError,
   type OutputColumnMetadata,
   sanitizeErrorMessage,
   transformResults,
@@ -207,6 +208,7 @@ export async function executeTSQL<TOut extends z.ZodSchema>(
       // EXPLAIN returns rows with an 'explain' column
       schema: isExplain ? z.object({ explain: z.string() }) : options.schema,
       settings: options.clickhouseSettings,
+      logFields: { tsql: options.query },
     });
 
     const [error, result] = await queryFn(params);
@@ -297,14 +299,21 @@ export async function executeTSQL<TOut extends z.ZodSchema>(
   } catch (error) {
     const errorMessage = error instanceof Error ? error.message : "Unknown error";
 
-    // Log TSQL compilation or unexpected errors (with original message for debugging)
-    logger.error("[TSQL] Query error", {
+    const logFields = {
       name: options.name,
       error: errorMessage,
       tsql: options.query,
       generatedSql: generatedSql ?? "(compilation failed)",
       generatedParams: generatedParams ?? {},
-    });
+    };
+
+    const callerWroteABadQuery = error instanceof ExposedTSQLError;
+
+    if (callerWroteABadQuery) {
+      logger.warn("[TSQL] Invalid query", logFields);
+    } else {
+      logger.error("[TSQL] Query error", logFields);
+    }
 
     // Sanitize error message to show TSQL names instead of ClickHouse internals
     const sanitizedMessage = sanitizeErrorMessage(errorMessage, options.tableSchema);

internal-packages/clickhouse/src/client/types.ts

@@ -135,6 +135,11 @@ export interface ClickhouseReader {
      * These will be merged with the default settings.
      */
     settings?: ClickHouseSettings;
+    /**
+     * Extra fields to attach to the error log if the query fails. Use this to
+     * record what produced the SQL, e.g. the TSQL a caller actually wrote.
+     */
+    logFields?: Record<string, unknown>;
   }): ClickhouseQueryWithStatsFunction<z.input<TIn>, z.output<TOut>>;
 
   queryFast<TOut extends Record<string, any>, TParams extends Record<string, any>>(req: {