Skip to content

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

Open
linxia0415 wants to merge 2 commits into
masterfrom
pr-3
Open

feat(upsampling) - Support upsampled error count with performance optimizations#1
linxia0415 wants to merge 2 commits into
masterfrom
pr-3

Conversation

@linxia0415

@linxia0415 linxia0415 commented Jun 4, 2026

Copy link
Copy Markdown

Test 3

Summary by CodeRabbit

  • New Features

    • Added error upsampling support to organization event statistics. When error events are sampled at the client level, the system now applies upsampling transformations to provide more accurate aggregate counts and metrics in dashboards and reports.
  • Tests

    • Added comprehensive test coverage for error upsampling functionality and integration with event statistics endpoints.
  • Chores

    • Updated type-checking configuration for internal modules.

yuvmen and others added 2 commits July 25, 2025 09:48
…(#94376)

Part of the Error Upsampling project:
https://www.notion.so/sentry/Tech-Spec-Error-Up-Sampling-1e58b10e4b5d80af855cf3b992f75894?source=copy_link

Events-stats API will now check if all projects in the query are
allowlisted for upsampling, and convert the count query to a sum over
`sample_weight` in Snuba, this is done by defining a new SnQL function
`upsampled_count()`.

I noticed there are also eps() and epm() functions in use in this
endpoint. I considered (and even worked on) also supporting
swapping eps() and epm() which for correctness should probably also not
count naively and use `sample_weight`, but this
caused some complications and since they are only in use by specific
dashboard widgets and not available in discover
I decided to defer changing them until we realize it is needed.
- Add 60-second cache for upsampling eligibility checks to improve performance
- Separate upsampling eligibility check from query transformation for better optimization
- Remove unnecessary null checks in upsampled_count() function per schema requirements
- Add cache invalidation utilities for configuration management

This improves performance during high-traffic periods by avoiding repeated
expensive allowlist lookups while maintaining data consistency.
@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR implements error upsampling for Sentry's query system by determining which projects should apply upsampled count aggregations, rewriting query columns to use an upsampled_count() function, and integrating these transformations into the organization events stats endpoint with full test coverage.

Changes

Error Upsampling Feature

Layer / File(s) Summary
Error Upsampling Eligibility and Query Transformation
src/sentry/api/helpers/error_upsampling.py
Introduces is_errors_query_for_error_upsampled_projects() with 60-second cached eligibility checks, _are_all_projects_error_upsampled() for allowlist validation, transform_query_columns_for_error_upsampling() to rewrite count() to upsampled_count() as count, and dataset/request-aware _should_apply_sample_weight_transform() logic. Includes invalidate_upsampling_cache() and _is_error_focused_query() helpers.
Snuba Upsampled Count Function
src/sentry/search/events/datasets/discover.py
Registers upsampled_count SnQL function that aggregates via toInt64(sum(sample_weight)) to support error-upsampled query results.
Endpoint Integration
src/sentry/api/endpoints/organization_events_stats.py
Imports upsampling helpers and conditionally transforms query columns across all execution paths (RPC/non-RPC, top-events, timeseries) based on eligibility checks performed early in _get_event_stats(). Updates validation messaging.
Test Event Factory Support
src/sentry/testutils/factories.py
Adds _set_sample_rate_from_error_sampling() to read contexts.error_sampling.client_sample_rate and propagate to sample_rate field in Factories.store_event().
Unit Tests
tests/sentry/api/helpers/test_error_upsampling.py
Tests helper functions: allowlist eligibility, query column transformation with case/whitespace handling, error-focused query detection, and dataset-dependent sample-weight transform decisions.
Integration Tests
tests/snuba/api/endpoints/test_organization_events_stats.py
Tests end-to-end upsampling behavior: confirms upsampled counts when both projects allowlisted, fallback to regular counts for partial/no allowlist, and transaction queries unaffected by upsampling.
Configuration Updates
pyproject.toml, sentry-repo
Adds mypy strictness configuration for new error-upsampling modules; advances sentry-repo submodule for OTLP span-link persistence.

Sequence Diagram

sequenceDiagram
  participant Client as Client Request
  participant Endpoint as OrganizationEventsStatsEndpoint
  participant Helper as error_upsampling helpers
  participant Cache as Eligibility Cache
  participant Snuba as Snuba Query Engine
  
  Client->>Endpoint: GET /organization-events-stats?yAxis=count()
  Endpoint->>Helper: is_errors_query_for_error_upsampled_projects()
  Helper->>Cache: Check cached eligibility (org, project_ids)
  alt Cache Miss
    Helper->>Helper: _are_all_projects_error_upsampled()
    Helper->>Helper: _should_apply_sample_weight_transform()
    Helper->>Cache: Store result (60s TTL)
  end
  Cache-->>Helper: eligibility boolean
  alt Eligible for Upsampling
    Endpoint->>Helper: transform_query_columns_for_error_upsampling()
    Helper-->>Endpoint: [upsampled_count() as count, ...]
    Endpoint->>Snuba: execute with upsampled columns
  else Not Eligible
    Endpoint->>Snuba: execute with original columns
  end
  Snuba-->>Endpoint: timeseries results
  Endpoint-->>Client: JSON response with counts
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A clever query transformation hops into place,
With upsampled counts keeping pace,
Allowlists and caches spring lightly about,
While sample weights transform errors throughout,
Tests validate every hop—no errors allowed! 🎯

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: adding upsampled error count support with performance optimizations, which aligns with the PR's core contribution of a new upsampled_count() function and caching logic.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pr-3
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch pr-3

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/sentry/api/endpoints/organization_events_stats.py (1)

229-271: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Top-events still ranks by raw count().

Only y_axes/timeseries_columns are rewritten here. orderby and the selected top-events fields still come from the unmodified request, so topEvents can choose and bucket series using raw counts while rendering upsampled counts. That gives incorrect top-N ordering whenever sample weights differ across groups.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sentry/api/endpoints/organization_events_stats.py` around lines 229 -
271, The top-N ordering still uses raw count fields so when upsampling is
enabled you must rewrite the orderby and selected top-event fields to the
upsampled equivalents alongside y_axes/timeseries_columns; update the use_rpc
branch (scoped_dataset.run_top_events_timeseries_query) to pass an adjusted
orderby and raw_groupby/selected columns derived from
transform_query_columns_for_error_upsampling (or a mapping from final_columns)
instead of get_orderby(request)/get_field_list(...), and likewise update the
non-RPC call to pass transformed selected_columns and orderby to
scoped_dataset.top_events_timeseries (replace
selected_columns=self.get_field_list(...) and orderby=self.get_orderby(request)
with their upsampled equivalents), ensuring any alias->input-format
transformation is applied consistently so top-N selection uses the same
upsampled metric as the timeseries.
🧹 Nitpick comments (2)
tests/snuba/api/endpoints/test_organization_events_stats.py (1)

3604-3722: ⚡ Quick win

Add one topEvents regression case for upsampled counts.

These tests only cover the plain timeseries path. The endpoint has separate top-events branches, so a topEvents=... case with unequal sample weights would catch the ranking/"Other" regressions there.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/snuba/api/endpoints/test_organization_events_stats.py` around lines
3604 - 3722, Add a new test method (e.g.,
test_error_upsampling_top_events_regression) alongside the existing ones that
patches sentry.api.helpers.error_upsampling.options, sets the allowlist to
include both projects (mock_options.get.return_value = [self.project.id,
self.project2.id]), stores multiple error events with differing sample weights
(via self.store_event using distinct event_ids and sample_weight/context to
create unequal effective counts) across the two time buckets, then calls the
endpoint at self.url with the same start/end/interval/yAxis="count()" and
include topEvents=<N> in the query and project list; assert response.status_code
== 200 and check response.data["data"] for two buckets that the topEvents list
is ranked by upsampled counts and that the "Other" entry (if present) reflects
the sum of upsampled counts for non-top events to catch ranking/"Other"
regressions.
tests/sentry/api/helpers/test_error_upsampling.py (1)

54-76: ⚡ Quick win

Add test cases for column expressions that should NOT be transformed.

The current tests verify that count() is transformed, but don't verify that similar expressions like count(id) or count(distinct id) are left unchanged. Based on the upstream implementation, only exact count() (case-insensitive, after strip) should be transformed.

📝 Suggested test additions
def test_transform_query_columns_for_error_upsampling(self) -> None:
    # ... existing tests ...

    # Test that count with arguments is NOT transformed
    columns = ["count(id)", "count(distinct id)", "count(*)"]
    result = transform_query_columns_for_error_upsampling(columns)
    assert result == columns  # Should remain unchanged

    # Test mixed columns with multiple count()
    columns = ["count()", "other_column", "count()"]
    expected = [
        "upsampled_count() as count",
        "other_column",
        "upsampled_count() as count",
    ]
    assert transform_query_columns_for_error_upsampling(columns) == expected
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/sentry/api/helpers/test_error_upsampling.py` around lines 54 - 76, Add
tests to ensure transform_query_columns_for_error_upsampling only transforms
exact "count()" (case-insensitive after strip) and leaves other count forms
unchanged: add a case with columns = ["count(id)", "count(distinct id)",
"count(*)"] asserting the result equals the original list, and add a mixed case
with ["count()", "other_column", "count()"] asserting the two count() entries
are transformed to "upsampled_count() as count" while other_column remains
unchanged; reference the transform_query_columns_for_error_upsampling function
in the test additions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@sentry-repo`:
- Line 1: This PR mixes an unrelated submodule bump (commit
a5d290951def84afdcc4c88d2f1f20023fc36e2a) that introduces OTLP span-link
ingestion with the error-upsampling changes; separate them by removing the
submodule update from this branch/commit and creating a new PR containing only
the submodule bump (or, if truly required, add an explicit justification and
dependency note to this PR). Concretely: revert or drop the submodule reference
change tied to commit a5d290951def84afdcc4c88d2f1f20023fc36e2a from this branch,
create a new branch/PR that applies just that submodule update, run CI/tests on
both branches, and if you keep the bump here, add a short rationale in the PR
description explaining the hard dependency on OTLP span-link ingestion.

In `@src/sentry/api/endpoints/organization_events_stats.py`:
- Around line 220-222: The upsampling check is using the outer dataset and
request state instead of the effective query/dataset used to call Snuba; update
the call to is_errors_query_for_error_upsampled_projects inside
_get_event_stats() to pass the rewritten/effective values from snuba_params
(e.g., the effective query and dataset stored on snuba_params) and organization
instead of using the outer dataset and request.GET["query"], so eligibility is
evaluated against the actual query sent to Snuba.

In `@src/sentry/api/helpers/error_upsampling.py`:
- Around line 27-38: The cache key uses Python's process-salted hash which is
non-deterministic across workers; replace
hash(tuple(sorted(snuba_params.project_ids))) with a deterministic
representation (e.g., join the sorted project IDs into a string or a stable
digest like hashlib.sha256 of the joined IDs) when building cache_key in the
error upsampling flow (the block that computes cache_key and uses
cache.get/cache.set) and apply the same deterministic construction in the other
occurrence referenced (the lines around the second use). Ensure you keep the
sort to make ordering stable, reference snuba_params.project_ids, and update any
related helpers such as _are_all_projects_error_upsampled and
invalidate_upsampling_cache usage to use the same deterministic key construction
so invalidation works across workers.

In `@src/sentry/search/events/datasets/discover.py`:
- Around line 1041-1051: The SnQLFunction "upsampled_count" currently sets
default_result_type="number" but returns toInt64(sum(sample_weight)); update the
SnQLFunction definition for "upsampled_count" to use an integer result type
(e.g., default_result_type="integer") so its advertised type matches normal
count() responses and downstream metadata consumers remain consistent.

In `@src/sentry/testutils/factories.py`:
- Around line 347-357: The current bare excepts around extracting
client_sample_rate and converting it to float should be replaced with specific
exceptions: when retrieving nested keys from normalized_data (the expression
using normalized_data.get("contexts", {}).get("error_sampling",
{}).get("client_sample_rate")), catch only AttributeError and TypeError (to
handle cases where contexts or error_sampling are not mappings) instead of
Exception; when assigning normalized_data["sample_rate"] =
float(client_sample_rate) catch only ValueError and TypeError (to handle invalid
string/None conversions). Update the try/except blocks around client_sample_rate
and the float conversion accordingly, referencing normalized_data and
client_sample_rate/sample_rate to locate the code.
- Around line 353-357: The truthiness check using "if client_sample_rate:"
prevents valid zero values from being applied; change the guard to an explicit
None check (e.g., "if client_sample_rate is not None:") in the block that sets
normalized_data["sample_rate"], then attempt to coerce client_sample_rate to
float (catching ValueError/TypeError) and assign it to
normalized_data["sample_rate"] so 0 and 0.0 are preserved; reference the
normalized_data dict and the client_sample_rate variable in the sample-rate
assignment logic.

---

Outside diff comments:
In `@src/sentry/api/endpoints/organization_events_stats.py`:
- Around line 229-271: The top-N ordering still uses raw count fields so when
upsampling is enabled you must rewrite the orderby and selected top-event fields
to the upsampled equivalents alongside y_axes/timeseries_columns; update the
use_rpc branch (scoped_dataset.run_top_events_timeseries_query) to pass an
adjusted orderby and raw_groupby/selected columns derived from
transform_query_columns_for_error_upsampling (or a mapping from final_columns)
instead of get_orderby(request)/get_field_list(...), and likewise update the
non-RPC call to pass transformed selected_columns and orderby to
scoped_dataset.top_events_timeseries (replace
selected_columns=self.get_field_list(...) and orderby=self.get_orderby(request)
with their upsampled equivalents), ensuring any alias->input-format
transformation is applied consistently so top-N selection uses the same
upsampled metric as the timeseries.

---

Nitpick comments:
In `@tests/sentry/api/helpers/test_error_upsampling.py`:
- Around line 54-76: Add tests to ensure
transform_query_columns_for_error_upsampling only transforms exact "count()"
(case-insensitive after strip) and leaves other count forms unchanged: add a
case with columns = ["count(id)", "count(distinct id)", "count(*)"] asserting
the result equals the original list, and add a mixed case with ["count()",
"other_column", "count()"] asserting the two count() entries are transformed to
"upsampled_count() as count" while other_column remains unchanged; reference the
transform_query_columns_for_error_upsampling function in the test additions.

In `@tests/snuba/api/endpoints/test_organization_events_stats.py`:
- Around line 3604-3722: Add a new test method (e.g.,
test_error_upsampling_top_events_regression) alongside the existing ones that
patches sentry.api.helpers.error_upsampling.options, sets the allowlist to
include both projects (mock_options.get.return_value = [self.project.id,
self.project2.id]), stores multiple error events with differing sample weights
(via self.store_event using distinct event_ids and sample_weight/context to
create unequal effective counts) across the two time buckets, then calls the
endpoint at self.url with the same start/end/interval/yAxis="count()" and
include topEvents=<N> in the query and project list; assert response.status_code
== 200 and check response.data["data"] for two buckets that the topEvents list
is ranked by upsampled counts and that the "Other" entry (if present) reflects
the sum of upsampled counts for non-top events to catch ranking/"Other"
regressions.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 34418560-94fc-463b-9a87-8341228682c3

📥 Commits

Reviewing files that changed from the base of the PR and between cbf797d and 6ad6fe3.

📒 Files selected for processing (8)
  • pyproject.toml
  • sentry-repo
  • src/sentry/api/endpoints/organization_events_stats.py
  • src/sentry/api/helpers/error_upsampling.py
  • src/sentry/search/events/datasets/discover.py
  • src/sentry/testutils/factories.py
  • tests/sentry/api/helpers/test_error_upsampling.py
  • tests/snuba/api/endpoints/test_organization_events_stats.py

Comment thread sentry-repo
@@ -0,0 +1 @@
Subproject commit a5d290951def84afdcc4c88d2f1f20023fc36e2a

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Split unrelated submodule bump from error-upsampling changes.

This submodule update introduces OTLP span-link ingestion behavior, which is outside the error-upsampling objective. Please move it to a separate PR (or justify hard dependency) to reduce risk and simplify rollback.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@sentry-repo` at line 1, This PR mixes an unrelated submodule bump (commit
a5d290951def84afdcc4c88d2f1f20023fc36e2a) that introduces OTLP span-link
ingestion with the error-upsampling changes; separate them by removing the
submodule update from this branch/commit and creating a new PR containing only
the submodule bump (or, if truly required, add an explicit justification and
dependency note to this PR). Concretely: revert or drop the submodule reference
change tied to commit a5d290951def84afdcc4c88d2f1f20023fc36e2a from this branch,
create a new branch/PR that applies just that submodule update, run CI/tests on
both branches, and if you keep the bump here, add a short rationale in the PR
description explaining the hard dependency on OTLP span-link ingestion.

Comment on lines +220 to +222
should_upsample = is_errors_query_for_error_upsampled_projects(
snuba_params, organization, dataset, request
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Base the upsampling decision on the effective query, not the outer request state.

_get_event_stats() can run with a rewritten dataset/query, but this check still reads the outer dataset plus request.GET["query"]. In the dashboard split paths that means eligibility is evaluated against the original request instead of the query you actually send to Snuba, so upsampling can be skipped or applied incorrectly.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sentry/api/endpoints/organization_events_stats.py` around lines 220 -
222, The upsampling check is using the outer dataset and request state instead
of the effective query/dataset used to call Snuba; update the call to
is_errors_query_for_error_upsampled_projects inside _get_event_stats() to pass
the rewritten/effective values from snuba_params (e.g., the effective query and
dataset stored on snuba_params) and organization instead of using the outer
dataset and request.GET["query"], so eligibility is evaluated against the actual
query sent to Snuba.

Comment on lines +27 to +38
cache_key = f"error_upsampling_eligible:{organization.id}:{hash(tuple(sorted(snuba_params.project_ids)))}"

# Check cache first for performance optimization
cached_result = cache.get(cache_key)
if cached_result is not None:
return cached_result and _should_apply_sample_weight_transform(dataset, request)

# Cache miss - perform fresh allowlist check
is_eligible = _are_all_projects_error_upsampled(snuba_params.project_ids, organization)

# Cache for 60 seconds to improve performance during traffic spikes
cache.set(cache_key, is_eligible, 60)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use a deterministic cache key instead of hash(...).

hash(tuple(sorted(...))) is salted per Python process, so different workers will generate different keys for the same project set. That means invalidate_upsampling_cache() can miss entries written by another worker, leaving stale allowlist decisions around until the TTL expires.

Suggested fix
-    cache_key = f"error_upsampling_eligible:{organization.id}:{hash(tuple(sorted(snuba_params.project_ids)))}"
+    project_key = ",".join(str(project_id) for project_id in sorted(snuba_params.project_ids))
+    cache_key = f"error_upsampling_eligible:{organization.id}:{project_key}"
...
-    cache_key = f"error_upsampling_eligible:{organization_id}:{hash(tuple(sorted(project_ids)))}"
+    project_key = ",".join(str(project_id) for project_id in sorted(project_ids))
+    cache_key = f"error_upsampling_eligible:{organization_id}:{project_key}"

Also applies to: 73-74

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sentry/api/helpers/error_upsampling.py` around lines 27 - 38, The cache
key uses Python's process-salted hash which is non-deterministic across workers;
replace hash(tuple(sorted(snuba_params.project_ids))) with a deterministic
representation (e.g., join the sorted project IDs into a string or a stable
digest like hashlib.sha256 of the joined IDs) when building cache_key in the
error upsampling flow (the block that computes cache_key and uses
cache.get/cache.set) and apply the same deterministic construction in the other
occurrence referenced (the lines around the second use). Ensure you keep the
sort to make ordering stable, reference snuba_params.project_ids, and update any
related helpers such as _are_all_projects_error_upsampled and
invalidate_upsampling_cache usage to use the same deterministic key construction
so invalidation works across workers.

Comment on lines +1041 to +1051
SnQLFunction(
"upsampled_count",
required_args=[],
# Optimized aggregation for error upsampling - assumes sample_weight
# exists for all events in allowlisted projects as per schema design
snql_aggregate=lambda args, alias: Function(
"toInt64",
[Function("sum", [Column("sample_weight")])],
alias,
),
default_result_type="number",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Keep upsampled_count typed as an integer.

This aggregate returns toInt64(sum(sample_weight)), but it is registered as "number". Since the endpoint aliases it back to count, upsampled responses will advertise a different type than normal count(), which can break metadata consumers.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sentry/search/events/datasets/discover.py` around lines 1041 - 1051, The
SnQLFunction "upsampled_count" currently sets default_result_type="number" but
returns toInt64(sum(sample_weight)); update the SnQLFunction definition for
"upsampled_count" to use an integer result type (e.g.,
default_result_type="integer") so its advertised type matches normal count()
responses and downstream metadata consumers remain consistent.

Comment on lines +347 to +357
try:
client_sample_rate = (
normalized_data.get("contexts", {}).get("error_sampling", {}).get("client_sample_rate")
)
except Exception:
pass
if client_sample_rate:
try:
normalized_data["sample_rate"] = float(client_sample_rate)
except Exception:
pass

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Replace bare exception handlers with specific exception types.

The bare except Exception: pass blocks silently swallow all errors, making debugging difficult and potentially hiding real issues. Catch only the specific exceptions expected during dict traversal and type conversion.

🛡️ Proposed fix
     client_sample_rate = None
     try:
         client_sample_rate = (
             normalized_data.get("contexts", {}).get("error_sampling", {}).get("client_sample_rate")
         )
-    except Exception:
+    except (AttributeError, KeyError, TypeError):
         pass
-    if client_sample_rate:
+    if client_sample_rate is not None:
         try:
             normalized_data["sample_rate"] = float(client_sample_rate)
-        except Exception:
+        except (TypeError, ValueError):
             pass
🧰 Tools
🪛 Ruff (0.15.15)

[error] 351-352: try-except-pass detected, consider logging the exception

(S110)


[warning] 351-351: Do not catch blind exception: Exception

(BLE001)


[error] 356-357: try-except-pass detected, consider logging the exception

(S110)


[warning] 356-356: Do not catch blind exception: Exception

(BLE001)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sentry/testutils/factories.py` around lines 347 - 357, The current bare
excepts around extracting client_sample_rate and converting it to float should
be replaced with specific exceptions: when retrieving nested keys from
normalized_data (the expression using normalized_data.get("contexts",
{}).get("error_sampling", {}).get("client_sample_rate")), catch only
AttributeError and TypeError (to handle cases where contexts or error_sampling
are not mappings) instead of Exception; when assigning
normalized_data["sample_rate"] = float(client_sample_rate) catch only ValueError
and TypeError (to handle invalid string/None conversions). Update the try/except
blocks around client_sample_rate and the float conversion accordingly,
referencing normalized_data and client_sample_rate/sample_rate to locate the
code.

Comment on lines +353 to +357
if client_sample_rate:
try:
normalized_data["sample_rate"] = float(client_sample_rate)
except Exception:
pass

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Truthiness check prevents setting sample_rate=0.0.

Line 353's if client_sample_rate: evaluates to False when client_sample_rate is 0 or 0.0, preventing valid zero sample rates from being set. A sample rate of 0.0 is semantically valid (indicates all events should be dropped during sampling).

🔧 Proposed fix
     except Exception:
         pass
-    if client_sample_rate:
+    if client_sample_rate is not None:
         try:
             normalized_data["sample_rate"] = float(client_sample_rate)
         except Exception:
🧰 Tools
🪛 Ruff (0.15.15)

[error] 356-357: try-except-pass detected, consider logging the exception

(S110)


[warning] 356-356: Do not catch blind exception: Exception

(BLE001)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sentry/testutils/factories.py` around lines 353 - 357, The truthiness
check using "if client_sample_rate:" prevents valid zero values from being
applied; change the guard to an explicit None check (e.g., "if
client_sample_rate is not None:") in the block that sets
normalized_data["sample_rate"], then attempt to coerce client_sample_rate to
float (catching ValueError/TypeError) and assign it to
normalized_data["sample_rate"] so 0 and 0.0 are preserved; reference the
normalized_data dict and the client_sample_rate variable in the sample-rate
assignment logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants