diff --git a/.depot/workflows/ci-backend.yml b/.depot/workflows/ci-backend.yml index 06faf1db67b0..cc182e96a2f9 100644 --- a/.depot/workflows/ci-backend.yml +++ b/.depot/workflows/ci-backend.yml @@ -60,6 +60,11 @@ # shards fall back to the union .test_durations # - actions/cache reads/writes route to Depot Cache, not GitHub Actions Cache, # so shared keys with canonical (schema, uv, pnpm) do not collide +# - hypothesis constants cache: the restore and save steps mirror canonical verbatim, but the +# save only fires on master test-matrix runs and the shadow skips those, so the Depot Cache +# entry only exists after a master workflow_dispatch. Sampled PR shards run cold (they rebuild +# the pool, as canonical did before this cache existed) until then: bounded, documented, and +# harmless because the shadow never gates merges. # - GITHUB_TOKEN / OTEL_SERVICE_NAME neutralized (ambient on Depot, not on GHA) # - Three data-modeling tests deselected (Depot-only MinIO 403 quarantine) # - COMPOSE_PROJECT_NAME pinned to posthog because bin/wait-for-docker filters by that @@ -135,6 +140,10 @@ env: # and HEAD (push) key computations below can't drift. # ci-e2e-playwright.yml, ci-dagster.yml, ci-mcp.yml and ci-rust-flags-integration.yml restore by the same key; keep their copies in sync when bumping. SCHEMA_CACHE_EPOCH: v2 + # Hypothesis constants cache epoch. Bump to abandon every shared entry at once + # (key is posthog-hypothesis-constants----); used by + # the Django test shards below. + HYPOTHESIS_CONSTANTS_EPOCH: v1 SECRET_KEY: '6b01eee4f945ca25045b5aab440b953461faf08693a9abbf1166dc7c6b9772da' # unsafe - for testing only COMPOSE_PROJECT_NAME: posthog DATABASE_URL: 'postgres://posthog:posthog@localhost:5432/posthog' @@ -2005,6 +2014,37 @@ jobs: key: posthog-segment-durations-${{ github.run_id }} restore-keys: | posthog-segment-durations- + - name: Compute hypothesis constants cache key + # hypothesis builds its constants pool of property-test inputs by + # AST-parsing every local module in sys.modules. It caches the result + # in .hypothesis/constants under a hash of each source file, so the + # pool is content-addressed: restoring a stale copy is safe, because a + # changed file reads as a miss and rebuilds. CI otherwise pays the full + # parse on every shard's pytest collection (about 10 s per shard). + id: hyp-constants-key + shell: bash + run: | + # The key includes the installed hypothesis version because the + # entry format is an implementation detail, and rotates weekly so + # entries for deleted or rewritten files expire. The restore prefix + # covers the gap until the first master run of a new week saves. + hyp_version=$(python -c "import hypothesis; print(hypothesis.__version__)") + prefix="posthog-hypothesis-constants-${hyp_version}-${HYPOTHESIS_CONSTANTS_EPOCH}-" + { + echo "key=${prefix}$(date -u +%G-%V)" + echo "restore_prefix=${prefix}" + } >> "$GITHUB_OUTPUT" + - name: Restore hypothesis constants cache + id: hyp-constants + uses: actions/cache/restore@cdf6c1fa76f9f475f3d7449005a359c84ca0f306 # v5.0.3 + with: + # Constants only. Do NOT add .hypothesis/examples: the example + # database decides which inputs property tests replay, so reusing + # one across runs changes what the tests check. + path: .hypothesis/constants + key: ${{ steps.hyp-constants-key.outputs.key }} + restore-keys: | + ${{ steps.hyp-constants-key.outputs.restore_prefix }} # Tests - name: Set snapshot update flags if: ${{ needs.changes.outputs.backend == 'true' && (needs.detect-snapshot-mode.outputs.mode == 'update' || matrix.person-on-events) }} @@ -2261,6 +2301,27 @@ jobs: else exit $exit_code fi + - name: Save hypothesis constants cache + # One Core shard per master run writes the weekly entry: a save from + # every shard would race on the same key and churn the shared cache + # budget. cache-hit != 'true' skips the save once this week's entry + # exists, which keeps later master runs cheap. + # continue-on-error: overlapping master runs at week rollover fail its + # reservation check when an earlier run has saved the same key, and a + # cache miss must never red a green run. + continue-on-error: true + if: | + github.ref == 'refs/heads/master' && + matrix.segment == 'Core' && + matrix.group == 1 && + !matrix.person-on-events && + !matrix.new-events-schema && + !matrix.compat && + steps.hyp-constants.outputs.cache-hit != 'true' + uses: actions/cache/save@cdf6c1fa76f9f475f3d7449005a359c84ca0f306 # v5.0.3 + with: + path: .hypothesis/constants + key: ${{ steps.hyp-constants-key.outputs.key }} # Post tests - name: Show docker compose logs on failure if: failure() && (needs.changes.outputs.backend == 'true' && steps.run-core-tests.outcome != 'failure' && steps.run-temporal-tests.outcome != 'failure') diff --git a/.github/workflows/ci-backend.yml b/.github/workflows/ci-backend.yml index c457c08bc257..debe189fa398 100644 --- a/.github/workflows/ci-backend.yml +++ b/.github/workflows/ci-backend.yml @@ -46,6 +46,10 @@ env: # between the merge-base (PR) and HEAD (push) key computations below. # ci-e2e-playwright.yml, ci-dagster.yml, ci-mcp.yml and ci-rust-flags-integration.yml restore by the same key; keep their copies in sync when bumping. SCHEMA_CACHE_EPOCH: v2 + # Hypothesis constants cache epoch. Bump to abandon every shared entry at once + # (key is posthog-hypothesis-constants----); used by + # the Django test shards below. + HYPOTHESIS_CONSTANTS_EPOCH: v1 SECRET_KEY: '6b01eee4f945ca25045b5aab440b953461faf08693a9abbf1166dc7c6b9772da' # unsafe - for testing only DATABASE_URL: 'postgres://posthog:posthog@localhost:5432/posthog' REDIS_URL: 'redis://localhost' @@ -2970,6 +2974,39 @@ jobs: restore-keys: | posthog-segment-durations- + - name: Compute hypothesis constants cache key + # hypothesis builds its constants pool of property-test inputs by + # AST-parsing every local module in sys.modules. It caches the result + # in .hypothesis/constants under a hash of each source file, so the + # pool is content-addressed: restoring a stale copy is safe, because a + # changed file reads as a miss and rebuilds. CI otherwise pays the full + # parse on every shard's pytest collection (about 10 s per shard). + id: hyp-constants-key + shell: bash + run: | + # The key includes the installed hypothesis version because the + # entry format is an implementation detail, and rotates weekly so + # entries for deleted or rewritten files expire. The restore prefix + # covers the gap until the first master run of a new week saves. + hyp_version=$(python -c "import hypothesis; print(hypothesis.__version__)") + prefix="posthog-hypothesis-constants-${hyp_version}-${HYPOTHESIS_CONSTANTS_EPOCH}-" + { + echo "key=${prefix}$(date -u +%G-%V)" + echo "restore_prefix=${prefix}" + } >> "$GITHUB_OUTPUT" + + - name: Restore hypothesis constants cache + id: hyp-constants + uses: actions/cache/restore@cdf6c1fa76f9f475f3d7449005a359c84ca0f306 # v5.0.3 + with: + # Constants only. Do NOT add .hypothesis/examples: the example + # database decides which inputs property tests replay, so reusing + # one across runs changes what the tests check. + path: .hypothesis/constants + key: ${{ steps.hyp-constants-key.outputs.key }} + restore-keys: | + ${{ steps.hyp-constants-key.outputs.restore_prefix }} + - name: Download the run's sharding plan snapshot # Pin every attempt to the plan turbo-discover snapshotted (rationale on # that step); a missing snapshot falls back to the floating caches above. @@ -3326,6 +3363,28 @@ jobs: exit $exit_code fi + - name: Save hypothesis constants cache + # One Core shard per master run writes the weekly entry: a save from + # every shard would race on the same key and churn the shared cache + # budget. cache-hit != 'true' skips the save once this week's entry + # exists, which keeps later master runs cheap. + # continue-on-error: overlapping master runs at week rollover fail its + # reservation check when an earlier run has saved the same key, and a + # cache miss must never red a green run. + continue-on-error: true + if: | + github.ref == 'refs/heads/master' && + matrix.segment == 'Core' && + matrix.group == 1 && + !matrix.person-on-events && + !matrix.new-events-schema && + !matrix.compat && + steps.hyp-constants.outputs.cache-hit != 'true' + uses: actions/cache/save@cdf6c1fa76f9f475f3d7449005a359c84ca0f306 # v5.0.3 + with: + path: .hypothesis/constants + key: ${{ steps.hyp-constants-key.outputs.key }} + # Post tests - name: Show docker compose logs on failure if: failure() && (needs.changes.outputs.backend == 'true' && steps.run-core-tests.outcome != 'failure' && steps.run-temporal-tests.outcome != 'failure') diff --git a/products/metrics/backend/diagnostics.py b/products/metrics/backend/diagnostics.py index 122eb6ec620a..5f48fa03624e 100644 --- a/products/metrics/backend/diagnostics.py +++ b/products/metrics/backend/diagnostics.py @@ -234,6 +234,13 @@ def decompose_bucket( for key in ordered_keys[:max_series]: samples = grouped[key] service_name, labels, resource_labels = identities[key] + if plan.temporal is TemporalReducer.POOLED_SAMPLES: + series_value = None + else: + # Normalized the same way as the bucket's total, so the series + # a reader adds up still reach the number they are explaining. + reduced = reduce_temporal(reduction_input[key], plan.temporal) + series_value = None if reduced is None else reduced / plan.divisor breakdown.append( MetricSeriesBreakdown( service_name=service_name, @@ -245,11 +252,7 @@ def decompose_bucket( ), sample_count=len(samples), samples_truncated=len(samples) > max_samples_per_series, - # Normalized the same way as the bucket's total, so the series - # a reader adds up still reach the number they are explaining. - value=None - if plan.temporal is TemporalReducer.POOLED_SAMPLES - else reduce_temporal(reduction_input[key], plan.temporal) / plan.divisor, + value=series_value, ) ) diff --git a/products/metrics/backend/fundamentals.py b/products/metrics/backend/fundamentals.py index fc3ceaf14a00..5fa5753ce4b5 100644 --- a/products/metrics/backend/fundamentals.py +++ b/products/metrics/backend/fundamentals.py @@ -176,11 +176,24 @@ def _deduped_in_time_order(samples: Sequence[Sample]) -> list[Sample]: return list(by_timestamp.values()) -def reduce_temporal(samples: Sequence[Sample], reducer: TemporalReducer) -> float: - """Collapse one series' samples to that series' value for the bucket.""" +def reduce_temporal(samples: Sequence[Sample], reducer: TemporalReducer) -> float | None: + """Collapse one series' samples to that series' value for the bucket. + + Returns None when the value is unknowable: a lone cumulative reading has + no predecessor to diff against, and 0 would read as a flat counter. + """ if reducer in (TemporalReducer.NONE, TemporalReducer.POOLED_SAMPLES): raise ValueError(f"{reducer!r} has no single per-series value; apply it through a plan") ordered = _deduped_in_time_order(samples) + if reducer == TemporalReducer.INCREASE: + # A reading below its predecessor means the counter restarted, and the + # post-restart reading is itself the increase. + if len(ordered) < 2: + return None + total = 0.0 + for previous, current in zip(ordered, ordered[1:]): + total += current.value - previous.value if current.value >= previous.value else current.value + return total if not ordered: return 0.0 @@ -190,14 +203,6 @@ def reduce_temporal(samples: Sequence[Sample], reducer: TemporalReducer) -> floa return sum(sample.value for sample in ordered) if reducer == TemporalReducer.AVG_OVER_TIME: return sum(sample.value for sample in ordered) / len(ordered) - if reducer == TemporalReducer.INCREASE: - # The first sample's history is unknown, so it contributes nothing. A - # reading below its predecessor means the counter restarted, and the - # post-restart reading is itself the increase. - total = 0.0 - for previous, current in zip(ordered, ordered[1:]): - total += current.value - previous.value if current.value >= previous.value else current.value - return total raise ValueError(f"Unsupported temporal reducer: {reducer!r}") @@ -247,7 +252,10 @@ def apply_plan(series_samples: Mapping[K, Sequence[Sample]], plan: ReductionPlan sample.value for samples in series_samples.values() for sample in _deduped_in_time_order(samples) ] else: - per_series_values = [reduce_temporal(samples, plan.temporal) for samples in series_samples.values() if samples] + # An unknowable series value contributes nothing rather than a fake 0, + # and a bucket holding only unknowns has no value at all. + reduced = (reduce_temporal(samples, plan.temporal) for samples in series_samples.values() if samples) + per_series_values = [value for value in reduced if value is not None] value = reduce_spatial(per_series_values, plan.spatial, quantile=plan.quantile) # An empty bucket has no number, and normalizing None would invent one. return value if value is None else value / plan.divisor diff --git a/products/metrics/backend/metric_query_runner.py b/products/metrics/backend/metric_query_runner.py index db3c9941d0eb..17e60eeba3d9 100644 --- a/products/metrics/backend/metric_query_runner.py +++ b/products/metrics/backend/metric_query_runner.py @@ -42,10 +42,12 @@ # Widest queryable range. Counter/histogram queries scan raw samples within # the range on the ClickHouse cluster shared with the live logs/traces -# products, so the span has to be bounded. Those two also scan -# `counter_lookback(interval)` before `date_from` for a predecessor sample; -# the bound stays on the requested range, since the extra reach costs at most -# one more daily partition and returns no extra rows. +# products, so the span has to be bounded. The bound stays on the requested +# range: `date_from` snaps back to its bucket boundary and the counter and +# histogram scans reach a further `counter_lookback(interval)` for a +# predecessor sample, so the scan exceeds the request by under one interval +# step plus the lookback (up to two weeks of extra daily partitions at the +# `week` interval, a single one on the common sub-day charts). MAX_QUERY_SPAN = dt.timedelta(days=31) # These run on the shared logs cluster; cap how much one query may read. @@ -260,6 +262,34 @@ def _interval_step(name: str) -> dt.timedelta: raise ValueError(f"Unknown interval: {name!r}") +# The grids `toStartOfInterval` produces: intervals count from the epoch, +# except weeks, which count from a Monday. +_EPOCH = dt.datetime(1970, 1, 1, tzinfo=dt.UTC) +_WEEK_EPOCH = dt.datetime(1970, 1, 5, tzinfo=dt.UTC) + + +def _align_to_interval(timestamp: dt.datetime, interval: str) -> dt.datetime: + """Floor `timestamp` onto the bucket grid `toStartOfInterval` uses. + + The bucket labels come from `toStartOfInterval(sample_timestamp)`, so a + `date_from` inside a bucket would make that first bucket partial: labelled + as the whole interval but covering only the slice after `date_from`. Every + query scans and clips from this floor instead, so the first bucket holds + its full interval. Relative ranges like "-1h" resolve to now-minus-offset + with second precision, which makes the unaligned case the normal one. + + Not `posthog.interval_specs.align`: that grid honors the team's + `week_start_day` and lacks the sub-hour steps, where `toStartOfInterval` + always counts weeks from Monday — the two would disagree exactly where + agreement with the SQL is the point. + """ + if timestamp.tzinfo is None: + timestamp = timestamp.replace(tzinfo=dt.UTC) + epoch = _WEEK_EPOCH if interval == "week" else _EPOCH + step = _interval_step(interval) + return epoch + ((timestamp - epoch) // step) * step + + # Prometheus's default lookback delta. One interval step on its own is not # enough when the scrape interval is coarser than the bucket — a 60s scrape on # a `second` or `minute` chart — and `metrics1` is partitioned by day with @@ -273,9 +303,9 @@ def counter_lookback(interval: str) -> dt.timedelta: Those aggregations diff each sample against the one before it, so the last sample *outside* the requested range is an input to the first bucket inside - it. Without it the first bucket diffs against nothing and plots 0 - (histograms drop the point instead). The pre-range rows are cut again - before bucketing, so the returned grid is exactly the requested range. + it. Without it the first bucket diffs against nothing and is dropped as + uncomputable. The pre-range rows are cut again before bucketing, so the + returned grid is exactly the requested range. `diagnostics.decompose_bucket` reads its raw samples over the same window through this helper: a shorter reach there would find a different @@ -371,11 +401,13 @@ def __init__( self.team = team self.metric_name = metric_name self.aggregation = aggregation - self.date_from = date_from + self.interval = interval or _pick_interval(date_from, date_to) + # Validation above bounds the requested range; the scan then starts at + # the bucket boundary so the first bucket covers its whole interval. + self.date_from = _align_to_interval(date_from, self.interval) self.date_to = date_to self.filters = tuple(filters) self.group_by = tuple(group_by) - self.interval = interval or _pick_interval(date_from, date_to) self.quantile = quantile self.metric_type = metric_type @@ -559,8 +591,10 @@ def _build_counter_query(self) -> ast.SelectQuery: - cumulative temporality: contribution = value - prev, clamped for counter resets (value < prev means the counter restarted, so the post-reset absolute value IS the increase); a sample with no - predecessor within `counter_lookback` contributes 0 (its history is - unknown). + predecessor within `counter_lookback` has an unknowable increase, so + it contributes NULL, and a bucket where nothing was computable is + dropped rather than plotted as 0 (the histogram path drops such + buckets too). - delta temporality: each sample already is the increase, so it contributes its own value. @@ -586,7 +620,7 @@ def _build_counter_query(self) -> ast.SelectQuery: resource_attributes AS resource_attributes, multiIf( aggregation_temporality = 'delta', value, - isNull(prev_value), 0.0, + isNull(prev_value), NULL, value >= assumeNotNull(prev_value), value - assumeNotNull(prev_value), value ) AS contribution @@ -613,6 +647,7 @@ def _build_counter_query(self) -> ast.SelectQuery: ) WHERE sample_timestamp >= {date_from} GROUP BY time + HAVING isNotNull(value) ORDER BY time ASC LIMIT {row_limit} """, diff --git a/products/metrics/backend/tests/test_diagnostics.py b/products/metrics/backend/tests/test_diagnostics.py index 8ea4ed7c835f..6d4dd5288662 100644 --- a/products/metrics/backend/tests/test_diagnostics.py +++ b/products/metrics/backend/tests/test_diagnostics.py @@ -140,6 +140,30 @@ def test_cumulative_counter_increase_diffs_within_the_series(self) -> None: # +20, then a restart whose post-reset reading is itself the increase. assert decomposition.reference_value == 25.0 + def test_lone_cumulative_sample_has_no_increase_on_either_side(self) -> None: + seed_metric( + team_id=self.team.pk, + metric_name="bytes_total", + metric_type="sum", + aggregation_temporality="cumulative", + is_monotonic=True, + points=[(BUCKET, 100.0)], + ) + + decomposition = decompose_bucket( + team=self.team, + metric_name="bytes_total", + aggregation="increase", + bucket_start=BUCKET, + interval="minute_5", + ) + + # The sample's history is unknown, so both the reference and the chart + # return no value — a 0 on either side would fabricate a flat counter. + assert decomposition.reference_value is None + assert decomposition.actual_value is None + assert decomposition.agrees is True + def test_empty_bucket_reports_no_series_rather_than_zero(self) -> None: decomposition = decompose_bucket( team=self.team, diff --git a/products/metrics/backend/tests/test_fundamentals.py b/products/metrics/backend/tests/test_fundamentals.py index f595767a8294..5a2bdc5603e9 100644 --- a/products/metrics/backend/tests/test_fundamentals.py +++ b/products/metrics/backend/tests/test_fundamentals.py @@ -87,8 +87,9 @@ def test_increase_corrects_counter_reset(self) -> None: # 100 -> 120 is +20; the drop to 5 is a restart, so 5 itself is the increase; 5 -> 25 is +20. assert reduce_temporal(_samples(100, 120, 5, 25), TemporalReducer.INCREASE) == 45 - def test_increase_ignores_history_before_the_first_sample(self) -> None: - assert reduce_temporal(_samples(100), TemporalReducer.INCREASE) == 0 + def test_increase_of_a_lone_sample_is_unknown_not_zero(self) -> None: + # One reading has no predecessor to diff against; 0 would read as "flat". + assert reduce_temporal(_samples(100), TemporalReducer.INCREASE) is None def test_avg_over_time_keeps_the_whole_bucket_not_just_the_tail(self) -> None: # A queue that spiked to 240 and settled at 8 did not average 8. @@ -119,6 +120,14 @@ def test_empty_bucket_has_no_value(self, _name: str, reducer: SpatialReducer) -> # returned 0 here would report every empty bucket as a disagreement. assert reduce_spatial([], reducer) is None + def test_unknown_series_values_drop_out_rather_than_zeroing_the_bucket(self) -> None: + plan = plan_reduction(aggregation="increase", metric_type="sum", temporality="cumulative") + # A lone-sample series adds nothing to the total, and a bucket holding + # only such series has no value at all — mirroring the runner, which + # drops the bucket instead of plotting 0. + assert apply_plan({"a": _samples(100), "b": _samples(10, 25)}, plan) == 15.0 + assert apply_plan({"a": _samples(100)}, plan) is None + class TestPooledQuantile: def test_percentile_reads_the_samples_rather_than_one_value_per_series(self) -> None: diff --git a/products/metrics/backend/tests/test_metric_query_runner.py b/products/metrics/backend/tests/test_metric_query_runner.py index 7a19940b41c8..1d41eaeb76a3 100644 --- a/products/metrics/backend/tests/test_metric_query_runner.py +++ b/products/metrics/backend/tests/test_metric_query_runner.py @@ -22,7 +22,9 @@ from products.metrics.backend.facade.enums import AttributeScope, FilterOp, MetricAggregation from products.metrics.backend.formula import evaluate, parse_formula from products.metrics.backend.metric_query_runner import ( + _INTERVAL_LADDER, MetricQueryRunner, + _align_to_interval, _histogram_quantile, _pick_interval, attribute_field, @@ -46,6 +48,33 @@ def test_pick_interval(self, _name: str, delta: dt.timedelta, expected: str) -> assert _pick_interval(start, start + delta) == expected +class TestAlignToInterval(ClickhouseTestMixin, APIBaseTest): + @parameterized.expand([(name,) for name, _, _ in _INTERVAL_LADDER]) + def test_matches_clickhouse_bucket_boundaries(self, interval: str) -> None: + # The runner snaps date_from onto the bucket grid before querying; if + # this floor ever disagrees with toStartOfInterval, first buckets go + # partial again. + awkward = dt.datetime(2026, 3, 11, 17, 47, 33, 123456, tzinfo=dt.UTC) + aligned = _align_to_interval(awkward, interval) + + interval_call = next(expr for name, _, expr in _INTERVAL_LADDER if name == interval) + interval_arg = interval_call.args[0] + assert isinstance(interval_arg, ast.Constant) + interval_sql = f"{interval_call.name}({interval_arg.value})" + ((clickhouse_aligned,),) = sync_execute( + f"SELECT toStartOfInterval(toDateTime64(%(ts)s, 6, 'UTC'), {interval_sql})", + {"ts": awkward.strftime("%Y-%m-%d %H:%M:%S.%f")}, + ) + if not isinstance(clickhouse_aligned, dt.datetime): + # Week intervals come back as a bare Date. + clickhouse_aligned = dt.datetime.combine(clickhouse_aligned, dt.time(), tzinfo=dt.UTC) + elif clickhouse_aligned.tzinfo is None: + clickhouse_aligned = clickhouse_aligned.replace(tzinfo=dt.UTC) + + self.assertEqual(aligned, clickhouse_aligned) + self.assertLessEqual(aligned, awkward) + + class TestMetricQueryRunner(ClickhouseTestMixin, APIBaseTest): CLASS_DATA_LEVEL_SETUP = True @@ -207,6 +236,39 @@ def test_aggregations_run_across_series_not_samples(self, aggregation: str, expe self.assertEqual([row["value"] for row in runner.run()], [expected]) + def test_unaligned_date_from_reads_the_whole_first_bucket(self): + # The viewer's relative presets ("-1h") resolve to now-minus-offset with + # second precision, so date_from usually lands inside a bucket. A series + # whose only report came before date_from but inside that bucket must + # still count — the bucket stands for its whole interval. + anchor = (timezone.now() - dt.timedelta(minutes=30)).replace(second=0, microsecond=0) + seed_metric( + team_id=self.team.id, + metric_name="m1", + points=[(anchor + dt.timedelta(seconds=5), 3.0)], + labels={"pod": "a"}, + ) + seed_metric( + team_id=self.team.id, + metric_name="m1", + points=[(anchor + dt.timedelta(seconds=40), 4.0)], + labels={"pod": "b"}, + ) + + rows = MetricQueryRunner( + team=self.team, + metric_name="m1", + aggregation="sum", + date_from=anchor + dt.timedelta(seconds=20), + date_to=anchor + dt.timedelta(minutes=1), + interval="minute", + ).run() + + self.assertEqual(len(rows), 1) + self.assertEqual(rows[0]["value"], 7.0) + earliest = dt.datetime.fromisoformat(rows[0]["time"]).astimezone(dt.UTC) + self.assertEqual(earliest, anchor) + def test_synthetic_original_timestamp_does_not_split_a_series(self): anchor = timezone.now().replace(second=0, microsecond=0) bucket = anchor - dt.timedelta(minutes=5) @@ -821,6 +883,52 @@ def test_first_bucket_diffs_against_the_sample_before_the_range(self, aggregatio earliest = dt.datetime.fromisoformat(rows[0]["time"]).astimezone(dt.UTC) self.assertEqual(earliest, self.anchor - dt.timedelta(minutes=1)) + @parameterized.expand( + [ + ("increase", [20.0, 20.0, 20.0]), + ("rate", [20.0 / 60.0, 20.0 / 60.0, 20.0 / 60.0]), + ] + ) + def test_unaligned_date_from_still_charts_a_complete_first_bucket(self, aggregation: str, expected: list[float]): + # date_from usually lands inside a bucket (relative presets resolve to + # now-minus-offset with second precision). The first bucket must cover + # its whole interval, not just the slice after date_from. + self._seed_counter([(self.anchor + dt.timedelta(seconds=s), 100.0 + s / 3.0) for s in range(-60, 181, 15)]) + rows = self._run( + aggregation, + date_from=self.anchor + dt.timedelta(seconds=20), + date_to=self.anchor + dt.timedelta(minutes=3), + ) + for row, expected_value in zip(rows, expected): + self.assertAlmostEqual(row["value"], expected_value) + self.assertEqual(len(rows), len(expected)) + earliest = dt.datetime.fromisoformat(rows[0]["time"]).astimezone(dt.UTC) + self.assertEqual(earliest, self.anchor) + + def test_bucket_with_no_computable_increase_is_dropped_not_zero(self): + # Scraped every 10 minutes: the first in-range sample's predecessor sits + # beyond counter_lookback, so its increase is unknowable. Unknown must + # be a missing point, not a plotted 0 — the histogram path already + # drops such buckets. + start = self.anchor - dt.timedelta(minutes=self.anchor.minute % 5) + self._seed_counter( + [ + (start - dt.timedelta(minutes=10), 100.0), + (start, 200.0), + (start + dt.timedelta(minutes=10), 300.0), + (start + dt.timedelta(minutes=20), 400.0), + ] + ) + rows = self._run( + "increase", + date_from=start, + date_to=start + dt.timedelta(minutes=30), + interval="minute_5", + ) + self.assertEqual([row["value"] for row in rows], [100.0, 100.0]) + earliest = dt.datetime.fromisoformat(rows[0]["time"]).astimezone(dt.UTC) + self.assertEqual(earliest, start + dt.timedelta(minutes=10)) + def test_rate_divides_by_bucket_seconds(self): self._seed_counter( [ @@ -1057,6 +1165,29 @@ def test_first_bucket_diffs_against_the_histogram_before_the_range(self): earliest = dt.datetime.fromisoformat(rows[0]["time"]).astimezone(dt.UTC) self.assertEqual(earliest, self.anchor - dt.timedelta(minutes=1)) + def test_unaligned_date_from_keeps_the_first_buckets_full_distribution(self): + # Growth recorded before date_from but inside the first bucket is part + # of that bucket's distribution; clipping at date_from skews the + # quantile toward whatever happened to grow last. + self._seed_histogram( + [ + (self.anchor, [100, 100, 100, 0]), + (self.anchor + dt.timedelta(seconds=20), [110, 100, 100, 0]), + (self.anchor + dt.timedelta(seconds=40), [110, 110, 100, 0]), + ], + temporality="cumulative", + ) + rows = self._run( + 0.5, + date_from=self.anchor + dt.timedelta(seconds=30), + date_to=self.anchor + dt.timedelta(minutes=1), + ) + self.assertEqual(len(rows), 1) + # Window contribution [10, 10, 0, 0]: p50 sits in the first bucket. + self.assertAlmostEqual(rows[0]["value"], 0.1) + earliest = dt.datetime.fromisoformat(rows[0]["time"]).astimezone(dt.UTC) + self.assertEqual(earliest, self.anchor) + def test_mismatched_bounds_raise(self): self._seed_histogram([(self.anchor + dt.timedelta(seconds=0), [1, 1, 1, 0])], temporality="delta") self._seed_histogram( diff --git a/products/metrics/frontend/components/MetricsOverview.tsx b/products/metrics/frontend/components/MetricsOverview.tsx index ab8d9dc94398..8d84ba911b64 100644 --- a/products/metrics/frontend/components/MetricsOverview.tsx +++ b/products/metrics/frontend/components/MetricsOverview.tsx @@ -1,6 +1,6 @@ import { useActions, useValues } from 'kea' -import { LemonBanner, LemonTable, LemonTag, Link, Spinner } from '@posthog/lemon-ui' +import { LemonBanner, LemonSkeleton, LemonTable, LemonTag, Link } from '@posthog/lemon-ui' import { TZLabel } from 'lib/components/TZLabel' import { dayjs } from 'lib/dayjs' @@ -11,11 +11,58 @@ import { STALE_AFTER_MS, metricsOverviewLogic } from './metricsOverviewLogic' const isStale = (lastSeen: string): boolean => dayjs().diff(dayjs(lastSeen)) > STALE_AFTER_MS -const OverviewStat = ({ label, value, caption }: { label: string; value: number; caption: string }): JSX.Element => ( +// Shared by the loaded cards and their placeholders, so a rename cannot make the +// labels change as the data lands. +const STAT_LABELS = ['Services', 'Metric names', 'Active series'] as const + +// Header text only, so the placeholder table has the same columns as the real one. +const SERVICE_COLUMN_TITLES = ['Service', 'Metrics', 'Active series', 'Last seen'] + +// `null` renders the placeholder. One component for both states, so the loading +// card cannot drift from the loaded one and change size when the data lands. +const OverviewStat = ({ + label, + value, + caption, +}: { + label: string + value: number | null + caption: string | null +}): JSX.Element => (
- {humanFriendlyNumber(value)} + {value === null ? ( + + ) : ( + {humanFriendlyNumber(value)} + )} {label} - {caption} + {caption === null ? ( + + ) : ( + {caption} + )} +
+) + +// The window length arrives with the data, so the captions are placeholders too +// rather than a hardcoded guess that flashes if the server default ever changes. +const MetricsOverviewSkeleton = (): JSX.Element => ( +
+ +
+ {STAT_LABELS.map((label) => ( + + ))} +
+ ({ + title, + align: title === 'Metrics' || title === 'Active series' ? 'right' : undefined, + }))} + />
) @@ -65,11 +112,7 @@ export const MetricsOverview = (): JSX.Element => { const { viewService } = useActions(metricsOverviewLogic) if (!overview) { - return ( -
- -
- ) + return } const windowHours = Math.round(overview.lookback_seconds / 3600) @@ -79,9 +122,20 @@ export const MetricsOverview = (): JSX.Element => {
- - - + {STAT_LABELS.map((label) => ( + + ))}