feat(metrics): label filters with explicit resource/attribute/auto scope (P3)#63125
Open
DanielVisca wants to merge 1 commit into
Open
Conversation
This was referenced Jun 11, 2026
Contributor
Author
This was referenced Jun 11, 2026
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
products/metrics/backend/metric_query_runner.py:127-143
The parameter name `filter` shadows Python's built-in `filter()` function. Rename it to avoid masking the builtin throughout the function body.
```suggestion
def _filter_condition(metric_filter: MetricFilter) -> ast.Expr:
"""One label predicate as a HogQL boolean expression.
Missing map keys resolve to `''`, so `neq`/`not_regex` also match rows
that lack the key entirely — same as Prometheus negative matchers.
"""
field = attribute_field(metric_filter.key, scope=metric_filter.scope.value)
placeholders: dict[str, ast.Expr] = {"field": field, "value": ast.Constant(value=metric_filter.value)}
if metric_filter.op == FilterOp.EQ:
return parse_expr("{field} = {value}", placeholders=placeholders)
if metric_filter.op == FilterOp.NEQ:
return parse_expr("{field} != {value}", placeholders=placeholders)
if metric_filter.op == FilterOp.REGEX:
return parse_expr("match({field}, {value})", placeholders=placeholders)
if metric_filter.op == FilterOp.NOT_REGEX:
return parse_expr("not match({field}, {value})", placeholders=placeholders)
raise ValueError(f"Unsupported filter op: {metric_filter.op!r}")
```
### Issue 2 of 2
products/metrics/backend/presentation/api.py:42-45
The `value` field has no `max_length` constraint while `key` is capped at 255. For `regex`/`not_regex` ops, `value` is the regex pattern sent straight to ClickHouse's `match()`. Without a length cap, an arbitrarily large pattern string could be submitted, increasing both parse overhead and payload size.
```suggestion
value = serializers.CharField(
allow_blank=True,
max_length=1024,
help_text="Value to compare against. For regex operators this is the pattern.",
)
```
Reviews (1): Last reviewed commit: "feat(metrics): label filters with explic..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+127
to
+143
| def _filter_condition(filter: MetricFilter) -> ast.Expr: | ||
| """One label predicate as a HogQL boolean expression. | ||
|
|
||
| Missing map keys resolve to `''`, so `neq`/`not_regex` also match rows | ||
| that lack the key entirely — same as Prometheus negative matchers. | ||
| """ | ||
| field = attribute_field(filter.key, scope=filter.scope.value) | ||
| placeholders: dict[str, ast.Expr] = {"field": field, "value": ast.Constant(value=filter.value)} | ||
| if filter.op == FilterOp.EQ: | ||
| return parse_expr("{field} = {value}", placeholders=placeholders) | ||
| if filter.op == FilterOp.NEQ: | ||
| return parse_expr("{field} != {value}", placeholders=placeholders) | ||
| if filter.op == FilterOp.REGEX: | ||
| return parse_expr("match({field}, {value})", placeholders=placeholders) | ||
| if filter.op == FilterOp.NOT_REGEX: | ||
| return parse_expr("not match({field}, {value})", placeholders=placeholders) | ||
| raise ValueError(f"Unsupported filter op: {filter.op!r}") |
Contributor
There was a problem hiding this comment.
filter shadows Python's built-in filter() function. Rename it to avoid masking the builtin throughout the function body.
Suggested change
| def _filter_condition(filter: MetricFilter) -> ast.Expr: | |
| """One label predicate as a HogQL boolean expression. | |
| Missing map keys resolve to `''`, so `neq`/`not_regex` also match rows | |
| that lack the key entirely — same as Prometheus negative matchers. | |
| """ | |
| field = attribute_field(filter.key, scope=filter.scope.value) | |
| placeholders: dict[str, ast.Expr] = {"field": field, "value": ast.Constant(value=filter.value)} | |
| if filter.op == FilterOp.EQ: | |
| return parse_expr("{field} = {value}", placeholders=placeholders) | |
| if filter.op == FilterOp.NEQ: | |
| return parse_expr("{field} != {value}", placeholders=placeholders) | |
| if filter.op == FilterOp.REGEX: | |
| return parse_expr("match({field}, {value})", placeholders=placeholders) | |
| if filter.op == FilterOp.NOT_REGEX: | |
| return parse_expr("not match({field}, {value})", placeholders=placeholders) | |
| raise ValueError(f"Unsupported filter op: {filter.op!r}") | |
| def _filter_condition(metric_filter: MetricFilter) -> ast.Expr: | |
| """One label predicate as a HogQL boolean expression. | |
| Missing map keys resolve to `''`, so `neq`/`not_regex` also match rows | |
| that lack the key entirely — same as Prometheus negative matchers. | |
| """ | |
| field = attribute_field(metric_filter.key, scope=metric_filter.scope.value) | |
| placeholders: dict[str, ast.Expr] = {"field": field, "value": ast.Constant(value=metric_filter.value)} | |
| if metric_filter.op == FilterOp.EQ: | |
| return parse_expr("{field} = {value}", placeholders=placeholders) | |
| if metric_filter.op == FilterOp.NEQ: | |
| return parse_expr("{field} != {value}", placeholders=placeholders) | |
| if metric_filter.op == FilterOp.REGEX: | |
| return parse_expr("match({field}, {value})", placeholders=placeholders) | |
| if metric_filter.op == FilterOp.NOT_REGEX: | |
| return parse_expr("not match({field}, {value})", placeholders=placeholders) | |
| raise ValueError(f"Unsupported filter op: {metric_filter.op!r}") |
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/metrics/backend/metric_query_runner.py
Line: 127-143
Comment:
The parameter name `filter` shadows Python's built-in `filter()` function. Rename it to avoid masking the builtin throughout the function body.
```suggestion
def _filter_condition(metric_filter: MetricFilter) -> ast.Expr:
"""One label predicate as a HogQL boolean expression.
Missing map keys resolve to `''`, so `neq`/`not_regex` also match rows
that lack the key entirely — same as Prometheus negative matchers.
"""
field = attribute_field(metric_filter.key, scope=metric_filter.scope.value)
placeholders: dict[str, ast.Expr] = {"field": field, "value": ast.Constant(value=metric_filter.value)}
if metric_filter.op == FilterOp.EQ:
return parse_expr("{field} = {value}", placeholders=placeholders)
if metric_filter.op == FilterOp.NEQ:
return parse_expr("{field} != {value}", placeholders=placeholders)
if metric_filter.op == FilterOp.REGEX:
return parse_expr("match({field}, {value})", placeholders=placeholders)
if metric_filter.op == FilterOp.NOT_REGEX:
return parse_expr("not match({field}, {value})", placeholders=placeholders)
raise ValueError(f"Unsupported filter op: {metric_filter.op!r}")
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+42
to
+45
| value = serializers.CharField( | ||
| allow_blank=True, | ||
| help_text="Value to compare against. For regex operators this is the pattern.", | ||
| ) |
Contributor
There was a problem hiding this comment.
value field has no max_length constraint while key is capped at 255. For regex/not_regex ops, value is the regex pattern sent straight to ClickHouse's match(). Without a length cap, an arbitrarily large pattern string could be submitted, increasing both parse overhead and payload size.
Suggested change
| value = serializers.CharField( | |
| allow_blank=True, | |
| help_text="Value to compare against. For regex operators this is the pattern.", | |
| ) | |
| value = serializers.CharField( | |
| allow_blank=True, | |
| max_length=1024, | |
| help_text="Value to compare against. For regex operators this is the pattern.", | |
| ) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/metrics/backend/presentation/api.py
Line: 42-45
Comment:
The `value` field has no `max_length` constraint while `key` is capped at 255. For `regex`/`not_regex` ops, `value` is the regex pattern sent straight to ClickHouse's `match()`. Without a length cap, an arbitrarily large pattern string could be submitted, increasing both parse overhead and payload size.
```suggestion
value = serializers.CharField(
allow_blank=True,
max_length=1024,
help_text="Value to compare against. For regex operators this is the pattern.",
)
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
|
Size Change: 0 B Total Size: 72.4 MB ℹ️ View Unchanged
|
DanielVisca
force-pushed
the
posthog-code/metrics-ql3-label-filters
branch
from
cf2ee47 to
8465408
Compare
DanielVisca
force-pushed
the
posthog-code/metrics-ql2-series-shape
branch
2 times, most recently
from
7b38644 to
4d9b5bc
Compare
DanielVisca
force-pushed
the
posthog-code/metrics-ql3-label-filters
branch
from
8465408 to
9dc3731
Compare
DanielVisca
added
the
stamphog
Graphite App
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No showstoppers. The filter values are passed as HogQL AST constants (parameterized, not string-interpolated), so there's no injection risk. The bot's P2 comments — filter shadowing the built-in and missing max_length on value — are style/DoS-mitigation suggestions, not correctness or security blockers. Tests cover all filter ops, AND logic, the API endpoint, and the facade layer. Generated types match the serializer.
Contributor
🕸️ Eager graphHow much code each root forces the browser to download and decode through static imports — the regression class total bundle size can't see.
✅ Largest files eagerly reachable from
|
| Size | File |
|---|---|
| 878.2 KiB | src/styles/global.scss |
| 609.0 KiB | public/hedgehog/burning-money-hog.png |
| 541.9 KiB | public/hedgehog/waving-hog.png |
| 448.2 KiB | public/hedgehog/stop-sign-hog.png |
| 362.0 KiB | public/hedgehog/phone-pair-hogs.png |
| 354.8 KiB | ../node_modules/.pnpm/@posthog+icons@0.36.6_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 335.6 KiB | public/hedgehog/desk-hog.png |
| 323.2 KiB | public/hedgehog/3-bears-hogs.png |
| 297.4 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 285.8 KiB | src/lib/api.ts |
Largest files eagerly reachable from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 878.2 KiB | src/styles/global.scss |
| 760.0 KiB | src/queries/validators.js |
| 609.0 KiB | public/hedgehog/burning-money-hog.png |
| 541.9 KiB | public/hedgehog/waving-hog.png |
| 448.2 KiB | public/hedgehog/stop-sign-hog.png |
| 398.7 KiB | ../node_modules/.pnpm/chart.js@4.5.1/node_modules/chart.js/dist/chart.js |
| 362.0 KiB | public/hedgehog/phone-pair-hogs.png |
| 354.8 KiB | ../node_modules/.pnpm/@posthog+icons@0.36.6_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 335.6 KiB | public/hedgehog/desk-hog.png |
| 323.2 KiB | public/hedgehog/3-bears-hogs.png |
Posted automatically by check-eager-graph · sizes are input-source bytes from the esbuild metafile · part of #32479
…ope (P3)
Adds filters to the query endpoint and facade: [{key, op, value, scope}]
ANDed into the WHERE clause through attribute_field() — the single seam
the schema-v2 rewrite will swap. Ops: eq, neq, regex, not_regex (RE2);
negative ops also match rows lacking the key, mirroring Prometheus
negative matchers. scope defaults to auto (resource first, per-datapoint
fallback) so callers stay forward-compatible with the merged-label v2
layout.
How to validate manually:
- hogli test products/metrics/backend/tests/ (69 tests)
- with the local pipe running:
POST /api/environments/:id/metrics/query with filters
[{"key": "service_name", "op": "eq", "value": "metrics-ingestion"}]
returns the series; value "no-such" returns empty points
DanielVisca
force-pushed
the
posthog-code/metrics-ql2-series-shape
branch
from
4d9b5bc to
7db0be0
Compare
DanielVisca
force-pushed
the
posthog-code/metrics-ql3-label-filters
branch
from
9dc3731 to
4facf57
Compare
github-actions
Bot
dismissed
their stale review
New commits pushed (delta classified non_linear_history) — stamphog approval dismissed; re-review running automatically.
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
1 participant
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
Metric queries could only select by metric name — no way to scope to a service, pod, or endpoint. Filters are the first feature to flow through the
attribute_field()seam.Changes
filters: [{key, op, value, scope}]on the query body, ANDed into the WHERE clause viaattribute_field():eq",neq,regex,not_regex` (RE2). Negative ops also match rows lacking the key — same semantics as Prometheus negative matchers (documented on the serializer).scopedefaults toauto(resource attributes first, per-datapoint fallback) — forward-compatible with the schema-v2 merged-label layout where the distinction disappears.MetricAttributeScopeEnumtoENUM_NAME_OVERRIDES(the filter and group-by serializers share the choice set and collided).How did you test this code?
I'm an agent. Automated: parameterized runner tests for every op/scope combination against real ClickHouse (these also exercise the
__strsuffix path end-to-end — a filter on a seeded attribute only matches if the tag convention is right). Live check with the local pipe:🤖 Agent context
Autonomy: Human-driven (agent-assisted) — directed by @DanielVisca