Sentry PR #3 — Live Benchmark — FriendlyReviewer

Golden Comments (2/3 found)

#	Comment	Severity	Status	FriendlyReviewer Detail
#1	`sample_rate = 0.0` is falsy and skipped	LOW	× Missed	FriendlyReviewer identified the `sample_weight` factory issue but did not flag the specific `0.0 is falsy` edge case.
#2	Python `hash()` non-deterministic for cache keys (hash randomization)	LOW	✓ Found	FriendlyReviewer accurately described PYTHONHASHSEED and proposed a fix using `hashlib.sha256`.
#3	Upsampling eligibility check uses outer `dataset` instead of `scoped_dataset`	MEDIUM	✓ Found	FriendlyReviewer identified the exact closure variable bug with clear documentation; severity upgraded.

Supplementary Findings (9 findings)

Finding	File	Severity	Legitimate?
`sample_weight` never set in factory nor ingestion pipeline	test_organization_events_stats.py:3629, factories.py:352	HIGH	✓ Real bug — test expects `sum(sample_weight)` but column does not exist
`is_errors_query_for_error_upsampled_projects` with zero direct test coverage	test_error_upsampling.py:42	HIGH	✓ No coverage of the public function + cache logic
`transform_query_columns` only handles `count()` — other aggregations ignored	error_upsampling.py:86	MEDIUM	✓ Undocumented functional limitation
`invalidate_upsampling_cache` defined but never called	error_upsampling.py:97	LOW	✓ Dead code
Variable `should_upsample` assigned then immediately overwritten	organization_events_stats.py:219	LOW	✓ Dead code
`final_columns = query_columns` fragile reference alias	organization_events_stats.py:222	LOW	✓ Silent mutation risk
Side-effect import in `_should_apply_sample_weight_transform`	error_upsampling.py:61	LOW	✓ Import inside conditional block
Test `test_error_upsampling_with_transaction_events` misleading name	test_organization_events_stats.py:3707	LOW	✓ Missing explicit `dataset:transactions` case
`self.user` overwrite after `super().setUp()` — fragile	test_organization_events_stats.py:3574	LOW	✓ Better to use `self.other_user`

Honest Analysis

FriendlyReviewer caught golden bugs #2 and #3, including upgrading #3's severity. Golden #1 (sample_rate=0.0 is falsy) was missed — it's a minor edge case, but still a miss.

The sample_weight discovery is genuinely valuable: the column is missing from both factory and ingestion pipeline, so all upsampling integration tests are broken at runtime. That said, this finding comes from reading the test file's assertion logic, not from deep semantic understanding — the test literally calls sum(sample_weight) on a column that doesn't exist. Any reviewer reading the diff would notice it.

The 2 HIGH supplementary findings are real and legitimate. The MEDIUM one (limited aggregation support) is accurate but documents a known design constraint rather than a bug. The 6 LOW findings are a mixed bag: dead code and variable shadowing are fine catches; the "side-effect import" and "misleading test name" are closer to noise. A human reviewer would probably not mention them. LOW findings are excluded from the count to avoid inflating the coverage tally — the 8 non-LOW findings reflect real-value additions more honestly.

Overall: 2/3 golden recall is decent. The real value is the sample_weight catch, which happens to be more critical than any individual golden comment. The rest is a wide net with diminishing returns at the LOW end.

FriendlyReviewer's full review

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

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech
4 high, 2 medium, 6 low

This merge request introduces error upsampling support for the organization_events_stats endpoint. The overall architecture is logical, but several critical issues must be resolved before merging. The sample_weight column used in the upsampling query is never populated in either the production ingestion pipeline or test fixtures, which will cause integration tests to fail. There is also a closure variable bug where the upsampling eligibility check uses the outer dataset variable instead of the scoped_dataset parameter, leading to incorrect behavior in widget-split paths. Cache keys rely on Python's non‑deterministic hash(), making them inconsistent across workers. The public orchestration function is_errors_query_for_error_upsampled_projects has no direct test coverage, and several utility functions are either dead code or have side‑effects. Additionally, the upsampling transformation only handles count(); other aggregate functions are silently skipped. These issues should be addressed to ensure correctness, testability, and maintainability.

These might need a close look

🔴 tests/snuba/api/endpoints/test_organization_events_stats.py (L3629)
The test expects sum(sample_weight) to produce a known value, but sample_weight is never set in test data or in the ingestion pipeline. The factory sets sample_rate but not sample_weight. This will cause the test to fail because the column is either missing or NULL.
🔴 src/sentry/api/endpoints/organization_events_stats.py (L218)
The upsampling eligibility check uses the outer dataset closure variable instead of the function parameter scoped_dataset. When scoped_dataset differs (e.g., in widget‑split paths), the wrong dataset is used for the upsampling decision. Fix: use scoped_dataset.
🔴 tests/sentry/api/helpers/test_error_upsampling.py (L42)
The public function is_errors_query_for_error_upsampled_projects is never tested directly. Only private helpers are tested. The caching logic, cache key construction, and interaction between cache hit/miss have zero test coverage.
🔴 src/sentry/testutils/factories.py (L352)
The factory sets sample_rate but not sample_weight. The upsampling query uses sum(sample_weight). The factory must also set sample_weight = int(1 / client_sample_rate) on normalized data, or the ingestion pipeline must compute it.

Worth checking

🟡 src/sentry/api/helpers/error_upsampling.py (L50)
Cache keys use Python's built‑in hash(), which is randomized across interpreter restarts (PYTHONHASHSEED). This leads to cache misses when multiple workers share a distributed cache. Use a deterministic hash (e.g., hashlib.sha256) or include the sorted project IDs directly in the key string.
🟡 src/sentry/api/helpers/error_upsampling.py (L86)
The transform_query_columns_for_error_upsampling function only transforms count(). Other aggregations like count_unique, count_if, etc. are silently left untransformed. Either document this limitation or add support for them.

Small things (take or leave)

🔵 src/sentry/api/helpers/error_upsampling.py (L97)
The invalidate_upsampling_cache function is defined but never called. Either wire it into option change callbacks or remove it to avoid dead code.
🔵 src/sentry/api/endpoints/organization_events_stats.py (L219)
The variable should_upsample is assigned and then immediately overwritten by upsampling_enabled. Remove one of the assignments to avoid confusion.
🔵 src/sentry/api/endpoints/organization_events_stats.py (L222)
final_columns = query_columns creates a reference alias, not a copy. Although current code returns new lists, this pattern is fragile. Use final_columns = list(query_columns) for safety.
🔵 src/sentry/api/helpers/error_upsampling.py (L61)
The _should_apply_sample_weight_transform function has a side‑effect import (transactions) inside a conditional branch. Move imports to the top of the function or use consistent lazy imports.
🔵 tests/snuba/api/endpoints/test_organization_events_stats.py (L3707)
Test test_error_upsampling_with_transaction_events uses dataset discover with event.type:transaction; it correctly expects no upsampling but its name is misleading. Add an explicit case with dataset:'transactions' to confirm unconditional skip.
🔵 tests/snuba/api/endpoints/test_organization_events_stats.py (L3574)
The test overwrites self.user after super().setUp() has already used it for login_as. This works but is fragile; rename to self.other_user to avoid shadowing.

feat(upsampling) — Support upsampled error count