safety: gate property-MV route on explicit project allowlist - #358
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Both MVs the router uses filter `profile_id != device_id` on write, so anonymous events are excluded. The identified/anonymous split is project-specific: dashreels showOpen → 99.8% identified (MV complete) shortreels showOpen → 15.7% identified (MV misses 84%) shortreels show1Activated → 2% identified (MV misses 98%) Without a gate, a shortreels filtered/breakdown chart on showOpen would silently under-count by ~6x. Gate on an explicit allowlist so only projects with verified ≥90% identify rates on their hot events route through the MV. Considered a runtime coverage probe (mv_count / events_count over last 7d) cached 24h in Redis. Auto-adapts per project but adds ~60 lines of code + a CH probe on cache miss + Redis dependency in the gate. Allowlist is 2 lines; onboarding a new project is a one-liner after running the diagnostic SQL. Trade-off documented in docs/conversion-chart-perf.md — swap to the probe if the list grows beyond ~10 projects. Fail-closed default: unknown project → falls through to events-table path (correct, sometimes slower).
The three console.log calls in getChartSqlFromPropertyMV matched the existing getChartSqlFromMaterializedView pattern but bloat production logs on every filter/breakdown chart request. CodeRabbit flagged this as a potential PII leak (filter values printed verbatim). Removing here rather than leaving inconsistent with the other builders — those should follow up separately.
ayushjhanwar-png
force-pushed
the
perf/chart-property-mv-safety-allowlist
branch
from
July 7, 2026 12:49
e8f0785 to
b425049
Compare
ayushjhanwar-png
changed the base branch from
perf/chart-property-mv-routing
to
main
July 7, 2026 12:49
4 tasks
3 tasks
ayushjhanwar-png
added a commit
that referenced
this pull request
Jul 16, 2026
) * Revert "perf(db): extend property-MV router to breakdown-by-property charts (redo) (#359)" This reverts commit 8213214. * Revert "safety: gate property-MV route on explicit project allowlist (#358)" This reverts commit 5f9a286. * Revert "perf(db): route event-property-filter charts to profile_event_property_summary_mv (#356)" This reverts commit a1d884f.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #356. Adds a project allowlist gate to
canUsePropertyMVso the MV route only fires for projects with verified ≥90% identified traffic on their hot events.Why this is needed
Both MVs (
profile_event_property_summary_mv,profile_event_summary_mv) filter `profile_id != device_id` on write, so anonymous events are excluded. The identified/anonymous split is project-specific:Without this gate, PR #356 would silently under-count by ~6× on shortreels filtered charts. A `showOpen` breakdown chart on shortreels would show 15.7% of actual events. A `show1Activated` chart would show 2%.
What ships
Two-line hard gate:
```typescript
const MV_ROUTING_ALLOWED_PROJECTS = new Set(['dashreels']);
if (!MV_ROUTING_ALLOWED_PROJECTS.has(projectId)) return false;
```
Unknown projects fall through to the events-table path — always correct, sometimes slower.
Alternative considered
Runtime coverage probe: query MV vs `events_daily_stats` counts over last 7d, cache 24h in Redis per (project, event). Auto-adapts as new projects onboard. Rejected because:
If the allowlist grows unwieldy (10+ projects, frequent additions), swap to the probe. Shape documented in `docs/conversion-chart-perf.md`.
Verified locally against prod CH
Same-shape chart on both projects (`showOpen` filtered by `properties.source`):
Onboarding a new project
Run the diagnostic SQL in `docs/conversion-chart-perf.md` per hot event. If `pct_identified ≥ 90`, add the project to `MV_ROUTING_ALLOWED_PROJECTS`.
Base branch
Based on `perf/chart-property-mv-routing` (#356). When #356 merges, this PR's base auto-updates to `main`. Cleaner review boundary vs. bundling the guard into #356 or #357.
Related
Test plan