app/vmestimator: add filter field to stream config - #29
Conversation
cf57145 to
0982af8
Compare
Add optional `filter` field to `EstimatorConfig` accepting a MetricsQL
label selector (e.g. `{job="api",env!~"dev|staging"}`). Only time series
matching all matchers are counted. Supports =, !=, =~, !~ operators.
The compiled filters are applied in `insertMany` before any HLL
insertion, with a fast path that skips the check entirely when no filter
is configured.
The active filter is exposed as a `filter` label on every
`cardinality_estimate` metric to prevent label collisions between
streams that share the same `group_by` and `interval` but target
different subsets of series.
revert
go fmt
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/vmestimator/estimator.go">
<violation number="1" location="app/vmestimator/estimator.go:57">
P2: Streams differing only by `filter` emit duplicate internal estimator metric label sets on `/metrics`, so Prometheus can reject the scrape as duplicate samples. Include `filter` in these four estimator metric names as well, or avoid registering per-filter copies.</violation>
<violation number="2" location="app/vmestimator/estimator.go:80">
P1: A MetricsQL selector containing `or` silently drops every branch after the first, producing an underestimate rather than rejecting unsupported syntax. Reject filters with multiple `LabelFilterss` or implement OR matching before constructing the estimator.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
@makasim I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/vmestimator/config.go">
<violation number="1" location="app/vmestimator/config.go:105">
P3: Creating streams recompiles each regexp after `metricsql.Parse` already validated it and bypasses MetricsQL's shared regexp cache. Reuse `metricsql.CompileRegexpAnchored` to preserve identical anchoring without duplicate compilation.</violation>
</file>
<file name="app/vmestimator/snapshot.go">
<violation number="1" location="app/vmestimator/snapshot.go:101">
P2: In cluster/aggregation mode, snapshots are merged in snapshots.add using only GroupByKeysLabel as the key (not the new filter field). When two streams share the same group_by+interval but different filter values — exactly the scenario this PR introduces — their snapshots collapse into one entry, and Filter/MetricPrefix become last-wins (s.Filter = other.Filter). Per-group metrics remain correct because each sketch key embeds its own filter= prefix, but the single __group__ aggregate and group_limit lines are emitted with only one snapshot's filter, so the aggregate cardinality/limit for the other filtered stream is lost or mislabeled. Consider incorporating the filter into the aggregation key so distinct-filter streams are aggregated separately.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| } | ||
|
|
||
| s.Interval = other.Interval | ||
| s.Filter = other.Filter |
There was a problem hiding this comment.
P2: In cluster/aggregation mode, snapshots are merged in snapshots.add using only GroupByKeysLabel as the key (not the new filter field). When two streams share the same group_by+interval but different filter values — exactly the scenario this PR introduces — their snapshots collapse into one entry, and Filter/MetricPrefix become last-wins (s.Filter = other.Filter). Per-group metrics remain correct because each sketch key embeds its own filter= prefix, but the single group aggregate and group_limit lines are emitted with only one snapshot's filter, so the aggregate cardinality/limit for the other filtered stream is lost or mislabeled. Consider incorporating the filter into the aggregation key so distinct-filter streams are aggregated separately.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/vmestimator/snapshot.go, line 101:
<comment>In cluster/aggregation mode, snapshots are merged in snapshots.add using only GroupByKeysLabel as the key (not the new filter field). When two streams share the same group_by+interval but different filter values — exactly the scenario this PR introduces — their snapshots collapse into one entry, and Filter/MetricPrefix become last-wins (s.Filter = other.Filter). Per-group metrics remain correct because each sketch key embeds its own filter= prefix, but the single __group__ aggregate and group_limit lines are emitted with only one snapshot's filter, so the aggregate cardinality/limit for the other filtered stream is lost or mislabeled. Consider incorporating the filter into the aggregation key so distinct-filter streams are aggregated separately.</comment>
<file context>
@@ -97,6 +98,7 @@ func (s *snapshot) merge(other *snapshot) {
}
s.Interval = other.Interval
+ s.Filter = other.Filter
s.MetricPrefix = other.MetricPrefix
s.GroupLimit = other.GroupLimit
</file context>
|
@makasim I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/vmestimator/vmestimator.md">
<violation number="1" location="docs/vmestimator/vmestimator.md:195">
P3: The Cardinality Metrics prose still states that all `cardinality_estimate` metrics include only `interval`, `group_by_keys`, `group_by_values`, and static labels, but every example metric below it (updated in this PR) now also carries a `filter` label. Update the intro sentence to list `filter` so the prose matches the examples it is describing.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| For global estimates (no `group_by` configured), `group_by_keys` is `__global__` and `group_by_values` is omitted: | ||
| ``` | ||
| cardinality_estimate{interval="1h0m0s",group_by_keys="__global__"} 142300 | ||
| cardinality_estimate{interval="1h0m0s",filter="",group_by_keys="__global__"} 142300 |
There was a problem hiding this comment.
P3: The Cardinality Metrics prose still states that all cardinality_estimate metrics include only interval, group_by_keys, group_by_values, and static labels, but every example metric below it (updated in this PR) now also carries a filter label. Update the intro sentence to list filter so the prose matches the examples it is describing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/vmestimator/vmestimator.md, line 195:
<comment>The Cardinality Metrics prose still states that all `cardinality_estimate` metrics include only `interval`, `group_by_keys`, `group_by_values`, and static labels, but every example metric below it (updated in this PR) now also carries a `filter` label. Update the intro sentence to list `filter` so the prose matches the examples it is describing.</comment>
<file context>
@@ -181,15 +192,15 @@ All metrics include `interval`, `group_by_keys`, `group_by_values`, and any stat
For global estimates (no `group_by` configured), `group_by_keys` is `__global__` and `group_by_values` is omitted:
-cardinality_estimate{interval="1h0m0s",group_by_keys="global"} 142300
+cardinality_estimate{interval="1h0m0s",filter="",group_by_keys="global"} 142300
</file context>
1 participant
Add an optional
filterfield to stream config. It accepts a MetricsQL label selector (e.g.{job="api",env!~"dev|staging"}). Only time series matching all matchers are counted. Supports=,!=,=~,!~operators.Config:
The filter is exposed as a
filterlabel on everycardinality_estimatemetric to prevent label collisions between streams that share the samegroup_byandintervalbut target different subsets of series.I don't expect filters to be used frequently via the
-configflag. Their primary purpose is to support ad hoc profiling. For example, if an alert indicates that a particular job is experiencing a cardinality spike, the user could start a more detailed analysis for just that job. For example, by estimating label values cardinality or per metric name one. vmestimator would create temporary estimators with the specified filter, run them for a few minutes, and then return the results.