Sentry / PR #28

feat(upsampling) - Support upsampled error count with performance optimizations

getsentry/sentry#94376
Golden recall: 100% Extra findings: 14

Golden Comments (3/3 found)

# Comment Severity Status FriendlyReviewer Detail
#1 sample_rate = 0.0 is falsy and skipped LOW ✓ Found "_set_sample_rate_from_error_sampling mutates the normalized payload of every store_event call in the entire test suite, silently swallows exceptions with bare except, and uses a truthiness check that skips the valid value 0.0" (factories.py L344)
#2 Using Python's built-in hash() to build cache keys is non-deterministic across processes (hash randomization), so keys won't match across workers and invalidate_upsampling_cache may fail to delete them LOW ✓ Found "Cache key uses Python's built-in hash() on a tuple, which is randomized per-process (PYTHONHASHSEED) — different workers compute different keys, defeating cache sharing and making invalidate_upsampling_cache() compute a different key, so invalidation silently fails" (error_upsampling.py L25). Note: dataset rates this LOW, FriendlyReviewer rated it High
#3 The upsampling eligibility check passes the outer dataset instead of the actual dataset used by scoped_dataset; in paths where the query ultimately runs against discover (e.g. dashboard split) while the original dataset is metrics, upsampling may be skipped even when all projects are allowlisted MEDIUM ✓ Found "The upsampling eligibility check passes the closure dataset (the original dataset from get()) instead of scoped_dataset (the dataset actually queried). During dashboard discover-split, _get_event_stats is called with different scoped_dataset values, so error-split discover queries will never be upsampled even when they query errors on allowlisted projects" (organization_events_stats.py L220)

Supplementary Findings (14 findings)

Finding File Severity Legitimate?
invalidate_upsampling_cache() is defined but never called — no hook wires it to allowlist changes error_upsampling.py:67 HIGH ✓ Stale eligibility for up to the 60s TTL; combined with the non-deterministic key, invalidation is doubly broken
_is_error_focused_query uses a raw substring match on request.GET["query"] instead of the actual executed query error_upsampling.py:126 HIGH ✓ Mis-classifies mixed queries and the dashboard discover-split transactions-only sub-query
Even after fixing the dataset, discover-split queries are rewritten to ({query}) AND !event.type:transaction, but the transform check still inspects the raw request query organization_events_stats.py:220 HIGH ✓ Same region as golden #3; plain mixed discover queries are never upsampled — complementary, not duplicate
The 4 tests of OrganizationEventsStatsErrorUpsamplingTest share org/project ids and rely on the 60s cache — order-dependent suite test_organization_events_stats.py:3604 HIGH ✓ Later tests read the cached True from the first test and bypass their mocked allowlist
Nothing in the codebase derives or stores sample_weight, yet upsampled_count() = sum(sample_weight) and the tests assert count == 10 — unachievable test_organization_events_stats.py:3579 HIGH ✓ The test suite cannot pass as written and does not exercise upsampling end-to-end
The transform only rewrites the exact literal count() — multi-axis requests (yAxis=count()&yAxis=count_unique(user)) and aliased counts are left un-upsampled error_upsampling.py:89 MEDIUM ✓ Inconsistent weighting within one response
sum(sample_weight) assumes every matching event carries a weight; events without client_sample_rate have none — undercounting error_upsampling.py:94 MEDIUM ✓ Real edge case; needs a mixed sampled/unsampled test
upsampled_count SnQL registration uses default_result_type="number" and wraps sum in toInt64, truncating fractional weights (e.g. sample_rate 1/3 floors to int) discover.py:1041 MEDIUM ✓ Biases the estimate downward; also inconsistent with sibling functions
The unit tests do not cover the caching contract: cache hit/miss, the 60s TTL, the short-circuit, or invalidation deleting the key test_error_upsampling.py:1 MEDIUM ✓ The two cache functions are entirely untested
In the top_events > 0 branch, only timeseries_columns is upsampled; selected_columns and orderby remain raw count() organization_events_stats.py:251 MEDIUM ✓ Top-N rows are ranked by the un-upsampled count while returned values are upsampled — internally inconsistent
The new test class only exercises count() on the discover dataset with two buckets — no comparisonDelta, multi-dataset, discover-split, top-events or RPC coverage test_organization_events_stats.py:3554 MEDIUM ✓ Exactly where the scoped_dataset bug lives — the regressions hide there
The allowlist test uses a blanket options.get mock returning the same value for any key; the organization argument is unused test_error_upsampling.py:33 LOW ✓ Never verifies the implementation reads the right option; the not-allowlisted path is untested
upsampling_enabled = should_upsample is a redundant alias; the RPC transforms are dead code (the check returns False for spans_rpc/ourlogs) organization_events_stats.py:227 LOW ✓ Minor cleanup; misleading but harmless
setUp logs in self.user then reassigns it to a different, never-logged-in user while self.authed_user was captured before test_organization_events_stats.py:3566 LOW ✓ Misleading and fragile test setup; a fairness nit

Honest Analysis

Recall is 100% (3/3) including both Low goldens, but the review is very wide: 14 extra findings beyond the three goldens. Golden #2 (the hash() cache key) was found with the exact invalidation consequence and over-classified: the dataset rates it Low, FriendlyReviewer flagged it High with the precise mechanism (PYTHONHASHSEED randomization, cross-worker key divergence, silently failing invalidation). The over-classification is defensible — a non-deterministic cache key that defeats both sharing and invalidation is a functional defect, not a nit. Golden #3 (closure dataset vs scoped_dataset) matched a finding at the exact same location with the same dashboard discover-split example.

The strongest extras arguably outrank the goldens: invalidate_upsampling_cache() is never called anywhere (L67) — combined with the broken key, allowlist changes are only picked up after the 60s TTL expires, serving stale eligibility; the raw-substring classification of errors-vs-transactions (L126) mis-handles mixed queries; and the new tests assert count == 10 while nothing in the codebase populates sample_weight (L3579) — the suite cannot pass as written and does not validate the feature end-to-end. These are exactly the kind of findings that disappear when a benchmark scores only against its goldens.

The weaknesses are real too. Several findings are layered consequences of one root cause — the eligibility/query-classification cluster around L220/L126/L67 overlaps heavily — and five of the extras are test-quality observations (order-dependence, blanket mocks, self.user reassignment, narrow scenarios) that a human reviewer might compress into one comment. On this PR the wide net produces a lot of noise, even though each individual item is accurate.

One honest caveat about the score: 3/3 here is less impressive than it looks, because two of the three goldens are Low-severity test/implementation details that a review as thorough as this one was almost certain to hit.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech 🌥️ Feat
7 high, 7 medium, 3 low 1 issue

This MR adds support for upsampled error counts with a Redis cache and SnQL transform. However, the review uncovered multiple blocking issues: a non-deterministic cache key (built-in hash()) that breaks cross-worker sharing and makes invalidation unreliable; an invalidate_upsampling_cache() function that is never called; errors-vs-transactions classification that relies on raw substring matching of the request query rather than the actual executed query, which mis-handles dashboard discover-split and mixed queries; a top-events path that up-samples only the timeseries but not the ranking/grouping; and a set of tests that are order-dependent due to cache leakage, assert an upsampled count that cannot be produced from the stored events, and mutate the global store_event test helper. The core functionality is therefore not correctly implemented or validated, and the MR needs significant rework before it can be merged.

These might need a close look

  • 🔴 src/sentry/api/helpers/error_upsampling.py (L25)
    Cache key uses Python's built-in hash() on a tuple, which is randomized per-process (PYTHONHASHSEED). Different workers compute different keys, defeating cache sharing and making invalidate_upsampling_cache() compute a different key than the one used to write, so invalidation silently fails. Use a stable key (e.g. hashlib.sha256 or comma-joined sorted project IDs).
  • 🔴 src/sentry/api/helpers/error_upsampling.py (L67)
    invalidate_upsampling_cache() is defined but never called anywhere; no hook wires it to allowlist changes. Combined with the non-deterministic cache key, allowlist changes are only picked up after the 60s TTL expires, serving stale True/False eligibility. Either wire up invalidation or remove the function.
  • 🔴 src/sentry/api/helpers/error_upsampling.py (L126)
    _is_error_focused_query uses a raw substring match on request.GET["query"] instead of the actual query. This mis-classifies errors vs transactions: mixed queries (e.g. event.type:error or event.type:transaction) are treated as error-focused, collapsing transaction counts; and the dashboard discover-split transactions-only sub-query is upsampled when the original query mentions event.type:error. Use the event search parser and pass the actual query string.
  • 🔴 src/sentry/api/endpoints/organization_events_stats.py (L220)
    The upsampling eligibility check passes the closure dataset (the original dataset from get()) instead of scoped_dataset (the dataset actually queried). During dashboard discover-split, _get_event_stats is called with different scoped_dataset values, so error-split discover queries will never be upsampled even when they query errors on allowlisted projects.
  • 🔴 src/sentry/api/endpoints/organization_events_stats.py (L220)
    Even after fixing the dataset, discover-split error queries are rewritten to ({query}) AND !event.type:transaction, but _should_apply_sample_weight_transform only inspects the raw request.GET["query"]. A plain mixed discover query will be classified as non-error-focused even though the executed query is errors-only. Derive eligibility from the actual query argument passed to _get_event_stats.
  • 🔴 tests/snuba/api/endpoints/test_organization_events_stats.py (L3604)
    All four tests in OrganizationEventsStatsErrorUpsamplingTest share the same org/project ids and rely on the 60s cache. cache.clear() runs once per test class, so later tests read the cached True from the first test and bypass their mocked allowlist, making the suite order-dependent and non-deterministic. Use distinct org/project ids per test, patch the cache, or clear the cache at the start of each test.
  • 🔴 tests/snuba/api/endpoints/test_organization_events_stats.py (L3579)
    The stored events only carry client_sample_rate; nothing in the codebase derives or stores sample_weight on these events, yet upsampled_count() = sum(sample_weight) and the tests assert count == 10. With no sample_weight populated, the assertions cannot pass and do not exercise upsampling end-to-end. Populate sample_weight on stored events or assert against mocked Snuba results.

Worth checking

  • 🟡 src/sentry/api/helpers/error_upsampling.py (L89)
    The transform only rewrites the exact literal count(). Multi-axis requests (e.g. yAxis=count()&yAxis=count_unique(user)) only upsample count(), leaving sibling axes raw, and explicit-alias counts (count() as my_alias) are left un-upsampled. This produces inconsistent weighting within one response. Document the divergence or make it a no-op that raises/logs when non-transformable columns are present.
  • 🟡 src/sentry/api/helpers/error_upsampling.py (L94)
    sum(sample_weight) assumes every matching event carries a weight, but the ingestion helper only sets sample_rate when client_sample_rate is present and truthy. Events without that context have no sample_weight, causing undercounting. Confirm the schema guarantees a non-null sample_weight (e.g. default 1) and add a test for mixed sampled/unsampled events.
  • 🟡 src/sentry/search/events/datasets/discover.py (L1041)
    upsampled_count SnQL registration is structurally consistent and resolvable for the errors dataset, but default_result_type="number" (while count uses "integer") and the aggregate wraps sum in toInt64, which truncates fractional weights (e.g. sample_rate 1/3 silently floors to int), biasing the estimate downward. Also required_args=[] is inconsistent with sibling functions.
  • 🟡 tests/sentry/api/helpers/test_error_upsampling.py (L1)
    The unit tests do not cover the caching contract: is_errors_query_for_error_upsampled_projects and invalidate_upsampling_cache are untested. There is no test for cache hit/miss, the 60s TTL, the short-circuit, the not-eligible→cache-False case, or invalidation deleting the key.
  • 🟡 src/sentry/api/endpoints/organization_events_stats.py (L251)
    In the top_events > 0 branch, only timeseries_columns is upsampled; selected_columns and orderby remain raw count(). Consequently top-N rows are selected/ranked by the un-upsampled count while the returned values are upsampled — internally inconsistent. The RPC top-events branch has the same issue. Either order by the upsampled value or guard top-events from upsampling.
  • 🟡 src/sentry/testutils/factories.py (L344)
    _set_sample_rate_from_error_sampling mutates the normalized payload of every store_event call in the entire test suite (including transactions), silently swallows exceptions with bare except Exception: pass, and uses a truthiness check that skips the valid value 0.0. This is a large blast radius for a change serving only the new upsampling tests; consider a dedicated helper or gating.
  • 🟡 tests/snuba/api/endpoints/test_organization_events_stats.py (L3554)
    The new test class only exercises count() on the discover dataset with two buckets. There is no coverage of comparisonDelta, multi-dataset behavior, the discover-split/dashboard-widget path (where the scoped_dataset bug lives), top-events, the RPC path, or error-vs-discover dataset branches. These scenarios are exactly where regressions will hide.
Small things (take or leave)
  • 🔵 tests/sentry/api/helpers/test_error_upsampling.py (L33)
    The allowlist test uses a blanket options.get mock returning the same value for any key, so it never verifies the implementation reads issues.client_error_sampling.project_allowlist. Also the organization argument to _are_all_projects_error_upsampled is unused, and the 'some projects not in allowlist → False' path is not tested.
  • 🔵 src/sentry/api/endpoints/organization_events_stats.py (L227)
    upsampling_enabled = should_upsample is a redundant alias; the subsequent code only reads upsampling_enabled. The if upsampling_enabled: transform inside the two use_rpc branches is dead code because _should_apply_sample_weight_transform returns False for spans_rpc/ourlogs datasets. Remove the redundant variable and the misleading RPC transforms (also note trailing whitespace on new lines).
  • 🔵 tests/snuba/api/endpoints/test_organization_events_stats.py (L3566)
    setUp calls self.login_as(user=self.user) and then reassigns self.user = self.create_user(), so the subsequent self.user is a different, never-logged-in user while self.authed_user was captured before. This is misleading and fragile; use a dedicated variable (e.g. self.other_user) for the second user.

✅ Feature-level checklist

Not there yet

  • ❌ Support upsampled error count with performance optimizations
    The feature is not reliably supported. Multiple blocking issues prevent correct upsampling: the eligibility decision is based on the wrong dataset (closure vs scoped) and on raw request strings instead of the actual executed query; the Redis cache key is non-deterministic across workers, making invalidation ineffective and defeating performance optimizations; and the tests cannot validate the behavior (order-dependent, unachievable counts).