diff --git a/packages/cli/src/lib/api/infrastructure.ts b/packages/cli/src/lib/api/infrastructure.ts index 056f9a4fb..039670c09 100644 --- a/packages/cli/src/lib/api/infrastructure.ts +++ b/packages/cli/src/lib/api/infrastructure.ts @@ -40,6 +40,7 @@ import { getEnv } from "../env.js"; import { ApiError, CliError, + type FunctionArgument, stringifyUnknown, TimeoutError, } from "../errors.js"; @@ -230,12 +231,30 @@ export type ApiRequestOptions = { idempotent?: boolean; }; -/** The string `code` of a JSON error body, e.g. `invalid_search_query`. */ -function readErrorCode(body: unknown): string | undefined { - if (body === null || typeof body !== "object" || !("code" in body)) { - return undefined; +/** + * Structured fields of a JSON error body: its string `code` (e.g. + * `invalid_search_query`) and the rejected `function` and `argument` + * (sent with `invalid_function_argument`). + */ +function readErrorFields(body: unknown): { + code?: string; + functionArgument?: FunctionArgument; +} { + if (body === null || typeof body !== "object") { + return {}; } - return typeof body.code === "string" ? body.code : undefined; + const fields = body as Record; + const code = typeof fields.code === "string" ? fields.code : undefined; + if ( + typeof fields.function !== "string" || + typeof fields.argument !== "string" + ) { + return { code }; + } + return { + code, + functionArgument: { function: fields.function, argument: fields.argument }, + }; } const CONNECT_TIMEOUT_HINT = @@ -371,7 +390,7 @@ export function throwApiError( status, enrichDetail(status, detail, hasUsableDetail), undefined, - { enriched403: is403, code: readErrorCode(error) }, + { enriched403: is403, ...readErrorFields(error) }, ); } @@ -858,7 +877,7 @@ async function throwRawApiError( response.status, enrichDetail(response.status, detail, detail !== undefined), endpoint, - { enriched403: is403, code: readErrorCode(body.json) }, + { enriched403: is403, ...readErrorFields(body.json) }, ); } diff --git a/packages/cli/src/lib/errors.ts b/packages/cli/src/lib/errors.ts index c78eb9cc3..c2c2e25bb 100644 --- a/packages/cli/src/lib/errors.ts +++ b/packages/cli/src/lib/errors.ts @@ -169,6 +169,9 @@ export class HostScopeError extends CliError { } } +/** A function argument named by a Sentry 400 error body. */ +export type FunctionArgument = { function: string; argument: string }; + /** * API request errors from Sentry. * @@ -176,7 +179,7 @@ export class HostScopeError extends CliError { * @param status - HTTP status code * @param detail - Detailed error message from API response * @param endpoint - API endpoint that failed - * @param options - `enriched403` and `code`, documented on the fields below + * @param options - Structured error fields, documented on the fields below */ export class ApiError extends CliError { readonly status: number; @@ -189,6 +192,12 @@ export class ApiError extends CliError { */ readonly code?: string; + /** + * Function and argument the server rejected, sent with code + * `invalid_function_argument` (e.g. `p75` and `cart.total`). + */ + readonly functionArgument?: FunctionArgument; + /** * Set by centralized 403 enrichment in `infrastructure.ts`. * Command-layer code can check this to avoid double-enriching @@ -204,7 +213,12 @@ export class ApiError extends CliError { { enriched403 = false, code, - }: { enriched403?: boolean; code?: string } = {}, + functionArgument, + }: { + enriched403?: boolean; + code?: string; + functionArgument?: FunctionArgument; + } = {}, ) { super(message, EXIT.API); this.name = "ApiError"; @@ -213,6 +227,7 @@ export class ApiError extends CliError { this.endpoint = endpoint; this.enriched403 = enriched403; this.code = code; + this.functionArgument = functionArgument; } override format(): string { diff --git a/packages/cli/src/lib/explore-errors.ts b/packages/cli/src/lib/explore-errors.ts index 82b564332..6846079d8 100644 --- a/packages/cli/src/lib/explore-errors.ts +++ b/packages/cli/src/lib/explore-errors.ts @@ -6,7 +6,7 @@ import { parseFunction } from "@sentry/toolkit-core/events-fields"; import { getSortField } from "@sentry/toolkit-core/events-sort"; -import { ApiError, validationError } from "./errors.js"; +import { ApiError, type FunctionArgument, validationError } from "./errors.js"; /** User-authored expressions and the request they contributed to. */ type ExploreErrorContext = { @@ -22,9 +22,17 @@ type ExploreErrorContext = { userSort: string | undefined; }; -/** Match narrow backend diagnostics without maintaining an aggregate catalog. */ +/** `code` of a 400 that names the function argument it rejected (getsentry/sentry#129954). */ +const INVALID_FUNCTION_ARGUMENT_CODE = "invalid_function_argument"; + +/** + * Match narrow backend diagnostics without maintaining an aggregate catalog. + * Beyond the specific corrections, a 400 whose structured fields name this + * expression's own function and argument gets a generic hint. + */ function recoveryHint( detail: string, + rejected: FunctionArgument | undefined, expression: string, dataset: string, ): string | undefined { @@ -56,10 +64,30 @@ function recoveryHint( if (detail === `Unknown function ${parsed.name}`) { return "Aggregate functions depend on the dataset. Check the function name and selected dataset; sentry explore --help includes usage examples."; } + + if ( + !rejected || + rejected.function !== parsed.name || + !parsed.arguments.includes(rejected.argument) + ) { + return; + } + const { argument } = rejected; + // `--dataset transactions` queries spans, which keep durations in span.duration. + if (dataset === "spans" && argument === "transaction.duration") { + return "Use span.duration instead of transaction.duration. Transactions are queried as spans, which store their duration in span.duration."; + } + const problem = `${parsed.name}() does not accept ${argument} on dataset "${dataset}": it may not exist there, or may not have the type ${parsed.name}() needs.`; + return ATTRIBUTE_DATASETS.has(dataset) + ? `${problem} Reference a numeric custom attribute with its type, e.g. tags[${argument},number]; to check the key and its type, run: sentry attribute list / ${argument} --dataset ${dataset}` + : `${problem} Check the column name and its type.`; } +/** Datasets whose custom attributes use `tags[name,type]` keys listed by `sentry attribute list`. */ +const ATTRIBUTE_DATASETS = new Set(["spans", "logs"]); + /** - * Add a correction for a known rejected user expression, preserving the server + * Add a correction for a rejected user expression, preserving the server * detail. Unknown failures and errors caused by CLI-generated input are returned * unchanged so they retain their API error classification and diagnostics. */ @@ -82,8 +110,17 @@ export function toExploreFieldError( }); } + const rejected = + error.code === INVALID_FUNCTION_ARGUMENT_CODE + ? error.functionArgument + : undefined; for (const { expression, value, flag } of expressions) { - const hint = recoveryHint(error.detail, expression, context.dataset); + const hint = recoveryHint( + error.detail, + rejected, + expression, + context.dataset, + ); if (hint) { return validationError( `Sentry rejected --${flag} "${value}" for dataset "${context.dataset}".\n\n${error.detail}`, diff --git a/packages/cli/test/commands/explore.test.ts b/packages/cli/test/commands/explore.test.ts index bdecdcda3..cc0188fc6 100644 --- a/packages/cli/test/commands/explore.test.ts +++ b/packages/cli/test/commands/explore.test.ts @@ -1365,6 +1365,114 @@ describe("sentry explore", () => { expect((error as Error).message).not.toContain("attribute list"); }); + const rejectedArgument = (fn: string, argument: string) => ({ + code: "invalid_function_argument", + functionArgument: { function: fn, argument }, + }); + const rejectedArgumentError = ( + detail: string, + fn: string, + argument: string, + ) => + new ApiError( + "Bad request", + 400, + detail, + undefined, + rejectedArgument(fn, argument), + ); + + test.each([ + [ + "a string attribute in a numeric aggregate", + { dataset: "spans", field: ["span.op", "p75(cart.total)"] }, + rejectedArgumentError( + "cart.total is invalid for parameter 1 in p75. Its a string type field, but it must be one of these types: {'number'}", + "p75", + "cart.total", + ), + '--field "p75(cart.total)"', + "tags[cart.total,number]; to check the key and its type, run: sentry attribute list / cart.total --dataset spans", + ], + [ + "transaction.duration on transactions", + { + dataset: "transactions", + field: ["transaction", "sum(transaction.duration)"], + sort: "-sum(transaction.duration)", + }, + rejectedArgumentError( + "transaction.duration is invalid for parameter 1 in sum. Its a string type field, but it must be one of these types: {'number', 'duration'}", + "sum", + "transaction.duration", + ), + '--field "sum(transaction.duration)" for dataset "spans"', + "Use span.duration instead of transaction.duration.", + ], + [ + "a timestamp aggregate on spans", + { dataset: "spans", field: ["min(timestamp)"] }, + rejectedArgumentError( + "timestamp is invalid for parameter 1 in min. Its a string type field, but it must be one of these types: {'number'}", + "min", + "timestamp", + ), + '--field "min(timestamp)"', + 'min() does not accept timestamp on dataset "spans"', + ], + [ + "a non-numeric errors column", + { field: ["issue", "min(title)"] }, + rejectedArgumentError( + "min(title): column argument invalid: title is not a numeric column", + "min", + "title", + ), + '--field "min(title)" for dataset "errors"', + "Check the column name and its type.", + ], + [ + "an unknown errors column passed only through --sort", + { field: ["device", "count()"], sort: "-avg(device.memory)" }, + rejectedArgumentError( + "avg(device.memory): column argument invalid: device.memory is not a valid column", + "avg", + "device.memory", + ), + '--sort "-avg(device.memory)"', + 'avg() does not accept device.memory on dataset "errors"', + ], + [ + "an invalid conditional-count argument", + { dataset: "logs", field: ["count_if(severity,equals,error)"] }, + rejectedArgumentError( + "severity is not a valid argument for count_if", + "count_if", + "severity", + ), + '--field "count_if(severity,equals,error)"', + "sentry attribute list / severity --dataset logs", + ], + ])( + "attributes %s to the user's expression", + async (_label, flags, apiError, rejected, hint) => { + resolveTargetSpy.mockResolvedValue({ org: "test-org" }); + queryEventsSpy.mockRejectedValue(apiError); + const { context } = createContext(); + + const error = await func + .call(context, { ...DEFAULT_FLAGS, ...flags }, "test-org/") + .catch((caught: unknown) => caught); + + expect(error).toBeInstanceOf(ValidationError); + expect((error as Error).message).toContain(apiError.detail); + expect((error as Error).message).toContain(rejected); + expect((error as Error).message).toContain(hint); + expect((error as Error).message).not.toContain("timestamp_precise"); + expect(isUserError(error)).toBe(true); + }, + ); + test.each([ ["default fields", {}, "Unknown function count"], [ @@ -1383,9 +1491,21 @@ describe("sentry explore", () => { "Unknown function p10", ], [ - "a spans backend without timestamp aggregates", - { dataset: "spans", field: ["min(timestamp)"] }, - "timestamp is invalid for parameter 1 in min. Its a string type field", + "a rejected argument the user did not pass", + { dataset: "spans", field: ["p75(span.duration)"] }, + "cart.total is invalid for parameter 1 in p75. Its a string type field", + rejectedArgument("p75", "cart.total"), + ], + [ + "a rejected argument of a default field", + { query: "level:error" }, + "min(title): column argument invalid: title is not a numeric column", + rejectedArgument("min", "title"), + ], + [ + "a rejected argument from a server that sends no code", + { dataset: "spans", field: ["p75(cart.total)"] }, + "cart.total is invalid for parameter 1 in p75. Its a string type field", ], [ "an unknown rejection", @@ -1401,18 +1521,27 @@ describe("sentry explore", () => { }, "Unknown function sum", ], - ])("preserves the API error for %s", async (_label, flags, detail) => { - resolveTargetSpy.mockResolvedValue({ org: "test-org" }); - const original = new ApiError("Bad request", 400, detail); - queryEventsSpy.mockRejectedValue(original); - const { context } = createContext(); + ])( + "preserves the API error for %s", + async (_label, flags, detail, fields?) => { + resolveTargetSpy.mockResolvedValue({ org: "test-org" }); + const original = new ApiError( + "Bad request", + 400, + detail, + undefined, + fields, + ); + queryEventsSpy.mockRejectedValue(original); + const { context } = createContext(); - await expect( - func.call(context, { ...DEFAULT_FLAGS, ...flags }, "test-org/"), - ).rejects.toBe(original); - expect(isUserError(original)).toBe(false); - expect(classifySilenced(original)).toBeNull(); - }); + await expect( + func.call(context, { ...DEFAULT_FLAGS, ...flags }, "test-org/"), + ).rejects.toBe(original); + expect(isUserError(original)).toBe(false); + expect(classifySilenced(original)).toBeNull(); + }, + ); test("preserves non-400 errors even when their detail mentions a user field", async () => { resolveTargetSpy.mockResolvedValue({ org: "test-org" }); diff --git a/packages/cli/test/lib/api-client.coverage.test.ts b/packages/cli/test/lib/api-client.coverage.test.ts index e384bb440..eda59d83a 100644 --- a/packages/cli/test/lib/api-client.coverage.test.ts +++ b/packages/cli/test/lib/api-client.coverage.test.ts @@ -1502,6 +1502,35 @@ describe("infrastructure.ts", () => { }); }); + test("keeps the function and argument a 400 body rejected", async () => { + globalThis.fetch = mockFetch( + async () => + new Response( + JSON.stringify({ + detail: "cart.total is invalid for parameter 1 in p75.", + code: "invalid_function_argument", + function: "p75", + argument: "cart.total", + }), + { + status: 400, + statusText: "Bad Request", + headers: { "Content-Type": "application/json" }, + }, + ), + ); + + const error = await apiRequestToRegion( + "https://sentry.io", + "/test/", + ).catch((caught: unknown) => caught); + + expect(error).toMatchObject({ + code: "invalid_function_argument", + functionArgument: { function: "p75", argument: "cart.total" }, + }); + }); + test("JSON-stringifies error body without detail field", async () => { globalThis.fetch = mockFetch( async () => diff --git a/packages/cli/test/lib/api/infrastructure.test.ts b/packages/cli/test/lib/api/infrastructure.test.ts index dc29c4818..4ba46a5ae 100644 --- a/packages/cli/test/lib/api/infrastructure.test.ts +++ b/packages/cli/test/lib/api/infrastructure.test.ts @@ -323,6 +323,30 @@ describe("throwApiError", () => { ); }); + test("HTTP error keeps the function and argument the body rejected", () => { + const mockResponse = new Response("", { + status: 400, + statusText: "Bad Request", + }); + + expect(() => + throwApiError( + { + detail: "cart.total is invalid for parameter 1 in p75.", + code: "invalid_function_argument", + function: "p75", + argument: "cart.total", + }, + mockResponse, + "Failed to list events", + ), + ).toThrow( + expect.objectContaining({ + functionArgument: { function: "p75", argument: "cart.total" }, + }), + ); + }); + test("HTTP error without detail uses stringified error", () => { const mockResponse = new Response("", { status: 500,