Benchmark PR 3 - #7
celmis-codereviewer wants to merge 2 commits into
Conversation
…(#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.
celmis-codereviewer
left a comment
celmis-codereviewer
left a comment
There was a problem hiding this comment.
💬 COMMENT — findings to consider
Full findings and scope are in the review summary comment on this pull request — one persistent comment, updated in place on every run.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
celmis-codereviewer
left a comment
celmis-codereviewer
left a comment
There was a problem hiding this comment.
💬 COMMENT — findings to consider
Full findings and scope are in the review summary comment on this pull request — one persistent comment, updated in place on every run.
| expensive repeated option lookups during high-traffic periods. This is safe | ||
| because allowlist changes are infrequent and eventual consistency is acceptable. | ||
| """ | ||
| cache_key = f"error_upsampling_eligible:{organization.id}:{hash(tuple(sorted(snuba_params.project_ids)))}" |
There was a problem hiding this comment.
Why: Python's built-in hash() function uses per-process seed randomization, causing cache_key on line 27 in src/sentry/api/helpers/error_upsampling.py to evaluate to different values across worker processes for the same inputs and fail shared cache lookups and invalidations.
🟠 Use of process-randomized hash() in cross-process cache key
Python's built-in hash() function uses process-level seed randomization (PYTHONHASHSEED), meaning hash(tuple(sorted(...))) produces different integer results in different Python process instances. Because Django's cache (such as Redis or Memcached) is shared across process boundaries (e.g., Gunicorn/uWSGI workers or Celery tasks), cache entries set by one worker process under a key constructed with hash() cannot be retrieved by another worker process handling subsequent requests for the same organization and project IDs. Furthermore, calls to invalidate_upsampling_cache (line 73) in a different process will construct a different key and fail to delete existing cache entries.
| cache_key = f"error_upsampling_eligible:{organization.id}:{hash(tuple(sorted(snuba_params.project_ids)))}" | |
| cache_key = f"error_upsampling_eligible:{organization.id}:{','.join(str(p) for p in sorted(snuba_params.project_ids))}" |
agent: defect · rule: defect.non-deterministic-cache-key · confidence: 0.95
| client_sample_rate = ( | ||
| normalized_data.get("contexts", {}).get("error_sampling", {}).get("client_sample_rate") | ||
| ) | ||
| except Exception: |
There was a problem hiding this comment.
🟡 Empty except clause silently swallows errors
Catching an exception with only pass hides bugs and makes debugging harder. At minimum log the exception or re-raise.
except Exception: ⏎ pass
Also at line 356.
| except Exception: | |
| logger.exception('...') or raise |
agent: structural · rule: structural.py.empty-except · confidence: 1.00
🤖 Code Review for PR #7💬 COMMENT — findings to consider Findings
Scope
Performance
Powered by Code Analyzer · context: tree-sitter graph + structural, cve, contract, security, defect |
2 participants
Benchmark reproduction of ai-code-review-evaluation#3