Repository navigation
fix(analytics): exclude zero-visitor groups from property breakdown - #183
Merged
Merged
Conversation
get_property_breakdown grouped by referrer_hostname (and other dimensions) via GROUP BY, but COUNT(DISTINCT visitor_id) returns 0 for groups whose events all have visitor_id = NULL. Those groups still passed through GROUP BY, so the Referrers table (and other dimension breakdowns using aggregation_level=visitors/sessions) could show entries with 0 unique visitors — most visibly for spoofed referrer-spam hits sent directly to the ingest endpoint, which never carry a resolvable visitor cookie. Add a HAVING clause so a group only appears once its count is > 0.
Same bug as the TimescaleDB path (previous commit), ClickHouse side: property_breakdown_sql's value_counts CTE grouped by value with no floor on the aggregate, so a referrer_hostname (or other dimension) whose events all have visitor_id/session_id = NULL — uniq() skips NULLs — still passed through GROUP BY with count = 0. Add HAVING count > 0 so those groups never reach the response.
📓 Changelog previewThis is what your commits will add to the generated ## [Unreleased]
### Fixed
- **analytics:** Exclude zero-visitor groups from property breakdown
- **analytics:** Exclude zero-visitor groups from ClickHouse breakdown |
dviejokfs
added a commit
that referenced
this pull request
Jul 2, 2026
Resolves conflicts with #183 (zero-visitor-group breakdown fix), which landed on main after this branch was created. Both PRs independently appended new tests at the end of the same mod tests blocks in clickhouse_backend.rs and events_service.rs -- purely textual conflicts, no logical overlap. Resolved by keeping both additions. Verified after resolution: cargo check, clippy --all-targets, fmt --check, and cargo test --lib -p temps-analytics-events (52/52 passing, live against real TimescaleDB/ClickHouse containers).
dviejokfs
added a commit
that referenced
this pull request
Jul 2, 2026
… no baseline The dashboard "visitors in last 24h" trend badge showed misleading flat +100%/-100% swings, especially right after a server restart, on both analytics backends. - TimescaleDB (events_service.rs): the previous-period visitor count was read from the events_hourly continuous aggregate for speed. That aggregate's older buckets are only backfilled by a best-effort async job on startup, and its recurring policy only refreshes the last 3 hours, so right after a restart it could return no row for a project's previous-period window -- indistinguishable from "genuinely zero visitors". - ClickHouse (clickhouse_backend.rs): no staleness issue (it already read raw events for both periods), but carried an identical hardcoded fallback, explicitly written to mirror the Timescale impl. Both paths fed a missing/zero previous count into a fallback that hardcoded the trend to a flat +100.0 (or -100.0), instead of a real percentage or "no data yet". Fix: - TimescaleDB: query the previous period from the raw events table (same source/index as the current period), eliminating the staleness window entirely. - Both backends: stop fabricating +/-100% when there's no real previous-period baseline -- omit the trend instead. - Extracted the trend math into one shared calculate_trend_percentage(), reused by both backends. - Added testcontainer regression tests (TimescaleDB + ClickHouse), verified live against real containers. - Along the way, fixed the ClickHouse test harness's broken container-readiness wait condition (which had been silently skipping every ClickHouse test in this file), which surfaced and fixed two independent pre-existing bugs: a WITH FILL STEP clause that always failed against real ClickHouse 24.8, and a stale migration-count test assertion. Merged with main's #183 (zero-visitor-group breakdown fix), which landed after this branch was created -- textual conflict only (both PRs appended new tests to the same file locations), no logical overlap.
dviejokfs
added a commit
that referenced
this pull request
Jul 26, 2026
…with (#442) Closes 11 of the 14 open Dependabot alerts; the remaining 3 are upstream-blocked and re-verified unreachable (documented in Cargo.toml). Fixed: - next 16.2.6 -> 16.2.11 in examples/nextjs/basic (alerts #175-#183, 9 alerts: SSRF in Server Actions on custom servers, SSRF via rewrite destination hostname, middleware/proxy bypass with Turbopack + single locale, DoS in Server Actions, cache confusion on invalid-UTF-8 and on request bodies, unbounded Server Action payload on Edge, image optimization SVG DoS, and unauthenticated Server Function endpoint disclosure). Verified: tsc --noEmit clean and `next build` succeeds. - setuptools 82.0.0 -> 83.0.0 in sdks/python/uv.lock (alert #174, MANIFEST.in sdist exclusion bypass via NFC/NFD collision on APFS/HFS+). - serde_with 3.20.0 -> 3.21.0, pulling serde_with_macros 3.21.0 (alert #173, KeyValueMap serialize panic on empty sequence/map entries). Reached via cloudflare 0.14 -> temps-dns. Not fixable, re-verified unreachable (triage refreshed in Cargo.toml): - protobuf 2.28.0 (#4): pingora-core 0.8.1 declares prometheus = "0.13" with default features, and `protobuf` is a DEFAULT feature of prometheus 0.13 -- cargo features are additive, so it cannot be disabled without vendoring a pingora fork. No ProtobufEncoder / TextEncoder / protobuf:: / PrometheusServer reference exists under crates/, so the 2.x parser is never fed input. - opentelemetry_sdk 0.30.0 (#151): pinned via the getsentry/relay git rev. The bug is in the W3C BaggagePropagator; no case-insensitive match for `baggage` exists under crates/. - remove_dir_all 0.5.3 (#1): nixpacks 1.41.0 (still crates.io latest) uses deprecated tempdir, which hard-requires remove_dir_all ^0.5. The TOCTOU branch only runs when out_dir is None, and nixpacks_preset.rs:508 remains the only out_dir site, passing Some(..). - serde_with 2.3.3 also remains via nixpacks and has no fix (2.3.3 is the last 2.x; the fix landed only in 3.21.0). KeyValueMap does exist in 2.3.3, but nixpacks uses serde_with with default-features = false, features = ["macros"], solely for skip_serializing_none. Cargo.lock also re-unifies two entries that any re-resolve surfaces, both within their declared ranges and not regressions: pingora-lru moves to hashbrown 0.14.5 (its req is "0", and ahash/allocator-api2 are 0.14.5 defaults), and iana-time-zone moves to windows-core 0.57.0 (req is ">=0.56, <=0.62"; a cfg(target_os = "windows")-only dep, collapsing a duplicate copy). Verified: cargo check --lib --workspace clean (0 errors); cargo audit vulnerability count unchanged at the 3 known upstream-blocked entries.
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.
Summary
get_property_breakdown(TimescaleDB/Postgres) andproperty_breakdown_sql(ClickHouse) grouped events by dimension value (referrer_hostname, browser, country, ...) with no floor on the aggregate.COUNT(DISTINCT visitor_id)/uniq(visitor_id)returns 0 for a group whose events all havevisitor_id = NULL, butGROUP BYstill emits that group — so the Referrers table (and any other dimension breakdown ataggregation_level=visitors/sessions) could show rows with 0 unique visitors./_temps/eventingest endpoint directly (no real browser session, so no visitor cookie ever gets attached) withreferrerset to whatever domain the bot wants to bait admins with — surfaced as e.g.pornhub.com,msn.com,discord.comwith 0 visitors in a real project's Referrers list.HAVING count > 0to thevalue_countsCTE on both backends so a group only reaches the response once its count is non-zero.Test plan
cargo test --lib -p temps-analytics-events— 46/46 passing, including new regression tests:test_property_breakdown_excludes_zero_visitor_referrers(TimescaleDB, Docker-gatedTestDatabase): inserts a real visitor referred by bing.com + a spam event (spoofed pornhub.com referrer, no visitor) and asserts only the real one is returned.property_breakdown_sql_filters_zero_count_groups(ClickHouse, pure SQL-string unit test): asserts the generated query containsHAVING count > 0.cargo clippy -p temps-analytics-events --all-targets— clean