feat(metrics): add SQL editor scene for the alpha metrics product#60374
Merged
Conversation
Contributor
Author
Contributor
|
🎭 Playwright didn't run on this PR — your changes touch code that could affect E2E behavior, but Playwright is opt-in via label now to keep CI cost down. Add the Most PRs don't need this. Real regressions still get caught on master and fix-forward. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| cache.delete(f"team:{self.team.id}:has_metrics") | ||
|
|
||
| def test_has_metrics_returns_false_when_no_metrics(self): | ||
| sync_execute("TRUNCATE TABLE IF EXISTS metrics1") |
Contributor
There was a problem hiding this comment.
Truncates wrong table name. Uses metrics1 instead of metrics, which means the test won't properly clear the metrics table. This will cause test isolation issues.
sync_execute("TRUNCATE TABLE IF EXISTS metrics")
Suggested change
| sync_execute("TRUNCATE TABLE IF EXISTS metrics1") | |
| sync_execute("TRUNCATE TABLE IF EXISTS metrics") |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
Contributor
Author
There was a problem hiding this comment.
This is intentional and correct — metrics is a Distributed engine table, not a storage table:
-- bin/clickhouse-metrics.sql
CREATE OR REPLACE TABLE metrics1 (...) ENGINE = MergeTree ...
create or replace TABLE metrics AS metrics1 ENGINE = Distributed('posthog', 'default', 'metrics1');ClickHouse Distributed tables don't support TRUNCATE — the statement errors with Table engine Distributed doesn't support TRUNCATE method. Data lives in the local metrics1 MergeTree, which is the table that has to be truncated.
This matches the logs precedent at products/logs/backend/test/test_has_logs_query_runner.py:64, which truncates logs32 (the local table) not logs (the Distributed alias).
Switching to TRUNCATE TABLE metrics as suggested would break the test suite.
Contributor
|
Size Change: +12.6 kB (+0.02%) Total Size: 80.7 MB 📦 View Changed
ℹ️ View Unchanged
|
DanielVisca
force-pushed
the
daniel/metrics-sql-editor
branch
from
8a04137 to
2ee7716
Compare
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
products/metrics/backend/tests/test_has_metrics_query_runner.py:13-27
**Missing positive-path test for `HasMetricsQueryRunner`**
`TestHasMetricsQueryRunner` only exercises the empty-table branch. If a regression ever makes `run()` return `False` even when metrics exist (wrong table name, broken HogQL namespace resolution, etc.), none of the backend tests would catch it. A complementary positive test would insert one row into `metrics1`, call `runner.run()`, and assert the result is `True`.
Reviews (1): Last reviewed commit: "fix(metrics): satisfy strict product:lin..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+13
to
+27
| class TestHasMetricsQueryRunner(ClickhouseTestMixin, APIBaseTest): | ||
| CLASS_DATA_LEVEL_SETUP = True | ||
|
|
||
| def setUp(self): | ||
| super().setUp() | ||
| cache.delete(f"team:{self.team.id}:has_metrics") | ||
|
|
||
| def test_has_metrics_returns_false_when_no_metrics(self): | ||
| sync_execute("TRUNCATE TABLE IF EXISTS metrics1") | ||
| cache.delete(f"team:{self.team.id}:has_metrics") | ||
|
|
||
| runner = HasMetricsQueryRunner(self.team) | ||
| self.assertFalse(runner.run()) | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
HasMetricsQueryRunner
TestHasMetricsQueryRunner only exercises the empty-table branch. If a regression ever makes run() return False even when metrics exist (wrong table name, broken HogQL namespace resolution, etc.), none of the backend tests would catch it. A complementary positive test would insert one row into metrics1, call runner.run(), and assert the result is True.
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/metrics/backend/tests/test_has_metrics_query_runner.py
Line: 13-27
Comment:
**Missing positive-path test for `HasMetricsQueryRunner`**
`TestHasMetricsQueryRunner` only exercises the empty-table branch. If a regression ever makes `run()` return `False` even when metrics exist (wrong table name, broken HogQL namespace resolution, etc.), none of the backend tests would catch it. A complementary positive test would insert one row into `metrics1`, call `runner.run()`, and assert the result is `True`.
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Author
There was a problem hiding this comment.
Addressed in commit 48f89cd73b. Added two complementary tests:
test_has_metrics_returns_true_when_metrics_exist— inserts a minimal row intometrics1and assertsrunner.run()returnsTrue. Catches regressions in HogQLposthog.metricsnamespace resolution, table name, or theSELECT 1 LIMIT 1shape.test_has_metrics_respects_team_isolation— inserts a row for a differentteam_idand assertsrunner.run()returnsFalsefor our team. Catches regressions in HogQL's auto-injectedteam_idfilter.
All 7 backend tests in the file pass.
DanielVisca
force-pushed
the
daniel/metrics-sql-editor
branch
2 times, most recently
from
aa0ff1e to
ee13e5f
Compare
DanielVisca
force-pushed
the
daniel/metrics-sql-editor
branch
from
48f89cd to
7e5c2aa
Compare
jonmcwest
approved these changes
May 28, 2026
Mirrors the logs pattern to give the empty /metrics scene a usable starting point: a HogQL SQL editor over posthog.metrics plus a setup prompt when no metrics have been ingested. Adds the metrics scope (OAuth-hidden, alpha) and a has_metrics endpoint backed by a cached query runner. Generated-By: PostHog Code Task-Id: c20a4a56-d258-420f-8b00-df6aaf3083b5
- Tag HogQL queries from the Metrics scene (was UntaggedQueryError on every editor run): adds Metrics to SCENE_TO_TAGS so user-authored `FROM posthog.metrics` queries get product=metrics, since the table fallback only matches chain[0] which is "posthog". - Bound default editor query with WHERE timestamp > now() - INTERVAL 1 DAY so the auto-run on first mount uses partition pruning instead of cross-partition merging on the (team_id, ..., timestamp) sort key. - Wrap has_metrics probe in try/except: ClickHouse outages / un- provisioned tables now log + capture to Sentry and return False instead of bubbling a 500. - Drop the legacy /api/projects/.../metrics/ route — only register the modern environments-nested path. - Bump cache key to v1 suffix so a future shape change can roll cleanly. - Drop banner dismissKey so verification-failed hint reappears on each failed check. - Fix subject-verb agreement in empty-state copy. Generated-By: PostHog Code Task-Id: c20a4a56-d258-420f-8b00-df6aaf3083b5
Keep only the SCENE_TO_TAGS entry that fixes the actual UntaggedQueryError
in the SQL editor. Revert the rest to match the logs precedent that every
other product follows:
- has_metrics runner: no try/except (logs runner doesn't have one either)
- viewset registration: stay with register_grandfathered_environment_*
(matches every other product including logs)
- cache key: bare team:{id}:has_metrics (no :v1 suffix)
- LemonBanner: keep dismissKey (logs uses the same pattern)
- empty-state copy: match the logs phrasing 1:1
- default editor query: SELECT * FROM posthog.metrics LIMIT 10 to mirror
the logs default `SELECT * FROM logs LIMIT 10` — no ORDER BY needed
Generated-By: PostHog Code
Task-Id: c20a4a56-d258-420f-8b00-df6aaf3083b5
…ntation/ The metrics product is in strict isolation mode (it has backend/facade/ contracts.py), so hogli product:lint flags backend/api.py at root as misplaced — the canonical location is backend/presentation/api.py. - Move backend/api.py -> backend/presentation/api.py - Update the import in posthog/api/__init__.py - Add the matching tach interface block exposing backend.facade and backend.presentation.api - Register products.metrics under posthog's depends_on so the import is allowed by tach - Update the report_user_action mock path in the test Generated-By: PostHog Code Task-Id: c20a4a56-d258-420f-8b00-df6aaf3083b5
CI import-linter flagged the strict-mode contract: products.metrics.backend.presentation.api was importing products.metrics.backend.has_metrics_query_runner directly. In strict mode (this product has backend/facade/contracts.py) the presentation layer must only import from the facade. - facade/api.py: thin team_has_metrics wrapper over the runner - presentation/api.py: import from facade instead of the runner module - package.json: add the canonical backend:contract-check script so turbo + product:lint recognise this product as isolated Verified locally: lint-imports passes, hogli product:lint metrics passes with no warnings, 6 backend tests still pass. Generated-By: PostHog Code Task-Id: c20a4a56-d258-420f-8b00-df6aaf3083b5
…yRunner Per PR review: the runner-level test class only covered the empty-table branch. Add two complementary tests: - test_has_metrics_returns_true_when_metrics_exist: insert one row into metrics1 (the local table — INSERTs go through Distributed but the point is end-to-end the HogQL SELECT must find it), assert run() returns True. Catches regressions in posthog.metrics namespace resolution, table name, or LIMIT 1 shape. - test_has_metrics_respects_team_isolation: insert a row for a different team, assert run() returns False for our team. Catches regressions in HogQL's team_id auto-filter. Generated-By: PostHog Code Task-Id: c20a4a56-d258-420f-8b00-df6aaf3083b5
DanielVisca
force-pushed
the
daniel/metrics-sql-editor
branch
from
7e5c2aa to
427a84b
Compare
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
2 participants
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.
Problem
The
/metricsscene shipped earlier as an empty scaffold — the route exists behindFEATURE_FLAGS.METRICSbut there's nothing to do on the page. We have OTLP metrics flowing through the Rustcapture-logsingester into ClickHouse (metrics/metric_attributestables) and a HogQLposthog.metricstable already registered. We need a usable surface so the alpha team can start exploring the data.This is the base of a 4-PR stack to bring Metrics close to feature-parity with the Sentry "Application Metrics" experience. The stack:
metric_query_runner.py+ viewer tab)query-metricstoolChanges
Mirrors the logs pattern so the two surfaces stay recognisable. The
/metricsscene now:has_metricsisFalse.SQLEditor(the same component the Logs SQL tab uses) preloaded withSELECT * FROM posthog.metrics LIMIT 10once the team has ingested at least one metric.keepSqlEditorMountedpattern fromlogsSceneLogic.Surface area
MetricsViewSet.has_metricsaction backed byHasMetricsQueryRunner(cached 7d, mirrorsteam_has_logs). NewmetricsAPI scope (OAuth-hidden, alpha).Product.METRICS+_TABLE_TO_TAGSfallback for{metrics, metric_attributes}. NewSCENE_TO_TAGS["Metrics"]entry so user-authored editor queries get tagged.MetricsScenerefactor,MetricsSetupPrompt,MetricsSqlEditor,metricsSceneLogic,metricsIngestionLogic,metricsSqlEditorTrackingLogic. Newapi.metrics.hasMetrics()client.ProductIntentContext.METRICS_DOCS_VIEWEDvalue. Regeneratedposthog/schema.py+schema.jsonviahogli build:schema.What's intentionally NOT in this PR
query-metricstool → PR4DRY notes for reviewers
metricsIngestionLogic,MetricsSetupPrompt, and the editor scaffolding are near-clones of the logs equivalents. Inlined rather than extracted into a genericproductIngestionLogic({ key, apiCall }): with only two callers, the abstraction would be premature. Worth extracting once a third product (tracing? metrics-mcp?) wants the same shape.How did you test this code?
I'm an agent. Automated coverage I actually ran:
pytest products/metrics/backend/tests— 6 tests pass (HasMetricsQueryRunneragainst a real ClickHouse, thehas_metricsAPI, auth gating, positive-result caching, no-cache on negative).pytest posthog/clickhouse/test/test_query_tagging.py -k 'scene or fallback'— all 34 query-tag tests still green after addingProduct.METRICSand theSCENE_TO_TAGS["Metrics"]entry.pnpm typescript:check— no new TS errors inproducts/metrics/.ruff check+ruff formatclean on touched files.I also confirmed end-to-end against a live dev stack: the SQL editor renders, runs the default query, and returns real metric rows (a
summarymetric being scraped from a local nodejs service via the OTel collector's Prometheus receiver — 10 rows ofaction_match_mswithservice_name=nodejs, populatedresource_attributesmap, sanetimestamp/time_bucket).I did not test:
MetricsSetupPromptempty-state path in a browser (only the post-ingestion path).teamHasMetricsCheckFailedbanner branch.Publish to changelog?
no
Docs update
Not needed — alpha gated by
FEATURE_FLAGS.METRICS, no public docs yet.🤖 Agent context
Authored by Daniel via PostHog Code (Claude Opus 4.7).
Approach. The task was "make
/metricsuseful". I mirrored the logs precedent because (a) it's the closest analogue, (b) the HogQLposthog.metricstable and Rust ingestion already exist in master, so a SQL editor over them is the minimum viable surface, and (c) Sentry's "SQL front end" framing (Better Stack and others do this too) is a credible alpha story while we work on the higher-fidelity viewer.Stack plan agreed before implementation. PR1 SQL editor → PR2 line-chart viewer +
metric_query_runner.py→ PR3 group-by + samples / trace-connected → PR4 MCPquery-metrics. Each shippable on its own.Decisions worth noting:
register_grandfathered_environment_nested_viewsetoverenvironments_router.register— followed the precedent every other product (including logs) uses, despite the helper's docstring discouraging new use. Worth a repo-wide migration but not in this PR.posthog.metricsnamespace in the probe query and editor default, not bareFROM metrics. The HogQL table is intentionally only registered under theposthog.node (seeposthog/hogql/database/database.py). Inline comments call this out so a future contributor doesn't "fix" it to match logs.MetricstoSCENE_TO_TAGSrather than extendingHogQLFeatureExtractorto look beyondchain[0]. Surgical, fixes the actualUntaggedQueryErrorthat originally happened when running queries from the editor.metricsscope added toOAUTH_HIDDEN_SCOPE_OBJECTS— same posturewizard_sessionuses for alpha products. PAT validation accepts it; OAuth/MCP scopes don't advertise it.try/exceptinHasMetricsQueryRunner, no negative-result caching, no in-flight dedup on the 5s setup-prompt polling, no localStoragefalsewrite-back. All match the logs precedent exactly. QA flagged these as concerns; treating them as a precedent-wide follow-up rather than a metrics-only fix in this PR.Stack tooling. Created and submitted via Graphite (
gt track+gt submit --draft).