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

.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/components/dashboard-agent/AgentChart.tsx

@@ -67,6 +67,7 @@ export function AgentChart({ block }: { block: ChartBlock }) {
  period: block.period ?? null,
  from: null,
  to: null,
+ userAuthoredQuery: true,
  }),
  signal: controller.signal,
  })

apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx

@@ -183,6 +183,7 @@ export const action = async ({ request, params }: ActionFunctionArgs) => {
  const queryResult = await executeQuery({
  name: "query-page",
  query,
+ userAuthoredQuery: true,
  scope,
  organizationId: project.organizationId,
  projectId: project.id,

apps/webapp/app/routes/api.v1.query.ts

@@ -54,6 +54,7 @@ const { action, loader } = createActionApiRoute(
  const queryResult = await executeQuery({
  name: "api-query",
  query,
+ userAuthoredQuery: true,
  scope: scope as QueryScope,
  organizationId: env.organization.id,
  projectId: env.project.id,

apps/webapp/app/routes/resources.metric.tsx

@@ -52,6 +52,7 @@ const MetricWidgetQuery = z.object({
  tags: z.array(z.string()).optional(),
  // Opt into server-side gap fill (carry-forward for gauges, zero-fill for counters).
  fillGaps: z.boolean().optional(),
+ userAuthoredQuery: z.boolean().optional(),
 });
 
 export const action = async ({ request }: ActionFunctionArgs) => {
@@ -88,6 +89,7 @@ export const action = async ({ request }: ActionFunctionArgs) => {
  providers,
  tags: _tags,
  fillGaps,
+ userAuthoredQuery,
  } = submission.data;
 
  // Check they should be able to access it
@@ -126,6 +128,7 @@ export const action = async ({ request }: ActionFunctionArgs) => {
  operations,
  providers,
  fillGaps,
+ userAuthoredQuery,
  // Set higher concurrency if many widgets are on screen at once
  customOrgConcurrencyLimit: env.METRIC_WIDGET_DEFAULT_ORG_CONCURRENCY_LIMIT,
  });

apps/webapp/app/services/queryService.server.ts

@@ -100,6 +100,13 @@ export type ExecuteQueryOptions<TOut extends z.ZodSchema> = Omit<
  };
  /** Custom per-org concurrency limit (overrides default) */
  customOrgConcurrencyLimit?: number;
+ /**
+ * Set when the caller wrote `query` themselves, as on the public query API and
+ * the query editor. ClickHouse rejecting their SQL is then their mistake, so it
+ * is logged as a warning instead of raising an alert. Leave unset for TRQL we
+ * generate, where the same rejection is a bug worth alerting on.
+ */
+ userAuthoredQuery?: boolean;
 };
 
 /**

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 sentryRemix 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,32 @@ 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.
+ *
+ * Reached through the default export because `@sentry/remix` is CommonJS and
+ * Node's ESM loader does not detect this transitively re-exported name, so a
+ * named import resolves at build time and then fails when the server boots.
+ */
+function createContextManager() {
+ return new sentryRemix.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 +324,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 sentryRemix 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 sentryRemix.SentryContextManager() });
+
+ const observed = await raceTwoRequests();
+
+ expect(observed).toEqual({ slow: "slow", fast: "fast" });
+ });
+});

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

@@ -171,13 +171,15 @@ 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,
- });
+ };
+
+ this.logger.error("Error querying clickhouse", errorLogFields);
 
  recordClickhouseError(span, clickhouseError);
 
@@ -260,6 +262,16 @@ 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>;
+ /**
+ * Set when the SQL originates from whoever made the request rather than
+ * from us. Invalid-SQL rejections are then their mistake, not a bug.
+ */
+ userAuthoredQuery?: boolean;
  }): ClickhouseQueryWithStatsFunction<z.input<TIn>, z.output<TOut>> {
  return async (params, options) => {
  const queryId = randomUUID();
@@ -320,13 +332,25 @@ export class ClickhouseClient implements ClickhouseReader, ClickhouseWriter {
  );
 
  if (clickhouseError) {
- this.logger.error("Error querying clickhouse", {
+ const errorLogFields = {
+ ...req.logFields,
  name: req.name,
  error: clickhouseError,
  query: req.query,
  params,
  queryId,
- });
+ };
+
+ switch (classifyClickhouseError(clickhouseError, req.userAuthoredQuery)) {
+ case "quota":
+ this.logger.warn("Query exceeded a ClickHouse limit", errorLogFields);
+ break;
+ case "invalid-sql":
+ this.logger.warn("ClickHouse rejected an invalid query", errorLogFields);
+ break;
+ default:
+ this.logger.error("Error querying clickhouse", errorLogFields);
+ }
 
  recordClickhouseError(span, clickhouseError);
 
@@ -453,13 +477,15 @@ 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,
- });
+ };
+
+ this.logger.error("Error querying clickhouse", errorLogFields);
 
  recordClickhouseError(span, clickhouseError);
 
@@ -599,13 +625,15 @@ export class ClickhouseClient implements ClickhouseReader, ClickhouseWriter {
 
  span.setAttributes({ "clickhouse.rows": rowCount });
  } catch (error) {
-self.logger.error("Error streaming clickhouse", {
+const errorLogFields = {
  name: req.name,
  error,
  query: req.query,
  params,
  queryId,
- });
+ };
+
+ self.logger.error("Error streaming clickhouse", errorLogFields);
 
  if (error instanceof Error) {
  recordClickhouseError(span, error);
@@ -1001,6 +1029,65 @@ export class ClickhouseClient implements ClickhouseReader, ClickhouseWriter {
  }
 }
 
+/**
+ * ClickHouse error types raised by a query that is valid but asks for more than
+ * it is allowed to spend. Only downgraded for SQL the caller wrote: a runaway
+ * query we generated is our bug and still has to alert.
+ */
+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",
+]);
+
+/**
+ * ClickHouse error types that mean the SQL itself is wrong. Only treated as the
+ * caller's fault when the query was written by the caller — the same error on a
+ * query we generated is our bug and has to keep alerting.
+ */
+const CLICKHOUSE_INVALID_SQL_ERROR_TYPES = new Set([
+ "NOT_AN_AGGREGATE",
+ "ILLEGAL_AGGREGATION",
+ "UNKNOWN_IDENTIFIER",
+ "UNKNOWN_FUNCTION",
+ "UNKNOWN_TABLE",
+ "AMBIGUOUS_COLUMN_NAME",
+ "MULTIPLE_EXPRESSIONS_FOR_ALIAS",
+ "SYNTAX_ERROR",
+ "BAD_ARGUMENTS",
+ "TYPE_MISMATCH",
+ "NO_COMMON_TYPE",
+ "ILLEGAL_TYPE_OF_ARGUMENT",
+ "ILLEGAL_COLUMN",
+ "CANNOT_CONVERT_TYPE",
+ "CANNOT_PARSE_TEXT",
+ "CANNOT_PARSE_NUMBER",
+ "CANNOT_PARSE_DATE",
+ "CANNOT_PARSE_DATETIME",
+ "CANNOT_PARSE_INPUT_ASSERTION_FAILED",
+]);
+
+type ClickhouseErrorCategory = "quota" | "invalid-sql" | "fault";
+
+function classifyClickhouseError(
+ error: Error,
+ userAuthoredQuery: boolean | undefined
+): ClickhouseErrorCategory {
+ if (!userAuthoredQuery || !(error instanceof ClickHouseError) || error.type === undefined) {
+ return "fault";
+ }
+ if (CLICKHOUSE_QUOTA_ERROR_TYPES.has(error.type)) {
+ return "quota";
+ }
+ if (CLICKHOUSE_INVALID_SQL_ERROR_TYPES.has(error.type)) {
+ return "invalid-sql";
+ }
+ return "fault";
+}
+
 function recordClickhouseError(span: Span, error: Error): void {
  if (error instanceof ClickHouseError) {
  span.setAttributes({