fix(eap): Sort absent order by attributes deterministically when paging - #8246
Draft
JoshuaKGoldberg wants to merge 2 commits into
Draft
fix(eap): Sort absent order by attributes deterministically when paging#8246JoshuaKGoldberg wants to merge 2 commits into
JoshuaKGoldberg wants to merge 2 commits into
Conversation
A flextime page token encodes the last row's ORDER BY values, and the next page asks for the rows less than that tuple. Both halves of that break when an ORDER BY column can be NULL: `get_filters` raised `AttributeError: None` reading a null bookmark value, since `is_null` is not part of the `value` oneof, and a NULL tuple element makes the comparison NULL, which drops the row rather than paging past it. Absent map-backed keys now sort as their type's zero value in both the ORDER BY and the page boundary, so any nullable attribute can take part in a flextime ORDER BY. Sentry needs that to break log ordering ties on `sentry.timestamp.sequence`, which older SDKs do not emit.
…ision `time_window is not None` says the routing strategy chose to page by keyset, but `get_routing_decision` swallows an estimation failure and rebuilds the decision with no time window, while the client's page token still compares against the ORDER BY of the page before it. Ordering absent keys on that signal could put the two out of step, silently skipping or repeating rows at a page boundary. The requested flextime mode is stable across a pagination session, so key off that. Also stop stamping a type on normalized page-token columns: those are never NULL and their ORDER BY compares the raw column, so the sentinel belongs only on the map-backed columns the resolver wraps.
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
1 participant
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.
Problem
Flextime pagination (
FlexibleTimeWindowPageWithFilters, added in #7445) encodes the last row's ORDER BY values in the page token, and the next page asks for the rowsless(tuple(order by columns), tuple(bookmark values)). That assumes every ORDER BY column is non-nullable. Map-backed attributes are not:add_existence_check_to_subscriptable_referencesselects them asif(mapContains(...), attributes_x[...], NULL), andconvert_resultsturns an absent key intoAttributeValue(is_null=True).So putting any sparse attribute in a flextime ORDER BY breaks in two ways:
get_filtersread the bookmark value withgetattr(value, str(value.WhichOneof("value"))).is_nullis field 11 and is not part of thevalueoneof, soWhichOneofreturnsNoneand the request dies withAttributeError: None— a 500 on page 2 onward.sentry.timestamp_precisesits in a flextime ORDER BY today and never hits this only because Relay writes it on every log.Changes
An absent map-backed key now sorts as its type's zero value, applied to both the ORDER BY (
_convert_order_by, for flextime requests) and the page-boundary comparison, so the two agree on one total order. A stored value equal to the sentinel merely ties with an absent one, and flextime's mandatory trailingsentry.item_idbreaks that tie.Only map-backed columns get a sentinel. Normalized columns are never NULL and their ORDER BY compares the raw column, so the page token leaves their type unset and both sides stay identical. The ORDER BY side keys off the requested flextime mode rather than off
routing_decision.time_window: a swallowed estimation failure rebuilds the decision without a time window, and that must not change how a page compares against the token issued by the page before it.Note this changes the ORDER BY SQL for flextime queries today —
sentry.timestamp_preciseis map-backed, so it picks up anifNull. Relay writes it on every log, so results are unchanged; it is not a no-op on the hot path though. Sort-key prefix matching foroptimize_read_in_orderis untouched (the leadingtimestampis normalized).The page token now carries the attribute type so
get_filtersknows how to sort a null bookmark. A token issued before this change has no type and keeps the old comparison, so pagination sessions spanning the deploy behave as they do today.Why
Logs want
sentry.timestamp.sequenceas the ordering tiebreaker aftertimestampandtimestamp_precise— sub-millisecond ties are exactly what that counter exists for, and float64timestamp_precisecan't resolve them. SDKs that predate the counter don't emit it, so that attribute is sparse and today can't go in the flextime ORDER BY. Sentry side: getsentry/sentry#120451Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.