fix(eap): Keep EAP queries inside the full-fidelity retention window - #120872
Open
phacops wants to merge 5 commits into
Open
fix(eap): Keep EAP queries inside the full-fidelity retention window#120872phacops wants to merge 5 commits into
phacops wants to merge 5 commits into
Conversation
full_retention_window() took the 90 day API default, but EAP only keeps 30 days of full-fidelity (tier 1) data. Snuba routes any query whose window starts more than 31 days ago to a sampled tier, so the wider window could not surface older data and instead pushed these existence checks onto a tier holding a fraction of the items, risking false negatives. Anchor the window to tier 1 retention instead, staying a day inside the boundary so the midnight flooring in adjust_start_end_window() cannot cross it.
EAP endpoints default to a 90 day window when a request omits statsPeriod/start/end, which Snuba routes to the TIER_8 downsampled table. Test fixtures are only written to tier 1, and the downsampled tables are fed by a materialized view keeping roughly 1 in 8 items, so those tests would assert against an arbitrary subset of what they stored. Add apply_eap_default_stats_period() to default EAP requests to the full-fidelity window unless a test pins its own, and move the few tests that stored data 35-80 days old inside that window. Datasets are derived from RPC_DATASETS so the set cannot drift, excluding the item types Snuba keeps at full retention.
The constant sat a day inside Snuba's 31 day downsampling boundary because adjust_start_end_window() floors the start to midnight, leaving as little as a second of headroom just before midnight UTC. Take the full 30 days and accept that margin.
phacops
marked this pull request as ready for review
July 30, 2026 15:48
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1726a0b. Configure here.
full_retention_window() capped every item type at tier 1 retention, but Snuba never downsamples the types in ITEM_TYPE_FULL_RETENTION (uptime results and preprod). The attribute-context endpoint accepts preprod, so the cap hid older data that is still stored at full fidelity, causing the false negatives the function exists to prevent. Take the item type as an argument and fall back to the 90 day API default for those types. The full-retention set now lives in one constant shared with the test helper.
Member
|
Reading this, just to clarify; since all base trace item types (spans / logs / metrics) are 30d full (+ optional 13 month downsampled), we're just matching the hardcoded values on the retention_days with the query window? Actual nit; Isn't the downsampling stable? Can't we just mock eap items that match the hash + modulo (and ones that don't) so we can actually test the downsampling is working / being applied as expected? |
k-fish
approved these changes
Jul 30, 2026
wmak
approved these changes
Jul 30, 2026
execute_trace_table_query passes no statsPeriod, so the traces endpoint falls back to MAX_STATS_PERIOD (90 days). Snuba routes any span query whose window starts more than 31 days ago to the TIER_8 downsampled table, so the tool got an arbitrary fraction of matching traces instead of all of them. Default the window to EAP's full-fidelity retention when the caller gives neither stats_period nor start/end. The docstring already claimed a 24 hour default that the code never implemented; it now describes the real behavior. Co-authored-by: Claude <noreply@anthropic.com> Agent transcript: https://claudescope.sentry.dev/share/h4bwQNRmO_pGUExexTuLJfWl2IejOSHecC7tybPX_zI
pbhandari
approved these changes
Jul 30, 2026
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.

Snuba is about to make long-term-retention downsampling unconditional
(snuba#8235): any EAP query whose window
starts more than 31 days ago is routed to the
TIER_8downsampled table, unless the itemtype is in
ITEM_TYPE_FULL_RETENTION. EAP endpoints default to a 90 day window when a requestomits
statsPeriod/start/end, which is past that boundary.full_retention_window()now anchors to tier 1's 30 day retention. The 90 day default couldnever surface older data anyway; it just guaranteed these context existence checks ran against
a sampled tier and could report an existing attribute as absent.
apply_eap_default_stats_period(), wired into theshared request helpers. Fixtures only land in the tier 1 table, so a downsampled query sees an
arbitrary ~1-in-8 subset of what the test just stored. A few tests that stored 35–80 day old
data were moved inside the window.
Note that
HIGHEST_ACCURACYdoes not rescue these queries: in_update_routing_decisiontheTIER_8assignment happens above the highest-accuracy early-return, so the window itself hasto stay inside the boundary.
The 30 day window leaves as little as a second of headroom, since
adjust_start_end_window()floors the start to midnight and Snuba evaluates the boundary at query time. That margin is
accepted deliberately to keep the full 30 days.
Datasets are derived from
RPC_DATASETSrather than hardcoded so the set can't drift, minusuptime_results/preprodSizeto mirror Snuba'sITEM_TYPE_FULL_RETENTION.I could not run the affected Snuba integration tests locally (no image pull for the Snuba
container), so the reasoning comes from the routing source plus confirming the materialized view
predicate against live ClickHouse, not a green run — worth watching CI here.
_trace_connected_issues_eapinsentry/issues/related/trace_connected.pyhas the same shapeand will also be downsampled; left out to keep this scoped.