Repository navigation
feat(cli): Narrow alert aggregates and match fields by segment - #1785
Conversation
`alert metrics create|edit --aggregate` now completes only what metric alerts accept on the `--dataset` given, from getsentry/sentry: `EAP_FUNCTIONS` for spans, the subset logs define (no formulas, no `p100`) for logs, `count()` and `count_unique(user|release|dist)` for errors, and the two crash-free percentages for `metrics`/`sessions`. `DatasetFields.aggregates` carries whole aggregates that are not a function of one field. Fields now match one `.` segment at a time (`fuzzyMatchSegments`): the Nth typed segment is fuzzy-matched against the candidate's Nth, so `span.d` finds `span.duration` but never `span.op`, while a typo within a segment (`span.duraton`) still matches. Co-Authored-By: BYK <126780+BYK@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
BYK
left a comment
There was a problem hiding this comment.
@sentry/api has no list to use. 0.428.0 types detector data_sources as Array. The only aggregates it mentions are in prose examples from workflow_engine/endpoints/validators/api_docs_help_text.py, and those are out of date: they list percentile(measurements.lcp,x), but EAP spans don't define percentile. Exposing the aggregates upstream would be worthwhile. The natural form is an aggregate schema on the SnubaQueryValidator data source, with an enum or pattern per dataset/eventTypes. Clients could then stop copying EAP_FUNCTIONS and the crash-rate regex.
Let's file an issue to make this available in @sentry/api. You'll need to figure out whether this should be from getsentry/sentry repo or getsentry/sentry-api-schema repo.
Background attribute-key fetch for explore: not added
How about we do this in auth, and/or upgrade as part of an "initialization" to get a cache in place? Too expensive and invasive or feasible? The logic would be:
- Refresh on
auth - Refresh on
upgradeif authenticated
Alternatively we can consider refreshing on auto complete setup if authenticated instead of upgrade which gets called anyway.
Follow-up to #1782, which was merged before these changes landed. It implements the answers to that PR's design questions.
Alert
--aggregatecompletes only what metric alerts acceptalert metrics create|edit --aggregateno longer reuses the widget function lists. Each--datasetgets the aggregates the Sentry backend accepts for it:--datasetspans(andtransaction(s))EAP_FUNCTIONS:count,count_unique,avg,p50–p99,p100,max,min,sum,epm,eps,failure_count,failure_rate,apdex,user_misery, called on cached span attribute keyslogsEAP_FUNCTIONSthat logs define: no formulas and nop100, called on cached log attribute keyserrors/eventscount(),count_unique(user),count_unique(release),count_unique(dist)metrics/sessions(crash rate)percentage(sessions_crashed,sessions),percentage(users_crashed,users)A new
DatasetFields.aggregatesholds whole aggregates that aren't a function of one field.Sources (getsentry/sentry master @
063a6cb2):src/sentry/snuba/snuba_query_validator.py_validate_aggregate(L253) callscheck_aggregate_column_supportandtranslate_aggregate_field. For EAP it skips the SnQL query check (L361).src/sentry/incidents/logic.py:EAP_FUNCTIONS(L140). For these functions,get_column_from_aggregate(L179) skips column validation on EAP datasets.SUPPORTED_COLUMNSandTRANSLATABLE_COLUMNS(L118, L124).check_aggregate_column_support(L214) checks columns against these lists, so errors can't take arbitrary tag keys.src/sentry/snuba/entity_subscription.py:PerformanceSpansEAPRpcEntitySubscription.build_rpc_requestresolves the aggregate withSpansorOurLogsdefinitions, depending on the event type.CRASH_RATE_ALERT_AGGREGATE_RE(L59) anddetermine_crash_rate_alert_entityrestrict crash rate to the percentages above.src/sentry/search/eap/spans/aggregates.pyandformulas.pydefine everyEAP_FUNCTIONSentry.src/sentry/search/eap/ourlogs/aggregates.pydefines nop100, andourlogs/definitions.pysetsformulas={}.static/app/views/detectors/datasetConfig/errors.tsxofferscountandcount_unique(user), andreleases.tsxmapscrash_free_rate(session|user)to the two percentages.@sentry/apihas no list to use. 0.428.0 types detectordata_sourcesasArray<unknown>. The only aggregates it mentions are in prose examples fromworkflow_engine/endpoints/validators/api_docs_help_text.py, and those are out of date: they listpercentile(measurements.lcp,x), but EAP spans don't definepercentile. Exposing the aggregates upstream would be worthwhile. The natural form is anaggregateschema on theSnubaQueryValidatordata source, with an enum or pattern perdataset/eventTypes. Clients could then stop copyingEAP_FUNCTIONSand the crash-rate regex. Tracked in getsentry/sentry#129450.Caveat: the errors list is the practical set, not an exhaustive allowlist. Errors accept any Discover function whose column is empty or in the column allowlist, provided the test query runs. Completion offers what the UI offers, plus the other allowed columns.
Fields match one
.segment at a timefuzzyMatchSegmentsmatches the Nth typed segment against the candidate's Nth segment, usingfuzzyMatchsearch mode. A candidate must have a segment for each typed one, and may have more after them. So:avg(span.d→avg(span.duration), neveravg(span.op)avg(span.duraton→avg(span.duration)(typo within the segment)span→ everyspan.*This applies to every field completion (
explore --field/--sort, widget--group-by/--query/--sort, alert--aggregate), both bare and insidefn(orfn:. Field completion now matches values only. It no longer matches attribute type labels such asnumber.Background attribute-key fetch for explore: not added
exploredoesn't get attribute keys from any call it already makes./events/meta covers only the requested fields, and the error path (toExploreFieldError) fetches nothing. A separate fetch costs too much for what the cache would keep. I measuredGET /organizations/sentry/trace-items/attributes/on sentry.io (3 runs each, curl, gzip):per_page=100The cache keeps 500 keys per kind per org. The server returns keys in name order, so the first rows are keys like
0,1,AGP_VERSION, and the kept 500 would missspan.*. A small page has the same problem. The fetch also adds a request that the process must either wait for before exiting or abandon.sentry attribute liststill seeds the cache.The 500 cap is unchanged.
Checks
pnpm exec tsc --noEmit(packages/cli): passpnpm run lint: passvitest runontest/lib/complete.test.ts,test/lib/fuzzy.test.ts,test/lib/db/completion-cache.test.ts,test/lib/completion-coverage.test.ts,test/lib/api/completion-seed.test.ts,test/lib/introspect.test.ts,test/commands/alert,test/commands/explore.test.ts,test/commands/dashboard: 28 files, 659 tests, all passtest/libsuite didn't finish within the sandbox's 15-minute limit, so it wasn't run to completion.via @BYK.
--
View Session in Sentry