Skip to content
Open
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
7 changes: 7 additions & 0 deletions .changeset/label-lookup-metric-name.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@hyperdx/api': patch
'@hyperdx/common-utils': patch
---

perf: PromQL label lookups with `match[]` read only the matched metrics' tags
rows when every selector names its metric
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,17 @@ describe('timeseriesEngine controller', () => {
).toEqual(['api', 'batch']);
});

// A selector without a metric name can match any metric, so naming
// `old_metric` in another selector mustn't narrow the lookup to it.
it('keeps the series of a selector without a metric name', async () => {
expect(
await labelValues({
labelName: 'job',
match: ['{job="web"}', 'old_metric'],
}),
).toEqual(['batch', 'web']);
});

it('scopes metric names to the selector', async () => {
expect(
await labelValues({
Expand Down
12 changes: 11 additions & 1 deletion packages/api/src/controllers/timeseriesEngine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
Metadata,
MetadataCache,
} from '@hyperdx/common-utils/dist/core/metadata';
import { promqlSelectorMetricName } from '@hyperdx/common-utils/dist/core/promqlSelector';
import {
PrometheusMatrixResult,
PrometheusVectorResult,
Expand Down Expand Up @@ -121,7 +122,16 @@ function getSeriesSelectorCondition({
)`,
);

return chSql`id IN (${concatChSql(' UNION DISTINCT ', idsPerSelector)})`;
const ids = chSql`id IN (${concatChSql(' UNION DISTINCT ', idsPerSelector)})`;
// The tags table is sorted by metric name, so `id IN (...)` alone reads every
// tags row. When each selector names its metric, say so: same rows, but only
// those metrics' part of the table is read.
const names = match
.map(promqlSelectorMetricName)
.filter((n): n is string => n != null);
if (names.length < match.length) return ids;

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.

🔵 minor — No test covers the new metric_name IN (...) clause or its fallback

Add a unit test on getSeriesSelectorCondition (or on the SQL that queryLabelValues/queryLabelNames builds) for three cases: all selectors named, which should add the metric_name filter with deduped names; one unnamed selector mixed in, which should leave only id IN (...); and two different named selectors, which should give metric_name IN (a, b). The existing timeseriesEngine.int.test.ts cases check only that results don't change, so they would still pass if the filter were never emitted (for example with an inverted names.length < match.length check), and they never exercise the mixed named/unnamed fallback.

Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond

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.

Added in 71e2aa6: labelValues for job with ['{job="web"}', 'old_metric'] expects ['batch', 'web'], and fails if a mixed list is narrowed to the named metric. I said earlier that the labelNames test covered this. It didn't: web's label names also appear on old_metric series.

Not adding a test on the generated SQL: the filter changes only speed, not results, and the parser's cases are unit-tested.

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.

🔵 minor — No test checks that the metric_name IN (...) filter is ever added, so the speed-up could vanish with every test still passing

By design the filter returns the same rows, so the integration tests in timeseriesEngine.int.test.ts give identical results whether it is added or not. If getSeriesSelectorCondition always returned ids, or the names.length < match.length check were flipped, every test would still pass. The only new integration test covers the case where no filter is added. Also, no integration test runs a braced {__name__="x"} selector through ClickHouse. Fix: export getSeriesSelectorCondition (or move it into a small module) and add unit tests on the generated ChSql. Check that the SQL contains metric_name IN with the deduplicated names when every selector names its metric (bare, name{...} and {__name__="x"} forms), and that it is left out when any selector does not name one.

Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond

const list = [...new Set(names)].map(n => chSql`${{ String: n }}`);
return chSql`metric_name IN (${concatChSql(', ', list)}) AND ${ids}`;
}

/**
Expand Down
27 changes: 27 additions & 0 deletions packages/common-utils/src/core/__tests__/promqlSelector.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { promqlSelectorMetricName } from '@/core/promqlSelector';

describe('promqlSelectorMetricName', () => {
it.each([
['up', 'up'],
[' node_cpu:rate5m{job="x"}', 'node_cpu:rate5m'],
['{__name__="up"}', 'up'],
['{job="x", __name__="up"}', 'up'],
["{job=~'a|b', env!=`x`, __name__='up'}", 'up'],
])('reads the metric of %s', (selector, name) => {
expect(promqlSelectorMetricName(selector)).toBe(name);
});

it.each([
'{job="x"}',
'{__name__=~"up|down"}',
'{job="x", __name__!="up"}',
'{job="__name__=\\"up\\""}',
`{job='a,__name__="up"'}`,
'{job=`a,__name__="up"`}',
'{__name__="a\\"b"}',
'sum(up)',
'up{',
])('returns undefined for %s', selector => {
expect(promqlSelectorMetricName(selector)).toBeUndefined();
});
});
41 changes: 41 additions & 0 deletions packages/common-utils/src/core/promqlSelector.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { parser } from '@prometheus-io/lezer-promql';

/**
* The metric a series selector is limited to: `name`, `name{...}`, or
* `{__name__="name", ...}`. Undefined when the selector may match more than
* one metric, isn't a single plain selector, or doesn't parse.
*/
export function promqlSelectorMetricName(selector: string): string | undefined {
const tree = parser.parse(selector);
let parseError = false;
tree.iterate({
enter: node => {
if (node.type.isError) parseError = true;
},
});
const vectorSelector = tree.topNode.firstChild;
if (parseError || vectorSelector?.name !== 'VectorSelector') return;

const identifier = vectorSelector.getChild('Identifier');
if (identifier) return selector.slice(identifier.from, identifier.to);

const matchers =
vectorSelector
.getChild('LabelMatchers')
?.getChildren('UnquotedLabelMatcher') ?? [];
for (const matcher of matchers) {
const name = matcher.getChild('LabelName');
const op = matcher.getChild('MatchOp')?.firstChild;
const value = matcher.getChild('StringLiteral');
if (
name &&
value &&
op?.name === 'EqlSingle' &&
selector.slice(name.from, name.to) === '__name__'
) {
// Values with escapes would need unquoting; fall back instead.
const text = selector.slice(value.from + 1, value.to - 1);
return text.includes('\\') ? undefined : text;
}
}
}
Loading