Repository navigation
fix(cli): Silence user-supplied search query syntax errors and validate failure_rate - #1666
Closed
sentry[bot] wants to merge 2 commits into
Closed
sentry[bot] wants to merge 2 commits into
sentry[bot] wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Replace the `void logError` catch, which sidesteps the no-silent-catch rule, with a log.debug call as required by packages/cli/AGENTS.md.
Contributor
There was a problem hiding this comment.
Junior review: I checked this against the issue data and the producer and consumer contracts. It looks justified. I pushed one small fix in 4e342f9.
What I verified
- CLI-1CZ breakdown (from the issue tags, 1837 events, all
cli_error.kind=query):issue.list1524,issues11,feedback.list1. That is about 83.6% coming throughtoSearchQueryErrorwith a user-supplied--query. The rest are CLI-side validators:dashboard.widget.add187 and.edit32 (unknown aggregate, invalid tracemetrics aggregate), andalert.metrics.create82 (query cannot be empty.). So the description's claim holds up. failure_rateis a real defect. It is the most common unknown aggregate in a sample of 100widget.addevents (about 40 hits), including on 0.47.0. Sentry supports it for spans: it is defined insrc/sentry/search/eap/spans/formulas.pyand appears inALLOWED_EXPLORE_VISUALIZE_AGGREGATESinstatic/app/utils/fields/index.ts. #1295 dropped it from the span list only because it was already in the discover list, and #1515 later removed that discover entry, so spans stopped accepting it.- The
deriveErrorKindchange splits the issue as intended. Thecliproject's fingerprint rules group ValidationErrors by{{ tags.cli_error.kind }}, soquery: <prefix>creates separate groups.extractMessagePrefixstrips quoted values, which keeps the number of groups small (Unknown aggregate function,Invalid tracemetrics aggregate,query cannot be).
Correction pushed
- The new catch in
recordSilencedErrorusedvoid logError. That gets past theno-silent-catchlint rule without doing whatpackages/cli/AGENTS.mdasks for, which is to log or re-throw. It now callslog.debug. These tests pass locally (397 tests):test/lib/error-reporting.test.ts,errors.test.ts,types/dashboard.test.ts,commands/issue/list.test.ts. oxlint and oxfmt are clean on the file.
For the merging maintainer (not blockers)
- This reverses the default chosen in getsentry/cli#1154, which kept user-query ValidationErrors captured on purpose as a query-linting UX signal (and offered to silence them separately). The
cli.error.silencedmetric and thecli.query_syntax_silencedlog keep the volume visible, which follows the pattern used for CLI-1JY, CLI-1FN and CLI-RP. Please confirm that's the policy you want. flags.queryhas already been throughsanitizeQuery, which applies bracket and comma fixes, numericproject:rewriting and OR rewriting. A 400 caused by one of those rewrites would also be silenced. The structured log keeps the server detail, so this is still traceable.- Since
ERROR_AGGREGATE_FUNCTIONSspreads the span list, error-events now acceptsfailure_ratetoo. That list already accepts other span-only functions (p50 etc.), so this doesn't make it newly loose.
Member
|
We don't want this to keep an eye how users are failing, how frequently, and to see if we can actually improve the situation. |
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root Cause
When the Sentry server rejects a user-supplied search query (e.g.
is:403,category:csp,status:401),toSearchQueryErrorwraps the 400ApiErrorinto aValidationErrorwith field"query". BecauseclassifySilenceddid not handle this field, every rejected user query was forwarded to Sentry as an issue.deriveErrorKindkeyed onerror.field ?? ..., so allValidationErrors with field"query"receivedcli_error.kind: "query"and were grouped into a single catch-all issue (CLI-1CZ). This bucketed together user query typos (~83% of volume) and genuine CLI validation bugs, such assentry dashboard widget addrejectingfailure_ratebecause it was missing fromSPAN_AGGREGATE_FUNCTIONS.Changes
Silence user query syntax errors (
src/lib/errors.ts,src/lib/error-reporting.ts):toSearchQueryError, construct theValidationErrorwithUSER_QUERY_SYNTAX_FIELD = "query.user_syntax".classifySilenced, return"user_query_syntax"for errors with this field. The CLI already displays the server's detail and search syntax suggestions; queries the CLI builds itself still fail as 400ApiErrors and remain captured.recordSilencedError, emitSentry.logger.info("cli.query_syntax_silenced")with the redacted parse detail and count the event under the existingcli.error.silencedmetric.Disambiguate remaining query ValidationErrors (
src/lib/error-reporting.ts):deriveErrorKind, whenerror.field === "query", suffix the kind with the message prefix (query: <first words>) so genuine CLI defects (e.g. unknown aggregates, empty alert query) do not group into one catch-all.Add
failure_rateto span aggregates (src/types/dashboard.ts):failure_rateinSPAN_AGGREGATE_FUNCTIONSso dashboard span widget creation accepts it.Tests:
test/lib/errors.test.tsandtest/commands/issue/list.test.tsto expectquery.user_syntax.test/lib/error-reporting.test.ts.failure_rateintest/types/dashboard.test.ts.Fixes CLI-1CZ
@sentry <feedback>: Autofix iterates on these changes@sentry stop iterating: Autofix stops iterating on this runThis PR was automatically generated by Sentry. You can adjust this setting at any time.