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

[CodingKylo]

Martian Code Review Benchmark PR (mirrored from source #3)

[re-entry-ai]

🛡️ re-entry.ai Code Review

@CodingKylo — Please review the following assessment:

🚨 Risk Score: 76/100 · CRITICAL

Dimension	Level
Likelihood	🔴 Critical
Impact	🔴 Critical
Detectability	🟡 Medium

Intent

Add an optimized “error upsampling” path so organization event stats can report upsampled error counts using sample-weighted aggregation for allowlisted projects.

Summary

The PR introduces a new helper module to decide whether to apply error upsampling, adds a new SnQL aggregation function upsampled_count that computes sum(sample_weight), and wires an early eligibility check into the organization events stats endpoint. The biggest risks are (1) a contract mismatch: upsampled_count assumes sample_weight is present, but the test factory changes appear to inject sample_rate from contexts.error_sampling rather than sample_weight, which could silently yield 0/NULL or runtime query failures. (2) eligibility/gating consistency: the endpoint performs an early cached eligibility decision, while the helper also applies additional gating via _should_apply_sample_weight_transform, so any mismatch can produce inconsistent results across code paths. Please verify end-to-end that the same conditions that make the helper eligible also guarantee sample_weight exists in the Snuba dataset rows and that the query transformation uses the same gating logic everywhere.

🎯 Review Focus

Verify the end-to-end contract for sample_weight: under the exact eligibility conditions used by the endpoint/helper, confirm that Snuba rows include sample_weight and that the generated SnQL uses upsampled_count (sum(sample_weight))—otherwise metrics will be silently wrong or the query will fail.

Key Findings

• 🚨 [src/sentry/search/events/datasets/discover.py:L1038] CRITICAL: upsampled_count hard-depends on sample_weight without any guard/validation — sum(sample_weight) can return NULL/0 or break the query if sample_weight is not present in the dataset for the eligible cases. Quote: snql_aggregate=lambda args, alias: Function( "toInt64", [Function("sum", [Column("sample_weight")])], alias, ), — Why: this is a production metrics endpoint; if sample_weight is missing for some events/projects (or only produced under different normalization than the tests), the endpoint will silently report incorrect counts or fail at query execution. Fix: ensure the dataset always materializes sample_weight when error upsampling is enabled, or add a safe fallback in SnQL (e.g., coalesce(sample_weight, 0)), and add an integration test that asserts the Snuba query includes sample_weight and returns non-zero results for an allowlisted error-focused query. Example SnQL change: Function("sum", [Function("coalesce", [Column("sample_weight"), Literal(0)])]) (or equivalent supported SnQL construct), plus an integration test that fails if sample_weight is absent.
• 🚨 [src/sentry/api/helpers/error_upsampling.py:L18] CRITICAL: cached eligibility can become stale and produce incorrect metrics for up to 60 seconds, and the cache key does not include request-level gating inputs (only org id + project_ids). Quote: cache_key = f"error_upsampling_eligible:{organization.id}:{hash(tuple(sorted(snuba_params.project_ids)))}" and cache.set(cache_key, is_eligible, 60) — Why: the helper returns is_eligible and _should_apply_sample_weight_transform(dataset, request), but the cached value only stores is_eligible. If _should_apply_sample_weight_transform(dataset, request) depends on request parameters (e.g., query shape, time range, dataset type), caching only the allowlist portion is fine only if that request-dependent gating is always recomputed (it is recomputed in the cached branch, but the cache key omission is still a correctness risk if _should_apply_sample_weight_transform ever changes to depend on more than dataset/request in a way not reflected). Fix: either (a) remove caching entirely or (b) include all request-dependent gating inputs in the cache key (or cache only the allowlist check and keep request gating strictly outside the cached value, with a comment + test that proves request gating is always recomputed). Concretely: change the cache to store only _are_all_projects_error_upsampled(...) and never mix it with request gating; add a unit test that varies request query parameters and asserts the final decision changes even when the allowlist cache hits.

✅ Action Checklist

• [ ] CRITICAL [src/sentry/search/events/datasets/discover.py:L1038] — upsampled_count hard-depends on sample_weight without any guard/validation — sum(sample_weight) can return NULL/0 or break the query if sample_weight is not present in the dataset for the eligible cases. Quote: snql_aggregate=lambda args, alias: Function( "toInt64", [Function("sum", [Column("sample_weight")])], alias, ), — Why: this is a production metrics endpoint; if sample_weight is missing for some events/projects (or only produced under different normalization than the tests), the endpoint will silently report incorrect counts or fail at query execution. Fix: ensure the dataset always materializes sample_weight when error upsampling is enabled, or add a safe fallback in SnQL (e.g., coalesce(sample_weight, 0)), and add an integration test that asserts the Snuba query includes sample_weight and returns non-zero results for an allowlisted error-focused query. Example SnQL change: Function("sum", [Function("coalesce", [Column("sample_weight"), Literal(0)])]) (or equivalent supported SnQL construct), plus an integration test that fails if sample_weight is absent.
• [ ] CRITICAL [src/sentry/api/helpers/error_upsampling.py:L18] — cached eligibility can become stale and produce incorrect metrics for up to 60 seconds, and the cache key does not include request-level gating inputs (only org id + project_ids). Quote: cache_key = f"error_upsampling_eligible:{organization.id}:{hash(tuple(sorted(snuba_params.project_ids)))}" and cache.set(cache_key, is_eligible, 60) — Why: the helper returns is_eligible and _should_apply_sample_weight_transform(dataset, request), but the cached value only stores is_eligible. If _should_apply_sample_weight_transform(dataset, request) depends on request parameters (e.g., query shape, time range, dataset type), caching only the allowlist portion is fine only if that request-dependent gating is always recomputed (it is recomputed in the cached branch, but the cache key omission is still a correctness risk if _should_apply_sample_weight_transform ever changes to depend on more than dataset/request in a way not reflected). Fix: either (a) remove caching entirely or (b) include all request-dependent gating inputs in the cache key (or cache only the allowlist check and keep request gating strictly outside the cached value, with a comment + test that proves request gating is always recomputed). Concretely: change the cache to store only _are_all_projects_error_upsampled(...) and never mix it with request gating; add a unit test that varies request query parameters and asserts the final decision changes even when the allowlist cache hits.
• [ ] SUGGESTION — [src/sentry/api/endpoints/organization_events_stats.py:L215] WARNING: endpoint introduces a second “source of truth” for eligibility (should_upsample / upsampling_enabled) but the diff is truncated and it’s unclear whether the later query transformation uses the same gating function as the helper. Quote: should_upsample = is_errors_query_for_error_upsampled_projects(...); upsampling_enabled = should_upsample; final_columns = query_columns — Why: if later code transforms columns based on upsampling_enabled while the helper also applies _should_apply_sample_weight_transform(dataset, request), any mismatch will yield inconsistent results across query-building paths. Fix: centralize gating by having the endpoint call only transform_query_columns_for_error_upsampling(...) and let that function decide internally; remove upsampling_enabled plumbing unless it is strictly redundant. Add a test that asserts the transformed SnQL uses upsampled_count exactly when _should_apply_sample_weight_transform would return true.
• [ ] SUGGESTION — [src/sentry/api/helpers/error_upsampling.py:L44-L74] WARNING: error-focused query detection relies on a brittle substring heuristic ("event.type:error" in request.GET['query']). Quote (from risk evidence): _is_error_focused_query checks only if "event.type:error" substring is present in request.GET['query']. — Why: SnQL query formatting can vary (whitespace, parentheses, aliases, URL encoding, different field names), causing eligibility to be applied/withheld incorrectly and therefore changing metrics semantics. Fix: parse the SnQL query AST (or at least normalize the query string) and detect error-focused intent structurally. If AST parsing isn’t available, implement robust normalization: URL-decoding, whitespace normalization, and case/field-name normalization, plus tests for common query variants.
• [ ] SUGGESTION — [src/sentry/testutils/factories.py:L341-L365] WARNING: test factory injects sample_rate from contexts.error_sampling.client_sample_rate, but the new aggregation uses sample_weight. Quote (from risk evidence): _set_sample_rate_from_error_sampling and calls it from store_event. — Why: if production normalization maps client_sample_rate → sample_weight differently (or not at all), unit tests may pass while production returns 0/NULL. Fix: align test event injection with the real pipeline: ensure the factory produces whatever field the Snuba dataset uses (sample_weight) or add an integration test that asserts sample_weight is present in the Snuba rows for eligible queries.
• [ ] SUGGESTION — [tests/sentry/api/helpers/test_error_upsampling.py:L1-L101] INFO: unit tests appear to import and test private helpers and mock options, which can mask integration failures around Snuba schema/fields. Quote (from risk evidence): tests cover _are_all_projects_error_upsampled, _is_error_focused_query, _should_apply_sample_weight_transform. — Fix: add one end-to-end test that runs the endpoint against Snuba (or a realistic dataset fixture) and asserts the response changes specifically because upsampled_count is used (e.g., verify non-zero and/or verify the generated SnQL contains sum(sample_weight)).

Suggestions

• [src/sentry/api/endpoints/organization_events_stats.py:L215] WARNING: endpoint introduces a second “source of truth” for eligibility (should_upsample / upsampling_enabled) but the diff is truncated and it’s unclear whether the later query transformation uses the same gating function as the helper. Quote: should_upsample = is_errors_query_for_error_upsampled_projects(...); upsampling_enabled = should_upsample; final_columns = query_columns — Why: if later code transforms columns based on upsampling_enabled while the helper also applies _should_apply_sample_weight_transform(dataset, request), any mismatch will yield inconsistent results across query-building paths. Fix: centralize gating by having the endpoint call only transform_query_columns_for_error_upsampling(...) and let that function decide internally; remove upsampling_enabled plumbing unless it is strictly redundant. Add a test that asserts the transformed SnQL uses upsampled_count exactly when _should_apply_sample_weight_transform would return true.
• [src/sentry/api/helpers/error_upsampling.py:L44-L74] WARNING: error-focused query detection relies on a brittle substring heuristic ("event.type:error" in request.GET['query']). Quote (from risk evidence): _is_error_focused_query checks only if "event.type:error" substring is present in request.GET['query']. — Why: SnQL query formatting can vary (whitespace, parentheses, aliases, URL encoding, different field names), causing eligibility to be applied/withheld incorrectly and therefore changing metrics semantics. Fix: parse the SnQL query AST (or at least normalize the query string) and detect error-focused intent structurally. If AST parsing isn’t available, implement robust normalization: URL-decoding, whitespace normalization, and case/field-name normalization, plus tests for common query variants.
• [src/sentry/testutils/factories.py:L341-L365] WARNING: test factory injects sample_rate from contexts.error_sampling.client_sample_rate, but the new aggregation uses sample_weight. Quote (from risk evidence): _set_sample_rate_from_error_sampling and calls it from store_event. — Why: if production normalization maps client_sample_rate → sample_weight differently (or not at all), unit tests may pass while production returns 0/NULL. Fix: align test event injection with the real pipeline: ensure the factory produces whatever field the Snuba dataset uses (sample_weight) or add an integration test that asserts sample_weight is present in the Snuba rows for eligible queries.
• [tests/sentry/api/helpers/test_error_upsampling.py:L1-L101] INFO: unit tests appear to import and test private helpers and mock options, which can mask integration failures around Snuba schema/fields. Quote (from risk evidence): tests cover _are_all_projects_error_upsampled, _is_error_focused_query, _should_apply_sample_weight_transform. — Fix: add one end-to-end test that runs the endpoint against Snuba (or a realistic dataset fixture) and asserts the response changes specifically because upsampled_count is used (e.g., verify non-zero and/or verify the generated SnQL contains sum(sample_weight)).

---

Posted by re-entry.ai · Risk governance for autonomous engineering teams

[re-entry-ai]

🛡️ re-entry.ai Code Review

@CodingKylo — Please review the following assessment:

🚨 Risk Score: 76/100 · CRITICAL

Dimension	Level
Likelihood	🔴 Critical
Impact	🔴 Critical
Detectability	🟡 Medium

Intent

Add an optimized “error upsampling” path so organization event stats can report upsampled error counts for allowlisted projects, using a cached eligibility check and a new SnQL aggregation.

Summary

The PR introduces a new helper module that decides whether to apply error upsampling based on project allowlist membership and a query-string heuristic, then wires that decision into the organization events stats endpoint. It also adds a new SnQL aggregation function upsampled_count implemented as sum(sample_weight) and modifies query-building to use it. The two biggest risks to verify are (1) correctness of the eligibility heuristic and caching key (wrong eligibility will change production metrics), and (2) the hard assumption that sample_weight exists for all events when upsampled_count is used (otherwise queries can error or silently miscount). You should also verify end-to-end wiring: unit tests may not exercise the full event normalization path that produces sample_weight.

🎯 Review Focus

Confirm the end-to-end contract between eligibility decision, query transformation, and the presence of sample_weight: when upsampled_count is used, the query must also guarantee sample_weight exists for every row, and the eligibility cache key must reflect the actual resolved project set for the request.

Key Findings

• 🚨 [src/sentry/api/helpers/error_upsampling.py:L18-L33] CRITICAL: cache_key is derived from snuba_params.project_ids without guaranteeing it is present/stable: hash(tuple(sorted(snuba_params.project_ids))) — If project_ids is missing, empty, or not deterministically populated for a given request, the cache can return eligibility for the wrong project set, causing the endpoint to apply upsampling transformations incorrectly and permanently skew metrics for that request. Fix: compute a deterministic project-id set in the endpoint (or in the helper) from the resolved Snuba dataset query, and fail closed when it’s unavailable. Example: in the helper, guard if not snuba_params.project_ids: return False (or compute from dataset/organization), and include all relevant dimensions in the cache key (e.g., dataset name + resolved project ids).
• 🚨 [src/sentry/search/events/datasets/discover.py:L1038-L1056] CRITICAL: upsampled_count hard-assumes sample_weight exists for all events: Function("sum", [Column("sample_weight")]) — If upstream events lack sample_weight (or the transform gate is wrong), Snuba/ClickHouse can error or return incorrect results (e.g., NULL propagation or missing column behavior), breaking the endpoint or silently corrupting counts. Fix: make the aggregation robust to missing weights, e.g. sum(ifNull(sample_weight, 0)) (or ClickHouse equivalent sum(coalesce(sample_weight, 0))) and/or enforce in transform_query_columns_for_error_upsampling that sample_weight is always produced whenever upsampled_count is used. Concretely, change the lambda to Function("sum", [Function("ifNull", [Column("sample_weight"), Literal(0)])]) (adjust to the SnQL function set available).
• ⚠️ [src/sentry/api/endpoints/organization_events_stats.py:L211-L215] WARNING: eligibility is computed early and then “applied later during query building” but the diff shows no guarantee that the same eligibility decision is used when constructing the final query columns: should_upsample = is_errors_query_for_error_upsampled_projects(...); upsampling_enabled = should_upsample; final_columns = query_columns — If later code paths mutate query_columns/dataset selection or if request.GET['query'] parsing differs from the actual Snuba query, you can end up with upsampled_count being used without the corresponding sample_weight transform (or vice versa), producing wrong metrics or runtime failures. Fix: thread upsampling_enabled explicitly into the exact query transformation function that adds upsampled_count and the sample_weight-producing transform, and add an assertion/test that when upsampled_count is present, the query also includes the sample_weight transform.
• ⚠️ [src/sentry/api/helpers/error_upsampling.py:L44-L74] WARNING: the “error-focused query” heuristic is a substring check on the raw query string: _is_error_focused_query checks only if "event.type:error" is present in request.GET['query'] — This is brittle: equivalent SnQL queries (different spacing/casing/aliases, or using event.type = "error") won’t match, so upsampling may not apply when it should (or apply when the substring appears in an unrelated context). Fix: determine “error-focused” from the parsed Snuba/SnQL AST or from the normalized query representation used to build the dataset, not from the raw request.GET['query'] string. If AST parsing isn’t available, at least normalize (lowercase, strip whitespace) and support common variants (e.g., event.type:error, event.type = error, event.type:"error").

✅ Action Checklist

• [ ] CRITICAL [src/sentry/api/helpers/error_upsampling.py:L18-L33] — cache_key is derived from snuba_params.project_ids without guaranteeing it is present/stable: hash(tuple(sorted(snuba_params.project_ids))) — If project_ids is missing, empty, or not deterministically populated for a given request, the cache can return eligibility for the wrong project set, causing the endpoint to apply upsampling transformations incorrectly and permanently skew metrics for that request. Fix: compute a deterministic project-id set in the endpoint (or in the helper) from the resolved Snuba dataset query, and fail closed when it’s unavailable. Example: in the helper, guard if not snuba_params.project_ids: return False (or compute from dataset/organization), and include all relevant dimensions in the cache key (e.g., dataset name + resolved project ids).
• [ ] CRITICAL [src/sentry/search/events/datasets/discover.py:L1038-L1056] — upsampled_count hard-assumes sample_weight exists for all events: Function("sum", [Column("sample_weight")]) — If upstream events lack sample_weight (or the transform gate is wrong), Snuba/ClickHouse can error or return incorrect results (e.g., NULL propagation or missing column behavior), breaking the endpoint or silently corrupting counts. Fix: make the aggregation robust to missing weights, e.g. sum(ifNull(sample_weight, 0)) (or ClickHouse equivalent sum(coalesce(sample_weight, 0))) and/or enforce in transform_query_columns_for_error_upsampling that sample_weight is always produced whenever upsampled_count is used. Concretely, change the lambda to Function("sum", [Function("ifNull", [Column("sample_weight"), Literal(0)])]) (adjust to the SnQL function set available).
• [ ] WARNING [src/sentry/api/endpoints/organization_events_stats.py:L211-L215] — eligibility is computed early and then “applied later during query building” but the diff shows no guarantee that the same eligibility decision is used when constructing the final query columns: should_upsample = is_errors_query_for_error_upsampled_projects(...); upsampling_enabled = should_upsample; final_columns = query_columns — If later code paths mutate query_columns/dataset selection or if request.GET['query'] parsing differs from the actual Snuba query, you can end up with upsampled_count being used without the corresponding sample_weight transform (or vice versa), producing wrong metrics or runtime failures. Fix: thread upsampling_enabled explicitly into the exact query transformation function that adds upsampled_count and the sample_weight-producing transform, and add an assertion/test that when upsampled_count is present, the query also includes the sample_weight transform.
• [ ] WARNING [src/sentry/api/helpers/error_upsampling.py:L44-L74] — the “error-focused query” heuristic is a substring check on the raw query string: _is_error_focused_query checks only if "event.type:error" is present in request.GET['query'] — This is brittle: equivalent SnQL queries (different spacing/casing/aliases, or using event.type = "error") won’t match, so upsampling may not apply when it should (or apply when the substring appears in an unrelated context). Fix: determine “error-focused” from the parsed Snuba/SnQL AST or from the normalized query representation used to build the dataset, not from the raw request.GET['query'] string. If AST parsing isn’t available, at least normalize (lowercase, strip whitespace) and support common variants (e.g., event.type:error, event.type = error, event.type:"error").
• [ ] SUGGESTION — [src/sentry/api/helpers/error_upsampling.py:L18-L33] Add a “fail closed” guard and a test: if snuba_params.project_ids is empty/None, return False (no upsampling) rather than hashing an empty tuple and caching a potentially wrong eligibility. Then add a unit test for the “projects provided vs projects resolved” scenario to ensure the cache key matches the actual resolved project set.
• [ ] SUGGESTION — [src/sentry/api/helpers/error_upsampling.py] Verify cache invalidation wiring: the helper caches for 60 seconds (cache.set(cache_key, is_eligible, 60)) but the diff doesn’t show invalidate_upsampling_cache being called on allowlist changes. Fix: ensure allowlist update code paths call invalidation, or reduce TTL and/or include an allowlist version in the cache key (e.g., options.get(...).updated_at or a monotonically increasing config revision).
• [ ] SUGGESTION — [src/sentry/api/endpoints/organization_events_stats.py] Add an end-to-end test that stores events and asserts the generated Snuba query uses upsampled_count AND includes the sample_weight transform. Current tests may only validate helper logic and not the full event normalization path that produces sample_weight (see factory changes in src/sentry/testutils/factories.py).
• [ ] SUGGESTION — [src/sentry/api/endpoints/organization_events_stats.py] Run the full tests/sentry/api/endpoints/test_organization_events_stats.py suite and check for brittle assertions around error messages/response shapes. The diff changes the validation detail string ("If topEvents needs to be at least 1" -> "topEvents needs to be at least 1"), which can break unrelated tests if they assert exact strings.

Suggestions

• [src/sentry/api/helpers/error_upsampling.py:L18-L33] Add a “fail closed” guard and a test: if snuba_params.project_ids is empty/None, return False (no upsampling) rather than hashing an empty tuple and caching a potentially wrong eligibility. Then add a unit test for the “projects provided vs projects resolved” scenario to ensure the cache key matches the actual resolved project set.
• [src/sentry/api/helpers/error_upsampling.py] Verify cache invalidation wiring: the helper caches for 60 seconds (cache.set(cache_key, is_eligible, 60)) but the diff doesn’t show invalidate_upsampling_cache being called on allowlist changes. Fix: ensure allowlist update code paths call invalidation, or reduce TTL and/or include an allowlist version in the cache key (e.g., options.get(...).updated_at or a monotonically increasing config revision).
• [src/sentry/api/endpoints/organization_events_stats.py] Add an end-to-end test that stores events and asserts the generated Snuba query uses upsampled_count AND includes the sample_weight transform. Current tests may only validate helper logic and not the full event normalization path that produces sample_weight (see factory changes in src/sentry/testutils/factories.py).
• [src/sentry/api/endpoints/organization_events_stats.py] Run the full tests/sentry/api/endpoints/test_organization_events_stats.py suite and check for brittle assertions around error messages/response shapes. The diff changes the validation detail string ("If topEvents needs to be at least 1" -> "topEvents needs to be at least 1"), which can break unrelated tests if they assert exact strings.

---

Posted by re-entry.ai · Risk governance for autonomous engineering teams

[re-entry-ai]

🛡️ re-entry.ai Code Review

@CodingKylo — Please review the following assessment:

🚨 Risk Score: 76/100 · CRITICAL

Dimension	Level
Likelihood	🔴 Critical
Impact	🔴 Critical
Detectability	🟡 Medium

Intent

Add an optimized “error upsampling” path so organization event stats can report upsampled error counts for allowlisted projects, using a cached eligibility check and a new SnQL aggregation.

Summary

The PR introduces a new helper module that decides whether to apply error upsampling based on project allowlist membership and a query-string heuristic, then wires that decision into the organization events stats endpoint. It also adds a new SnQL aggregation function upsampled_count implemented as sum(sample_weight) and modifies query-building to use it. The two biggest risks to verify are (1) correctness of the eligibility heuristic and caching key (wrong eligibility will change production metrics), and (2) the hard assumption that sample_weight exists for all events when upsampled_count is used (otherwise queries can error or silently miscount). You should also verify end-to-end wiring: unit tests may not exercise the full event normalization path that produces sample_weight.

🎯 Review Focus

Confirm the end-to-end contract between eligibility decision, query transformation, and the presence of sample_weight: when upsampled_count is used, the query must also guarantee sample_weight exists for every row, and the eligibility cache key must reflect the actual resolved project set for the request.

Key Findings

• 🚨 [src/sentry/api/helpers/error_upsampling.py:L18-L33] CRITICAL: cache_key is derived from snuba_params.project_ids without guaranteeing it is present/stable: hash(tuple(sorted(snuba_params.project_ids))) — If project_ids is missing, empty, or not deterministically populated for a given request, the cache can return eligibility for the wrong project set, causing the endpoint to apply upsampling transformations incorrectly and permanently skew metrics for that request. Fix: compute a deterministic project-id set in the endpoint (or in the helper) from the resolved Snuba dataset query, and fail closed when it’s unavailable. Example: in the helper, guard if not snuba_params.project_ids: return False (or compute from dataset/organization), and include all relevant dimensions in the cache key (e.g., dataset name + resolved project ids).
• 🚨 [src/sentry/search/events/datasets/discover.py:L1038-L1056] CRITICAL: upsampled_count hard-assumes sample_weight exists for all events: Function("sum", [Column("sample_weight")]) — If upstream events lack sample_weight (or the transform gate is wrong), Snuba/ClickHouse can error or return incorrect results (e.g., NULL propagation or missing column behavior), breaking the endpoint or silently corrupting counts. Fix: make the aggregation robust to missing weights, e.g. sum(ifNull(sample_weight, 0)) (or ClickHouse equivalent sum(coalesce(sample_weight, 0))) and/or enforce in transform_query_columns_for_error_upsampling that sample_weight is always produced whenever upsampled_count is used. Concretely, change the lambda to Function("sum", [Function("ifNull", [Column("sample_weight"), Literal(0)])]) (adjust to the SnQL function set available).
• ⚠️ [src/sentry/api/endpoints/organization_events_stats.py:L211-L215] WARNING: eligibility is computed early and then “applied later during query building” but the diff shows no guarantee that the same eligibility decision is used when constructing the final query columns: should_upsample = is_errors_query_for_error_upsampled_projects(...); upsampling_enabled = should_upsample; final_columns = query_columns — If later code paths mutate query_columns/dataset selection or if request.GET['query'] parsing differs from the actual Snuba query, you can end up with upsampled_count being used without the corresponding sample_weight transform (or vice versa), producing wrong metrics or runtime failures. Fix: thread upsampling_enabled explicitly into the exact query transformation function that adds upsampled_count and the sample_weight-producing transform, and add an assertion/test that when upsampled_count is present, the query also includes the sample_weight transform.
• ⚠️ [src/sentry/api/helpers/error_upsampling.py:L44-L74] WARNING: the “error-focused query” heuristic is a substring check on the raw query string: _is_error_focused_query checks only if "event.type:error" is present in request.GET['query'] — This is brittle: equivalent SnQL queries (different spacing/casing/aliases, or using event.type = "error") won’t match, so upsampling may not apply when it should (or apply when the substring appears in an unrelated context). Fix: determine “error-focused” from the parsed Snuba/SnQL AST or from the normalized query representation used to build the dataset, not from the raw request.GET['query'] string. If AST parsing isn’t available, at least normalize (lowercase, strip whitespace) and support common variants (e.g., event.type:error, event.type = error, event.type:"error").

✅ Action Checklist

• [ ] CRITICAL [src/sentry/api/helpers/error_upsampling.py:L18-L33] — cache_key is derived from snuba_params.project_ids without guaranteeing it is present/stable: hash(tuple(sorted(snuba_params.project_ids))) — If project_ids is missing, empty, or not deterministically populated for a given request, the cache can return eligibility for the wrong project set, causing the endpoint to apply upsampling transformations incorrectly and permanently skew metrics for that request. Fix: compute a deterministic project-id set in the endpoint (or in the helper) from the resolved Snuba dataset query, and fail closed when it’s unavailable. Example: in the helper, guard if not snuba_params.project_ids: return False (or compute from dataset/organization), and include all relevant dimensions in the cache key (e.g., dataset name + resolved project ids).
• [ ] CRITICAL [src/sentry/search/events/datasets/discover.py:L1038-L1056] — upsampled_count hard-assumes sample_weight exists for all events: Function("sum", [Column("sample_weight")]) — If upstream events lack sample_weight (or the transform gate is wrong), Snuba/ClickHouse can error or return incorrect results (e.g., NULL propagation or missing column behavior), breaking the endpoint or silently corrupting counts. Fix: make the aggregation robust to missing weights, e.g. sum(ifNull(sample_weight, 0)) (or ClickHouse equivalent sum(coalesce(sample_weight, 0))) and/or enforce in transform_query_columns_for_error_upsampling that sample_weight is always produced whenever upsampled_count is used. Concretely, change the lambda to Function("sum", [Function("ifNull", [Column("sample_weight"), Literal(0)])]) (adjust to the SnQL function set available).
• [ ] WARNING [src/sentry/api/endpoints/organization_events_stats.py:L211-L215] — eligibility is computed early and then “applied later during query building” but the diff shows no guarantee that the same eligibility decision is used when constructing the final query columns: should_upsample = is_errors_query_for_error_upsampled_projects(...); upsampling_enabled = should_upsample; final_columns = query_columns — If later code paths mutate query_columns/dataset selection or if request.GET['query'] parsing differs from the actual Snuba query, you can end up with upsampled_count being used without the corresponding sample_weight transform (or vice versa), producing wrong metrics or runtime failures. Fix: thread upsampling_enabled explicitly into the exact query transformation function that adds upsampled_count and the sample_weight-producing transform, and add an assertion/test that when upsampled_count is present, the query also includes the sample_weight transform.
• [ ] WARNING [src/sentry/api/helpers/error_upsampling.py:L44-L74] — the “error-focused query” heuristic is a substring check on the raw query string: _is_error_focused_query checks only if "event.type:error" is present in request.GET['query'] — This is brittle: equivalent SnQL queries (different spacing/casing/aliases, or using event.type = "error") won’t match, so upsampling may not apply when it should (or apply when the substring appears in an unrelated context). Fix: determine “error-focused” from the parsed Snuba/SnQL AST or from the normalized query representation used to build the dataset, not from the raw request.GET['query'] string. If AST parsing isn’t available, at least normalize (lowercase, strip whitespace) and support common variants (e.g., event.type:error, event.type = error, event.type:"error").
• [ ] SUGGESTION — [src/sentry/api/helpers/error_upsampling.py:L18-L33] Add a “fail closed” guard and a test: if snuba_params.project_ids is empty/None, return False (no upsampling) rather than hashing an empty tuple and caching a potentially wrong eligibility. Then add a unit test for the “projects provided vs projects resolved” scenario to ensure the cache key matches the actual resolved project set.
• [ ] SUGGESTION — [src/sentry/api/helpers/error_upsampling.py] Verify cache invalidation wiring: the helper caches for 60 seconds (cache.set(cache_key, is_eligible, 60)) but the diff doesn’t show invalidate_upsampling_cache being called on allowlist changes. Fix: ensure allowlist update code paths call invalidation, or reduce TTL and/or include an allowlist version in the cache key (e.g., options.get(...).updated_at or a monotonically increasing config revision).
• [ ] SUGGESTION — [src/sentry/api/endpoints/organization_events_stats.py] Add an end-to-end test that stores events and asserts the generated Snuba query uses upsampled_count AND includes the sample_weight transform. Current tests may only validate helper logic and not the full event normalization path that produces sample_weight (see factory changes in src/sentry/testutils/factories.py).
• [ ] SUGGESTION — [src/sentry/api/endpoints/organization_events_stats.py] Run the full tests/sentry/api/endpoints/test_organization_events_stats.py suite and check for brittle assertions around error messages/response shapes. The diff changes the validation detail string ("If topEvents needs to be at least 1" -> "topEvents needs to be at least 1"), which can break unrelated tests if they assert exact strings.

Suggestions

• [src/sentry/api/helpers/error_upsampling.py:L18-L33] Add a “fail closed” guard and a test: if snuba_params.project_ids is empty/None, return False (no upsampling) rather than hashing an empty tuple and caching a potentially wrong eligibility. Then add a unit test for the “projects provided vs projects resolved” scenario to ensure the cache key matches the actual resolved project set.
• [src/sentry/api/helpers/error_upsampling.py] Verify cache invalidation wiring: the helper caches for 60 seconds (cache.set(cache_key, is_eligible, 60)) but the diff doesn’t show invalidate_upsampling_cache being called on allowlist changes. Fix: ensure allowlist update code paths call invalidation, or reduce TTL and/or include an allowlist version in the cache key (e.g., options.get(...).updated_at or a monotonically increasing config revision).
• [src/sentry/api/endpoints/organization_events_stats.py] Add an end-to-end test that stores events and asserts the generated Snuba query uses upsampled_count AND includes the sample_weight transform. Current tests may only validate helper logic and not the full event normalization path that produces sample_weight (see factory changes in src/sentry/testutils/factories.py).
• [src/sentry/api/endpoints/organization_events_stats.py] Run the full tests/sentry/api/endpoints/test_organization_events_stats.py suite and check for brittle assertions around error messages/response shapes. The diff changes the validation detail string ("If topEvents needs to be at least 1" -> "topEvents needs to be at least 1"), which can break unrelated tests if they assert exact strings.

---

Posted by re-entry.ai · Risk governance for autonomous engineering teams

[re-entry-ai]

🛡️ re-entry.ai Code Review

🔴 Risk Score: 65/100 · HIGH

Dimension	Level
Likelihood	🟠 High
Impact	🟠 High
Detectability	🟡 Medium

Intent

Add an optimized “error upsampling” path for organization event stats by conditionally transforming Snuba query aggregations to account for sampling weights.

Summary

The PR introduces a new helper module that decides whether to apply error upsampling based on allowlisted projects and whether the request is querying error events, with a 60s cache to avoid repeated option lookups. It also adds a new SnQL aggregation function upsampled_count (implemented as sum(sample_weight)) and wires the endpoint to conditionally transform query columns. The biggest risks are correctness and cache safety: the cache key uses Python’s process-local hash() (non-deterministic across processes/restarts) and the eligibility logic assumes snuba_params.project_ids is always a concrete Sequence[int] with stable types. You must verify that sample_weight is guaranteed to exist whenever upsampled_count is selected, and that the cache key + invalidation logic cannot leave stale eligibility decisions across processes.

🎯 Review Focus

The cache key + eligibility normalization in src/sentry/api/helpers/error_upsampling.py (stable deterministic keying, correct handling of snuba_params.project_ids types, and ensuring upsampled_count is never selected without sample_weight).

✅ Action Checklist

• [ ] SUGGESTION — [src/sentry/api/helpers/error_upsampling.py:L18] Add a unit test that proves cache key stability across processes/restarts (or at least across multiple calls) by asserting the key derived from the same project IDs is identical. Concretely: expose the key helper (or test via cache_key computation) and assert it matches a known sha256 for a fixed input.
• [ ] SUGGESTION — [src/sentry/api/endpoints/organization_events_stats.py:L215] Verify sample_weight existence whenever upsampled_count is selected. The new SnQL aggregation is sum(sample_weight) (in src/sentry/search/events/datasets/discover.py:1038-1060 per risk report), but the endpoint only conditionally transforms columns. Add a guard in transform_query_columns_for_error_upsampling() to ensure the transformation is only applied when the query plan includes sample_weight (or make the aggregation resilient, e.g. sum(coalesce(sample_weight, 1)) if semantics allow).
• [ ] SUGGESTION — [src/sentry/api/helpers/error_upsampling.py:L18] Don’t couple eligibility caching to request-specific query shape unless you can prove it’s invariant. Right now the cached eligibility result is combined with _should_apply_sample_weight_transform(dataset, request) after a cache hit. Add a test for the same org/project set with two different request query formats to ensure the transformation decision changes correctly even when eligibility is cached.

Suggestions

• [src/sentry/api/helpers/error_upsampling.py:L18] Add a unit test that proves cache key stability across processes/restarts (or at least across multiple calls) by asserting the key derived from the same project IDs is identical. Concretely: expose the key helper (or test via cache_key computation) and assert it matches a known sha256 for a fixed input.
• [src/sentry/api/endpoints/organization_events_stats.py:L215] Verify sample_weight existence whenever upsampled_count is selected. The new SnQL aggregation is sum(sample_weight) (in src/sentry/search/events/datasets/discover.py:1038-1060 per risk report), but the endpoint only conditionally transforms columns. Add a guard in transform_query_columns_for_error_upsampling() to ensure the transformation is only applied when the query plan includes sample_weight (or make the aggregation resilient, e.g. sum(coalesce(sample_weight, 1)) if semantics allow).
• [src/sentry/api/helpers/error_upsampling.py:L18] Don’t couple eligibility caching to request-specific query shape unless you can prove it’s invariant. Right now the cached eligibility result is combined with _should_apply_sample_weight_transform(dataset, request) after a cache hit. Add a test for the same org/project set with two different request query formats to ensure the transformation decision changes correctly even when eligibility is cached.

📝 This review includes 4 inline comments (4 warnings)

---

Posted by re-entry.ai · Risk governance for autonomous engineering teams

[re-entry-ai]

⚠️ WARNING

Using Python's built-in hash() to derive the cache key makes the eligibility cache non-deterministic across interpreter restarts and processes. That means the same organization/project set can map to different keys, so cache invalidation in invalidate_upsampling_cache() may miss the entry that was originally written. The root cause is that the cache key is derived from a process-local hash instead of a stable digest of the project IDs.

---

re-entry.ai

[re-entry-ai]

⚠️ WARNING

This helper assumes snuba_params.project_ids is always a concrete sequence of ints, but the call site passes through request-derived search params without validating that contract. If project_ids is None or contains non-comparable/non-int values, sorted(...) and the membership checks here will fail or produce incorrect eligibility decisions. The underlying issue is missing input normalization at the API boundary before caching and allowlist evaluation.

---

re-entry.ai

[re-entry-ai]

⚠️ WARNING

The allowlist is read from options and then compared directly with project_id values, but there is no normalization of either side. If the option is configured as strings or another sequence type, the membership test can silently return false for valid projects, disabling upsampling unexpectedly. This is a correctness issue caused by relying on implicit type matching for configuration data.

---

re-entry.ai

[re-entry-ai]

⚠️ WARNING

invalidate_upsampling_cache() repeats the same unstable hash-based key derivation as the writer. Because Python's hash() is process-local, invalidation may target a different key than the one stored, leaving stale eligibility decisions in cache until TTL expiry. The root cause is the same non-stable cache key scheme used in both write and delete paths.

---

re-entry.ai

[re-entry-ai]

🛡️ re-entry.ai Code Review

@CodingKylo — Please review the following assessment:

🔴 Risk Score: 59/100 · HIGH

Dimension	Level
Likelihood	🟠 High
Impact	🟠 High
Detectability	🟡 Medium

Intent

Add an optimized “error upsampling” path to organization event stats by transforming Snuba aggregations to use sample_weight when querying error events for fully allowlisted projects.

Summary

The change introduces a new helper (error_upsampling.py) that caches allowlist eligibility for 60s and conditionally rewrites Snuba query columns (replacing count() with upsampled_count() as count). It also adds a new Snuba dataset function upsampled_count implemented as sum(sample_weight), and wires the helper into the organization_events_stats endpoint. The highest risks are correctness: hard-coded aliasing can break downstream expectations, and the cache key uses Python’s non-stable hash() plus a cached value that is combined with query-dependent logic, which can yield inconsistent behavior across workers/processes. You should verify alias preservation, cache-key stability, and that sample_weight is guaranteed to exist whenever upsampled_count() is selected.

🎯 Review Focus

Verify correctness of the Snuba query rewrite end-to-end: alias preservation for the rewritten aggregation, stable/correct caching behavior across workers, and guaranteeing sample_weight exists whenever upsampled_count() is used.

✅ Action Checklist

• [ ] SUGGESTION — Fix cache key stability in src/sentry/api/helpers/error_upsampling.py:L20 by replacing hash(tuple(sorted(snuba_params.project_ids))) with a stable hash. Example: projects = ','.join(map(str, sorted(snuba_params.project_ids))); digest = hashlib.sha256(projects.encode()).hexdigest(); cache_key = f'error_upsampling_eligible:{organization.id}:{digest}'.
• [ ] SUGGESTION — Make the cache decision consistent with the final transform decision in src/sentry/api/helpers/error_upsampling.py:L26. Either (a) include the relevant query discriminator(s) (e.g., whether the request query targets error events / contains the same predicate used by _should_apply_sample_weight_transform) in the cache key, or (b) cache only is_eligible and explicitly document/ensure the endpoint never treats the cached value as the final decision.
• [ ] SUGGESTION — Preserve aggregation aliases when rewriting columns in src/sentry/api/helpers/error_upsampling.py:L109. Implement alias extraction from the original column string (e.g., detect as <alias>), and emit upsampled_count() as <original_alias>; add a unit test covering a non-count alias to prevent regressions.
• [ ] SUGGESTION — Add an integration assertion that sample_weight is present/usable whenever upsampled_count() is selected. Given src/sentry/search/events/datasets/discover.py defines upsampled_count as sum(sample_weight), ensure the helper always injects the required sample_weight transform when selecting upsampled_count (or make upsampled_count robust via sum(coalesce(sample_weight, 1)) if that matches intended semantics).
• [ ] SUGGESTION — Reduce test brittleness in tests/sentry/api/helpers/test_error_upsampling.py by asserting mock call arguments for options.get (not just return values). This ensures the helper is reading the expected option keys and defaults, aligning unit tests with endpoint behavior.

Suggestions

• Fix cache key stability in src/sentry/api/helpers/error_upsampling.py:L20 by replacing hash(tuple(sorted(snuba_params.project_ids))) with a stable hash. Example: projects = ','.join(map(str, sorted(snuba_params.project_ids))); digest = hashlib.sha256(projects.encode()).hexdigest(); cache_key = f'error_upsampling_eligible:{organization.id}:{digest}'.
• Make the cache decision consistent with the final transform decision in src/sentry/api/helpers/error_upsampling.py:L26. Either (a) include the relevant query discriminator(s) (e.g., whether the request query targets error events / contains the same predicate used by _should_apply_sample_weight_transform) in the cache key, or (b) cache only is_eligible and explicitly document/ensure the endpoint never treats the cached value as the final decision.
• Preserve aggregation aliases when rewriting columns in src/sentry/api/helpers/error_upsampling.py:L109. Implement alias extraction from the original column string (e.g., detect as <alias>), and emit upsampled_count() as <original_alias>; add a unit test covering a non-count alias to prevent regressions.
• Add an integration assertion that sample_weight is present/usable whenever upsampled_count() is selected. Given src/sentry/search/events/datasets/discover.py defines upsampled_count as sum(sample_weight), ensure the helper always injects the required sample_weight transform when selecting upsampled_count (or make upsampled_count robust via sum(coalesce(sample_weight, 1)) if that matches intended semantics).
• Reduce test brittleness in tests/sentry/api/helpers/test_error_upsampling.py by asserting mock call arguments for options.get (not just return values). This ensures the helper is reading the expected option keys and defaults, aligning unit tests with endpoint behavior.

📝 This review includes 4 inline comments (1 critical, 3 warnings)

---

Posted by re-entry.ai · Risk governance for autonomous engineering teams

[re-entry-ai]

🚨 CRITICAL

Quote: transformed_columns.append("upsampled_count() as count")

Issue: The alias is hard-coded to count. If the original query expected a different alias (e.g., count() as total), this will break downstream consumers expecting total. This is a correctness/data integrity risk for query results.

Fix: Preserve the original alias when present. For example, detect as <alias> in column and reuse it:

# pseudo
alias = extract_alias(column)
transformed_columns.append(f"upsampled_count() as {alias or 'count'}")
``` (see also L109)

---
_[re-entry.ai](https://re-entry.ai)_

[re-entry-ai]

⚠️ WARNING

Quote: from unittest.mock import Mock, patch

Issue: The tests patch options in only one helper test, but other helpers depend on request/query parsing and Snuba dataset selection. Over-mocking can make tests pass even if integration behavior changes (e.g., if options.get is called with different keys/arguments).

Fix: Assert mock call arguments to ensure the helper is using the expected option key(s), e.g.:

mock_options.get.assert_called_with(<expected_key>, <expected_default>)

and/or add integration-style tests that exercise the helpers without patching where feasible. (same pattern in tests/snuba/api/endpoints/test_organization_events_stats.py:L3567, tests/snuba/api/endpoints/test_organization_events_stats.py:L3638)

---

re-entry.ai

[re-entry-ai]

⚠️ WARNING

Quote: cache_key = f"error_upsampling_eligible:{organization.id}:{hash(tuple(sorted(snuba_params.project_ids)))}"

Issue: Using Python's built-in hash() for a cache key is not stable across process restarts (and can vary by hash randomization). This can lead to cache misses after deploys and, more importantly, potential key collisions within a process if the hash space is stressed.

Fix: Use a stable hash (e.g., sha256) or avoid hashing entirely:

import hashlib
...
projects = ",".join(map(str, sorted(snuba_params.project_ids)))
cache_key = f"error_upsampling_eligible:{organization.id}:{hashlib.sha256(projects.encode()).hexdigest()}"

---

re-entry.ai

[re-entry-ai]

⚠️ WARNING

Quote: if cached_result is not None: return cached_result and _should_apply_sample_weight_transform(dataset, request)

Issue: The cached value only represents allowlist eligibility, but the return condition also depends on _should_apply_sample_weight_transform(dataset, request) which inspects request.GET['query']. This means the cache can cause different behavior for different queries within the same 60s window (not necessarily wrong), but it also means the function's output is not purely a function of the cached inputs. If callers assume the cached eligibility implies a stable transformation decision, this can cause semantic inconsistencies.

Fix: Either (a) cache only the eligibility and keep the final decision fully computed (current behavior), but document/ensure callers don't treat the cached result as final; or (b) include the relevant query discriminator(s) in the cache key (e.g., whether the query contains event.type:error).

---

re-entry.ai

[re-entry-ai]

🛡️ re-entry.ai Code Review

@CodingKylo — Please review the following assessment:

🚨 Risk Score: 92/100 · CRITICAL

Dimension	Level
Likelihood	🔴 Critical
Impact	🔴 Critical
Detectability	🟠 High

Intent

Add a cached, allowlist-gated “error upsampling” path that rewrites Snuba aggregations so organization event stats can report sample-weighted error counts.

Summary

Behaviorally, the PR introduces a new helper that decides whether to apply error upsampling based on project allowlisting and whether the request query appears to target error events, then rewrites Snuba query columns by replacing count() with a new upsampled_count() (implemented as sum(sample_weight)). The biggest risks are (1) correctness/contract mismatch: upsampled_count assumes sample_weight exists for the events being aggregated, but the transformation eligibility is request- and dataset-dependent and is cached in a way that can short-circuit needed predicates; and (2) test/fixture masking: test utilities swallow exceptions while setting sample_rate, and endpoint tests inject contexts.error_sampling.client_sample_rate directly, potentially bypassing the real ingestion/normalization contract. Before merge, verify that sample_weight is guaranteed whenever upsampled_count is selected, and that the cache key/predicate logic cannot apply the transformation for the wrong query shape or dataset.

🎯 Review Focus

Verify the end-to-end contract: whenever the query rewrite selects upsampled_count() (i.e., count() is replaced), sample_weight must be present for the aggregated events, and the cached eligibility logic must not apply the rewrite for a different request/query shape than intended.

✅ Action Checklist

• SUGGESTION — Fix the cache key to be stable and collision-resistant. In src/sentry/api/helpers/error_upsampling.py, replace hash(tuple(sorted(snuba_params.project_ids))) with a deterministic string, e.g.:

project_part = ",".join(map(str, sorted(snuba_params.project_ids)))
cache_key = f"error_upsampling_eligible:{organization.id}:{project_part}"

(or use hashlib.sha256(project_part.encode()).hexdigest()).

• SUGGESTION — Refactor the cache usage so request/dataset predicates are never skipped. In src/sentry/api/helpers/error_upsampling.py, change the cache hit path to:

cached_is_eligible = cache.get(cache_key)
if cached_is_eligible is None:
    cached_is_eligible = _are_all_projects_error_upsampled(...)
    cache.set(cache_key, cached_is_eligible, 60)
return bool(cached_is_eligible) and _should_apply_sample_weight_transform(dataset, request)

This ensures _should_apply_sample_weight_transform(dataset, request) is evaluated for every request.

• SUGGESTION — Make upsampled_count null-safe or enforce the contract. Given upsampled_count is sum(sample_weight) (see src/sentry/search/events/datasets/discover.py:1038-1060 per your evidence), ensure either (a) the transformation that materializes sample_weight is always applied whenever upsampled_count is selected, or (b) update the aggregation to tolerate missing weights, e.g. sum(coalesce(sample_weight, 0)) (or SnQL equivalent). Add an assertion/integration test that fails if sample_weight is absent but upsampled_count is used.
• SUGGESTION — Tighten the test utility exception handling and validate bounds. In src/sentry/testutils/factories.py, replace broad except Exception: pass with targeted exceptions and optionally enforce domain constraints (e.g., 0 < sample_rate <= 1) if that’s the intended meaning of client_sample_rate.
• SUGGESTION — Align endpoint tests with the real ingestion/normalization contract. In tests/snuba/api/endpoints/test_organization_events_stats.py, avoid injecting contexts.error_sampling.client_sample_rate in a way that bypasses the factory’s normalization path. Either rely solely on store_event + the factory mapping, or explicitly assert that the normalized field used by Snuba (sample_rate/sample_weight) is present after ingestion.

Suggestions

• Fix the cache key to be stable and collision-resistant. In src/sentry/api/helpers/error_upsampling.py, replace hash(tuple(sorted(snuba_params.project_ids))) with a deterministic string, e.g.:

project_part = ",".join(map(str, sorted(snuba_params.project_ids)))
cache_key = f"error_upsampling_eligible:{organization.id}:{project_part}"

(or use hashlib.sha256(project_part.encode()).hexdigest()).

• Refactor the cache usage so request/dataset predicates are never skipped. In src/sentry/api/helpers/error_upsampling.py, change the cache hit path to:

cached_is_eligible = cache.get(cache_key)
if cached_is_eligible is None:
    cached_is_eligible = _are_all_projects_error_upsampled(...)
    cache.set(cache_key, cached_is_eligible, 60)
return bool(cached_is_eligible) and _should_apply_sample_weight_transform(dataset, request)

This ensures _should_apply_sample_weight_transform(dataset, request) is evaluated for every request.

• Make upsampled_count null-safe or enforce the contract. Given upsampled_count is sum(sample_weight) (see src/sentry/search/events/datasets/discover.py:1038-1060 per your evidence), ensure either (a) the transformation that materializes sample_weight is always applied whenever upsampled_count is selected, or (b) update the aggregation to tolerate missing weights, e.g. sum(coalesce(sample_weight, 0)) (or SnQL equivalent). Add an assertion/integration test that fails if sample_weight is absent but upsampled_count is used.
• Tighten the test utility exception handling and validate bounds. In src/sentry/testutils/factories.py, replace broad except Exception: pass with targeted exceptions and optionally enforce domain constraints (e.g., 0 < sample_rate <= 1) if that’s the intended meaning of client_sample_rate.
• Align endpoint tests with the real ingestion/normalization contract. In tests/snuba/api/endpoints/test_organization_events_stats.py, avoid injecting contexts.error_sampling.client_sample_rate in a way that bypasses the factory’s normalization path. Either rely solely on store_event + the factory mapping, or explicitly assert that the normalized field used by Snuba (sample_rate/sample_weight) is present after ingestion.

📝 This review includes 3 inline comments (3 warnings)

---

Posted by re-entry.ai · Risk governance for autonomous engineering teams

[re-entry-ai]

Issue: Cache key uses hash(tuple(sorted(snuba_params.project_ids))). Python's hash() is randomized per process (and can vary across restarts), so cached entries may be effectively non-deterministic and can also collide more than expected across different inputs.

Fix: Use a stable hash (e.g., join IDs) instead of hash(), for example:

project_part = ",".join(map(str, sorted(snuba_params.project_ids)))
cache_key = f"error_upsampling_eligible:{organization.id}:{project_part}"

(or use hashlib like sha256 over the joined string). (see also L24)

[re-entry-ai]

Issue: Cache stores only is_eligible for 60 seconds, but the final decision also depends on _should_apply_sample_weight_transform(dataset, request). If the request context changes (e.g., different query parameter) within the 60s window, the cached eligibility may be correct but the overall decision still needs to be recomputed. The current implementation recomputes _should_apply_sample_weight_transform only on cache hit when cached_result is truthy; see the short-circuiting behavior.

Fix: Ensure the context predicate is always evaluated on cache hits (see previous finding), and consider caching only the allowlist eligibility while always applying the dataset/request predicate independently.

[re-entry-ai]

Issue: The helper swallows all exceptions when reading normalized_data and when converting client_sample_rate to float, which can hide data-shaping bugs and make tests pass while production behavior differs.

Evidence:

• except Exception:

•    pass
  

...

•    except Exception:
  

•        pass
  

Fix: Catch only expected exceptions (e.g., TypeError, ValueError) and/or add a debug assertion in test utilities. For example:

try:
    client_sample_rate = (
        normalized_data.get("contexts", {})
        .get("error_sampling", {})
        .get("client_sample_rate")
    )
except (AttributeError, TypeError):
    return

if client_sample_rate is not None:
    try:
        normalized_data["sample_rate"] = float(client_sample_rate)
    except (TypeError, ValueError):
        return

Also consider validating bounds (e.g., 0 < sample_rate <= 1) if that’s the intended domain.