Skip to content

fix(metrics): improve PromQL selector windows and counter queries - #4

Merged
frankh merged 5 commits into
mainfrom
posthog/fix-metrics-query-lookback
Sep 15, 2026
Merged

frankh merged 5 commits into
mainfrom
posthog/fix-metrics-query-lookback

Conversation

@frankh

@frankh frankh commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

Why

PromQL queries must return useful results when sample times do not match query steps. Short counter windows must include the previous sample, and counter increases must not estimate values at window edges.

Keep the PromQL-compatible API and add selected MetricsQL improvements for automatic query windows, counter calculations, and supported syntax extensions. Apply the same behavior to both ClickHouse storage formats. Preserve stale markers and return numeric constants as vectors.

Other functions and native histogram calculations retain Prometheus behavior. Automatic selectors and counter rollups use the evaluation engine and can read more raw samples. Existing query limits still apply.

Validation

  • go mod verify, make lint, and go build ./....
  • CH_SCHEMA_LAYOUT=current SNUFFLE_E2E_SKIP_DOCKER=1 make integration-test.
  • CH_SCHEMA_LAYOUT=posthog SNUFFLE_E2E_SKIP_DOCKER=1 make integration-test.

Both integration runs use real ClickHouse and include the full suite with race checks. Counter regression values also match VictoriaMetrics v1.152.0. The final documentation-only update passes git diff --check.


Created with PostHog Desktop

Generated-By: PostHog Desktop
Task-Id: 52a64169-b741-47be-ab09-da69151123f8
@frankh frankh self-assigned this Sep 15, 2026
Generated-By: PostHog Desktop
Task-Id: 52a64169-b741-47be-ab09-da69151123f8
@frankh frankh changed the title fix(metrics): preserve lookback in PostHog PromQL queries fix(metrics): apply MetricsQL selector and counter rules Sep 15, 2026
Generated-By: PostHog Desktop
Task-Id: 52a64169-b741-47be-ab09-da69151123f8
@frankh frankh changed the title fix(metrics): apply MetricsQL selector and counter rules fix(metrics): improve PromQL selector windows and counter queries Sep 15, 2026
@frankh
frankh marked this pull request as ready for review September 15, 2026 09:53
@frankh
frankh requested a review from rorylshanks September 15, 2026 09:53
Copy the range result instead of compacting the engine matrix in place, so
the engine does not return the same point buffer to its pool twice.

Restore the instant SQL fast paths: the internal default_rollup of an instant
query reads the lookback window, so the fast path unwraps it to the selector.
Range fast paths cannot reproduce automatic selector windows or MetricsQL
counter calculations, so the dead range SQL paths are removed.

Keep Prometheus functions that MetricsQL does not define by carrying their
names through the MetricsQL parser. Leave the selector arguments of timestamp
and absent untouched, use the library rollup argument index, and resolve
implicit windows inside a subquery against the subquery step.

Evaluate running_sum as a post-pass over the range result instead of a
doubled subquery. Guard the rate divisor, record query stats after the NaN
filter, and update README and PERFORMANCE for the paths that run.

Generated-By: PostHog Desktop
Task-Id: 46a47e68-6b45-4bbc-9303-303608734206

frankh commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Pushed 400c43a with fixes for the review findings.

Correctness

  • metricsQLValue builds new result slices. The engine returns the input matrix to its pools on close, so the result must not alias it.
  • Prometheus functions that MetricsQL does not define (histogram_count, histogram_sum, info, ...) work again. Their names travel through the MetricsQL parser as a string argument of union.
  • timestamp(m) and absent(m) keep their selector argument, so the engine returns the sample timestamp and the absent labels.
  • The rollup argument index comes from metricsql.GetRollupArgIdx.
  • Omitted windows and step units inside a subquery use the subquery step.
  • rate returns no value when the sample duration is zero.
  • Query stats are recorded after the NaN filter.

Fast paths

  • Instant queries: the fast path unwraps the internal default_rollup to its selector, because both read the latest non-stale sample in the lookback window. sum, topk, and nested counts over bare selectors use SQL again.
  • Range queries: automatic selector windows depend on each series and counter rollups use MetricsQL math, so SQL cannot reproduce them. The unreachable range SQL paths and their tests are removed. metricsQLServer and the per-request queryable copy are gone; no read path uses REMOTE_WRITE_SAMPLE_INTERVAL now.

running_sum is a post-pass over the range result. It no longer doubles the scanned range and no longer requires start aligned to step. It must be the outermost function.

Docs: README and PERFORMANCE describe the paths that run, including the one-step visibility of a single sample in a range query.

Validation here: make lint, go build ./..., and go test -race ./... pass. The ClickHouse integration suite was not run in this environment. The e2e table gained timestamp and absent cases and a rangePoints column; please run make integration-test for both layouts.

The ClickHouse e2e test evaluates running_sum inside quantile. The outermost
running_sum still runs on the range result; a nested one runs as a subquery
that spans the range from the aligned start.

Generated-By: PostHog Desktop
Task-Id: 46a47e68-6b45-4bbc-9303-303608734206
@frankh
frankh merged commit a20223a into main Sep 15, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant