Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions posthog/hogql/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ class HogQLQuerySettings(BaseModel):
join_algorithm: Optional[str] = None
grace_hash_join_initial_buckets: Optional[int] = None
force_data_skipping_indices: Optional[list[str]] = None
force_optimize_projection: Optional[bool] = None
load_balancing: Optional[str] = None
format_csv_allow_double_quotes: Optional[bool] = None
optimize_skip_unused_shards: Optional[bool] = None
Expand Down
3 changes: 3 additions & 0 deletions posthog/hogql/database/schema/metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,9 @@ class MetricSeriesTable(Table):
"last_seen": DateTimeDatabaseField(
name="timestamp", nullable=False, description="Most recent sample timestamp seen for this series."
),
"time_bucket": DateTimeDatabaseField(
name="time_bucket", nullable=False, description="Start of the UTC hour that contains `last_seen`."
),
"original_expiry_timestamp": DateTimeDatabaseField(
name="original_expiry_timestamp", nullable=False, description="When the series leaves retention."
),
Expand Down
19 changes: 11 additions & 8 deletions products/metrics/backend/metrics_overview_query_runner.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
"""No FINAL: `uniqExact` and `max(last_seen)` give the same result on unmerged duplicate rows."""
"""No FINAL: the distinct counts and `max(last_seen)` give the same result on unmerged duplicate rows."""

import datetime as dt
import contextvars
Expand All @@ -7,7 +7,7 @@
from opentelemetry import trace
from opentelemetry.trace import Span

from posthog.schema import HogQLQueryResponse
from posthog.schema import HogQLQueryModifiers, HogQLQueryResponse

from posthog.hogql import ast
from posthog.hogql.constants import HogQLGlobalSettings
Expand Down Expand Up @@ -112,18 +112,20 @@ def _run_metric_names_count(self) -> int:
def _run_services(self) -> _ServicesRollup:
with tracer.start_as_current_span("metrics.overview.services") as span:
span.set_attribute("team_id", self.team.pk)
# Each series has one service, so the sum of the service counts is exact.
# The services_by_hour projection answers this query only while it filters on time_bucket, uses uniq,
# and aggregates the bare last_seen column. The time zone conversion stays outside max() for that reason.
# Each series has one service, so the sum of the service counts counts each series once.
query = parse_select(
"""
SELECT
service_name,
uniqExact(metric_name) AS metric_names,
uniqExact(series_fingerprint) AS series,
max(last_seen) AS last_seen_at,
sum(uniqExact(series_fingerprint)) OVER () AS total_series,
max(max(last_seen)) OVER () AS total_last_seen_at
uniq(series_fingerprint) AS series,
toTimeZone(max(last_seen), 'UTC') AS last_seen_at,
sum(uniq(series_fingerprint)) OVER () AS total_series,
toTimeZone(max(max(last_seen)) OVER (), 'UTC') AS total_last_seen_at
FROM posthog.metric_series
WHERE last_seen > now() - {lookback}
WHERE time_bucket >= toStartOfHour(toTimeZone(now() - {lookback}, 'UTC'))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Lookback includes stale series. If a series was last seen before the lookback cutoff but within the same UTC hour, this bucket-only filter still includes it. The overview can therefore show nearly an extra hour of stale services and active series while labeling the counts “Last 24 hours.” Keep an exact last_seen cutoff alongside the bucket bound, or report the window the query actually uses.

Prompt To Fix With AI
This is a comment left during a code review.
Path: products/metrics/backend/metrics_overview_query_runner.py
Line: 128

Comment:
**Lookback includes stale series.** If a series was last seen before the lookback cutoff but within the same UTC hour, this bucket-only filter still includes it. The overview can therefore show nearly an extra hour of stale services and active series while labeling the counts “Last 24 hours.” Keep an exact `last_seen` cutoff alongside the bucket bound, or report the window the query actually uses.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it really doesn't matter if it's 25hrs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right — the 24–25 hour whole-UTC-hour window is explicitly intentional, so including rows from the cutoff hour is part of the desired behavior. My comment treated the lookback as an exact 24-hour cutoff; it isn't a valid finding for this change, and no adjustment is needed.

GROUP BY service_name
ORDER BY series DESC, service_name ASC
LIMIT {limit}
Expand All @@ -138,6 +140,7 @@ def _run_services(self) -> _ServicesRollup:
team=self.team,
workload=Workload.LOGS,
settings=_QUERY_SETTINGS,
modifiers=HogQLQueryModifiers(convertToProjectTimezone=False),
)
_set_query_timing_attributes(span, response)
span.set_attribute("services.count", len(response.results))
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import datetime as dt
from typing import Any

from posthog.test.base import APIBaseTest, ClickhouseTestMixin
from unittest.mock import patch
Expand All @@ -8,6 +9,10 @@
from parameterized import parameterized
from rest_framework import status

from posthog.schema import HogQLQueryResponse

from posthog.hogql.query import execute_hogql_query

from products.metrics.backend import metrics_overview_query_runner
from products.metrics.backend.metrics_overview_query_runner import MetricsOverviewQueryRunner
from products.metrics.backend.tests._seeder import seed_metric, seed_metric_event, truncate_metrics_tables
Expand Down Expand Up @@ -65,6 +70,20 @@ def test_rolls_up_services_within_the_window(self, _name: str, max_services: int
self.assertEqual(api_row.series, 3)
self.assertEqual(dt.datetime.fromisoformat(api_row.last_seen), anchor)

def test_services_query_reads_the_hourly_projection(self):
anchor = timezone.now().replace(microsecond=0) - dt.timedelta(minutes=5)
seed_metric(team_id=self.team.id, metric_name="http.duration", points=[(anchor, 1.0)], service_name="api")

def force_projection(**kwargs: Any) -> HogQLQueryResponse:
if kwargs["query_type"] == "MetricsOverviewServicesQuery":
kwargs["settings"] = kwargs["settings"].model_copy(update={"force_optimize_projection": True})
return execute_hogql_query(**kwargs)

with patch.object(metrics_overview_query_runner, "execute_hogql_query", side_effect=force_projection):
overview = MetricsOverviewQueryRunner(team=self.team).run()

self.assertEqual([(s.service_name, s.series) for s in overview.services], [("api", 1)])

def test_quiet_project_keeps_overall_last_seen_but_lists_no_services(self):
stale = timezone.now().replace(microsecond=0) - dt.timedelta(days=3)
seed_metric(team_id=self.team.id, metric_name="http.duration", points=[(stale, 1.0)], service_name="api")
Expand Down
Loading