Skip to content

fix(eap): Keep EAP queries inside the full-fidelity retention window - #120872

Open
phacops wants to merge 5 commits into
masterfrom
fix/eap-full-fidelity-query-window
Open

fix(eap): Keep EAP queries inside the full-fidelity retention window#120872
phacops wants to merge 5 commits into
masterfrom
fix/eap-full-fidelity-query-window

Conversation

@phacops

@phacops phacops commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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_8 downsampled table, unless the item
type is in ITEM_TYPE_FULL_RETENTION. EAP endpoints default to a 90 day window when a request
omits statsPeriod/start/end, which is past that boundary.

  • full_retention_window() now anchors to tier 1's 30 day retention. The 90 day default could
    never surface older data anyway; it just guaranteed these context existence checks ran against
    a sampled tier and could report an existing attribute as absent.
  • Tests get the same window by default via apply_eap_default_stats_period(), wired into the
    shared 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_ACCURACY does not rescue these queries: in _update_routing_decision the
TIER_8 assignment happens above the highest-accuracy early-return, so the window itself has
to 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_DATASETS rather than hardcoded so the set can't drift, minus
uptime_results/preprodSize to mirror Snuba's ITEM_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_eap in sentry/issues/related/trace_connected.py has the same shape
and will also be downsampled; left out to keep this scoped.

phacops added 2 commits July 29, 2026 18:52
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.
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 30, 2026
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
phacops marked this pull request as ready for review July 30, 2026 15:48
@phacops
phacops requested review from a team as code owners July 30, 2026 15:48

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ 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.

Comment thread src/sentry/api/endpoints/organization_trace_item_attributes.py
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.
@k-fish

k-fish commented Jul 30, 2026

Copy link
Copy Markdown
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?

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants