Repository navigation
fix(analytics): stop fabricating +/-100% dashboard trend when there's no baseline - #184
Merged
Merged
Conversation
…as no baseline The dashboard "visitors in last 24h" trend badge queried the previous period 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. That missing row was indistinguishable from "genuinely zero visitors" and fed into a fallback that hardcoded the trend to a flat +100% (or -100% the other direction), which is what showed up as misleading swings on the projects dashboard. Fix: - Query the previous period from the raw events table (same source and index as the current period) instead of the continuous aggregate, removing the staleness window entirely. - Stop fabricating +/-100% when there's no previous-period baseline; omit the trend instead, consistent with the existing both-zero case. - Extract the trend math into calculate_trend_percentage() and add unit tests locking in the corrected behavior.
📓 Changelog previewThis is what your commits will add to the generated ## [Unreleased]
### Fixed
- **analytics:** Stop fabricating +/-100% trend when previous period has no baseline
- **analytics:** Fix same fabricated +/-100% trend in ClickHouse backend
- **analytics:** Fix ClickHouse testcontainer wait + two live SQL bugs
### Testing
- **analytics:** Add testcontainer regression tests for dashboard trend fix |
clickhouse_backend.rs's query_dashboard_projects had the identical hardcoded +100.0 fallback, explicitly written to mirror the Timescale implementation. The ClickHouse path already queried both periods from raw `events FINAL` (no continuous-aggregate staleness there), but it still fabricated a flat trend whenever the previous period had no baseline. Extract calculate_trend_percentage() to pub(crate) in events_service and reuse it from clickhouse_backend, so both AnalyticsEvents implementations share one corrected, tested trend calculation instead of two copies that can drift.
…nd fix Adds real-container regression coverage for both AnalyticsEvents backends, proving the previous-period/trend fix actually holds end-to-end and not just at the unit-test level: - TimescaleDB (events_service.rs): test_dashboard_projects_analytics_survives_empty_continuous_aggregate runs full migrations but deliberately never calls run_post_migration_backfill, leaving events_hourly empty -- exactly what a freshly restarted server looks like. Asserts the previous-period count and trend still come out correct (reading raw events), and that a brand-new project with no baseline gets no fabricated trend. - ClickHouse (clickhouse_backend.rs): ch_dashboard_trend_omits_fabricated_percentage exercises the same two scenarios against a real ClickHouse container. Hoisted the seed-row struct/builder that setup_clickhouse tests already used out of seed_rows() so this new test can reuse it instead of duplicating the 50-field row type a third time. Both tests follow the existing CLAUDE.md pattern: skip gracefully (never #[ignore]) when Docker is unavailable, matching test_record_event_persists_crawler_flag and setup_clickhouse(). Verified: the TimescaleDB test passes end-to-end against a real timescale/timescaledb-ha container. The ClickHouse test compiles and follows the exact harness of the pre-existing (untouched) ch_backend_full_query_surface test; both currently skip in this dev environment due to Docker container-startup timeouts under heavy concurrent load (confirmed environmental, not test-specific, since the untouched pre-existing CH test times out identically).
Ran the new ClickHouse regression test for real instead of trusting a
compile-only pass, which surfaced that setup_clickhouse()'s wait
condition was broken and had been silently masking failures:
- WaitFor::message_on_stdout("Ready for connections") never matches --
clickhouse-server 24.8 writes that message only to its in-container
log file, never stdout/stderr, so every ClickHouse test in this file
(including the pre-existing ch_backend_full_query_surface) has been
timing out and silently skipping via the Docker-unavailable path,
not actually running against a live container. Switched to
WaitFor::http("/ping") + a non-empty CLICKHOUSE_PASSWORD, the same
fix already applied in temps-otel's clickhouse tests (added
http_wait_plain to this crate's testcontainers dev-dependency to
match).
Making the tests actually run for the first time uncovered two real,
previously-undetected bugs in query_events_timeline and
query_aggregated_buckets:
- Both built `WITH FILL ... STEP toInt64(INTERVAL 1 HOUR)`, which CH
24.8 rejects ("Illegal type IntervalHour of argument of function
toInt64") since Interval values aren't toInt64-castable. bucket_ms
is a numeric (Unix-ms) column, so STEP needs a plain number, not a
cast interval -- added ch_interval_step_ms() alongside the existing
ch_interval() and stepped by the equivalent millisecond width.
- ch_backend_full_query_surface asserted the migration runner skips
exactly 3 migrations on a second run; the list has grown to 6
(0004-0006 added later) and nobody caught the drift because the
assertion was never reached.
Both ClickHouse tests, and the full temps-analytics-events suite (50
tests), now pass live: cargo test --lib -p temps-analytics-events.
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).
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
AnalyticsEventstrait impl):events_service.rs): the previous-period visitor count was read from theevents_hourlycontinuous 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 can return no row for a project's previous-period window. A missing row was indistinguishable from "genuinely zero visitors."clickhouse_backend.rs): no staleness issue (it already queries rawevents FINALfor both periods), but it had the identical hardcoded fallback, explicitly written to mirror the Timescale implementation.+100.0(or-100.0), instead of a real percentage or "no data yet."eventstable (same source/index as the current period), eliminating the staleness window entirely.+/-100%when there's no real previous-period baseline — omit the trend instead (consistent with the existing both-zero case).calculate_trend_percentage()(pub(crate)inevents_service, reused byclickhouse_backend) so the two backends can't drift into inconsistent semantics again.Regression tests (testcontainers, real DB/CH — verified live, not just compiled)
test_dashboard_projects_analytics_survives_empty_continuous_aggregate: runs full migrations against a realtimescale/timescaledb-hacontainer but deliberately never callsrun_post_migration_backfill, leavingevents_hourlycompletely empty — exactly what a freshly restarted server looks like. Asserts the previous-period count and trend still come out correct (read from rawevents), and that a brand-new project with no baseline getsNoneinstead of a fabricated+100%. Passes end-to-end against a live container.ch_dashboard_trend_omits_fabricated_percentage: exercises the same two scenarios against a realclickhouse/clickhouse-servercontainer. Passes end-to-end against a live container.Bonus: fixed the ClickHouse test harness itself, which uncovered two real pre-existing bugs
Actually running the ClickHouse tests (rather than trusting a compile-only pass) surfaced that
setup_clickhouse()'s wait condition was broken and had been silently skipping every ClickHouse test in this file, including the pre-existingch_backend_full_query_surface— it never once ran against a live container:WaitFor::message_on_stdout("Ready for connections")never matches, because clickhouse-server 24.8 writes that message only to its in-container log file, never stdout/stderr. Switched toWaitFor::http("/ping")+ a non-emptyCLICKHOUSE_PASSWORD, the same fix already applied intemps-otel's ClickHouse tests (addedhttp_wait_plainto this crate'stestcontainersdev-dependency to match).With the harness actually running, two previously-undetected live bugs surfaced and are fixed here too:
query_events_timelineandquery_aggregated_bucketsboth builtWITH FILL ... STEP toInt64(INTERVAL 1 HOUR), which CH 24.8 rejects ("Illegal type IntervalHour of argument of function toInt64") — Interval values aren'ttoInt64-castable.bucket_msis a numeric (Unix-ms) column, soSTEPneeds a plain number; addedch_interval_step_ms()alongside the existingch_interval()and step by the equivalent millisecond width.ch_backend_full_query_surfaceasserted the migration runner skips exactly 3 migrations on a second run; the list has grown to 6 (0004-0006 added later) and nobody caught the drift since the assertion was never reached.Test plan
cargo check --lib --tests -p temps-analytics-eventscargo clippy -p temps-analytics-events --all-targets— cleancargo fmt -p temps-analytics-events -- --check— cleancargo test --lib -p temps-analytics-events— 50/50 passed, live, including both new regression tests and the now-repaired pre-existing ClickHouse test, all run against real TimescaleDB/ClickHouse testcontainers (not skipped)