Skip to content

Commit eb2439e

Browse files
authored
fix(common-utils): skip metric points that have no recorded value (#3583)
## Summary When a scraped series goes away, or one scrape fails, the collector's `prometheus` receiver sends one more point with `FLAG_NO_RECORDED_VALUE` set. This is the OTLP form of a Prometheus staleness marker. The ClickHouse exporter stores it with `Flags = 1` and zeros: `Value = 0` for gauges and sums, zero bucket counts for histograms, and `Count = 0`, `Scale = 0` and no buckets for exponential histograms. The metric queries in `renderChartConfig` did not read `Flags`, so they used the marker as a real zero. Alerts use the same queries. Effects on `main`, measured in the new integration test: - Gauge: at the marker minute, `min` is 0 instead of 10, `avg` is 5 instead of 10, and `count` is 2 instead of 1. This is the case from #3511. - Sum (cumulative counter): when the series comes back after the marker, the next point is compared with the marker's 0. After one failed scrape on a counter at 120, `increase` shows 130 for the next minute instead of 10. - Histogram: the same reset. `count` is 15 instead of 5, and p50 is 7.5 although all new observations are in (10, 30]. - Exponential histogram: the same reset. Also, the marker's scale 0 makes the quantile query downscale every series to scale 0. p50 is 3.36 instead of a value in (4, 4.36]. Changes: - New `renderMetricWhere` in `renderChartConfig.ts` adds `bitAnd(Flags, 1) = 0` to the `WHERE` of the source CTE. The gauge, sum, histogram and exponential histogram paths use it, so single and multi-series metric charts and metric alerts all get it. The filter applies before the window functions, so the next point is compared with the last real point. - The filter is added only when the table has a `Flags` column of a non-`Nullable` integer type (`UInt32` in the OTel schema). The gauge and sum queries already read `Flags`, but `ReqMetricTableColumns` does not require it on histogram tables, and no table needs it to be an integer. So a custom table without it, or with another type, gets the same query as before. A failed column lookup also keeps the old query. The lookup reads the `DESCRIBE` result that the same query already caches, so it adds no ClickHouse query. Things to be aware of: - A flagged point never has a value, so results for data without markers do not change. The existing metric integration tests and unit snapshots pass without changes. - Not changed: PromQL charts (they read the TimeSeries tables), raw SQL charts, and the MCP tools that read only metric names, units and attribute keys. Summary metrics have no chart query. - A `Nullable` `Flags` column is skipped on purpose: `bitAnd(NULL, 1) = 0` is `NULL`, so the filter would drop rows whose `Flags` is `NULL`. - A counter that restarts behind a marker (120 → marker → 5) is now compared with 120, and the Sum query clamps the decrease to 0, as it does for every counter reset (see the comment on its `Source` CTE). On `main`, the marker's 0 was the baseline, so this one case read 5. A counter that comes back without a restart (one failed scrape) no longer counts its whole total again. Using the Prometheus reset rule (`Value` when it is below the previous point) would fix both, but it changes every counter reset, so I left it out of this PR. ### Tests - New `packages/common-utils/src/__tests__/metricNoRecordedValue.int.test.ts` (ClickHouse): one table per metric type with the OTel columns, and marker rows shaped like the receiver output (checked against `receiver/prometheusreceiver/internal/metricfamily.go`). 10 tests: gauge `min`, `avg` and `count` at the marker minute, the minute before it (a control), sum `increase`, a counter restart behind a marker against the same restart without one, histogram `count` (with the minute before the marker) and p50, and exponential histogram `count` (with the minute before the marker) and p50. - `renderChartConfig.test.ts`: the filter is in the source scan once for each metric type and aggFn when `Flags` is `UInt32`, and it is there for `UInt8`, `Int64` and `LowCardinality(UInt32)`. It is not there when a histogram table has no `Flags`, when `Flags` is `String`, `Nullable(UInt32)` or `LowCardinality(Nullable(UInt32))`, or when the lookup fails. With `renderChartConfig.ts` from `main`, 9 of the 10 integration tests fail (the control passes), and 9 of the 17 new unit tests fail (the "same query as before" cases pass on both). Without the type check, the 3 type tests fail. `renderChartConfig.test.ts` and `builderToRawSql.test.ts` (290 tests, no snapshot changes) and `queryChartConfig.int.test.ts` (81 tests) pass. ESLint (no new warnings) and Prettier pass. I ran the integration tests on ClickHouse 26.8.14.3 and 26.8.34.13; CI uses 26.8.36.3. I did not run the `api` integration tests (alerts, MCP), `tsc` for the whole package, or the UI steps below. I ran the SQL below against the OTel gauge schema on ClickHouse 26.8. ### How to reproduce **Starting point:** `yarn dev`, which creates the OTel metric tables in the `default` database of the local ClickHouse. **On `main`:** 1. Insert two series of one gauge for the last 6 minutes. Series `b` ends with a marker in the last minute: ```sql INSERT INTO default.otel_metrics_gauge (ServiceName, MetricName, ResourceAttributes, TimeUnix, Value, Flags) SELECT 'repro', 'repro.cpu', map('host', host), toStartOfMinute(now()) - toIntervalMinute(5 - m), if(host = 'b' AND m = 5, 0, 10), if(host = 'b' AND m = 5, 1, 0) FROM (SELECT arrayJoin(['a', 'b']) AS host) AS hosts CROSS JOIN (SELECT number AS m FROM numbers(6)) AS minutes ``` 2. Open **Chart Explorer**. Pick the metrics source and the gauge `repro.cpu` with the **Min** aggregation, over the last 15 minutes with a 1 minute granularity. 3. The last minute drops to 0. With **Avg** it drops to 5. **On this branch:** 1. Do the same steps. The last minute stays at 10 for **Min** and **Avg**. The new integration test covers the other metric types: `make dev-int-common-utils FILE=metricNoRecordedValue`. ### How to test on Vercel preview N/A — non-UI change. ### References - Closes #3511 - Related: #3499, #3500 and #3502 report other problems in the same metric queries. This PR does not change them.
1 parent dc58efd commit eb2439e

4 files changed

Lines changed: 541 additions & 4 deletions

File tree

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
'@hyperdx/common-utils': patch
3+
'@hyperdx/app': patch
4+
'@hyperdx/api': patch
5+
---
6+
7+
fix: leave metric points with no recorded value out of charts and alerts
8+
9+
When a scraped series goes away or a scrape fails, the collector's `prometheus`
10+
receiver sends a point flagged "no recorded value" (a Prometheus staleness
11+
marker), and the ClickHouse exporter stores it as zeros. Metric charts and
12+
alerts read it as a real value: a gauge's `min` and `avg` dropped and `count`
13+
included the series that went away, and a counter or histogram that came back
14+
after the marker counted its whole total again. Metric queries now skip points
15+
with this flag.
Lines changed: 366 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,366 @@
1+
import type { ClickHouseClient } from '@clickhouse/client';
2+
import { createClient } from '@clickhouse/client';
3+
4+
import { ClickhouseClient as HdxClickhouseClient } from '@/clickhouse/node';
5+
import { Metadata, MetadataCache } from '@/core/metadata';
6+
import {
7+
ChartConfigWithOptDateRange,
8+
DisplayType,
9+
MetricsDataType,
10+
} from '@/types';
11+
12+
// A point with OTLP FLAG_NO_RECORDED_VALUE (Flags bit 0) carries no value. The
13+
// collector's prometheus receiver sends one as a staleness marker when a series
14+
// goes away or a scrape fails, and the ClickHouse exporter stores it as zeros.
15+
describe('metric points with no recorded value (integration)', () => {
16+
let client: ClickHouseClient;
17+
let hdxClient: HdxClickhouseClient;
18+
let metadata: Metadata;
19+
20+
const DATABASE = 'default';
21+
const GAUGE_TABLE = 'no_recorded_value_gauge_int_test';
22+
const SUM_TABLE = 'no_recorded_value_sum_int_test';
23+
const HIST_TABLE = 'no_recorded_value_histogram_int_test';
24+
const EXP_HIST_TABLE = 'no_recorded_value_exp_histogram_int_test';
25+
26+
const NO_RECORDED_VALUE = 1;
27+
28+
// Inserts are server-local strings; the CI ClickHouse runs in UTC.
29+
const insertTs = (minute: number) => `2025-04-15 10:0${minute}:00`;
30+
const bucket = (minute: number) => `2025-04-15T10:0${minute}:00Z`;
31+
32+
// Columns of docker/otel-collector/schema/seed/00003_otel_metrics.sql that
33+
// every metric table shares.
34+
const COMMON_COLUMNS = `
35+
ResourceAttributes Map(LowCardinality(String), String),
36+
ResourceSchemaUrl String,
37+
ScopeName String,
38+
ScopeVersion String,
39+
ScopeAttributes Map(LowCardinality(String), String),
40+
ScopeDroppedAttrCount UInt32,
41+
ScopeSchemaUrl String,
42+
ServiceName LowCardinality(String),
43+
MetricName LowCardinality(String),
44+
MetricDescription String,
45+
MetricUnit String,
46+
Attributes Map(LowCardinality(String), String),
47+
StartTimeUnix DateTime,
48+
TimeUnix DateTime,
49+
Flags UInt32`;
50+
const ENGINE = `ENGINE = MergeTree PARTITION BY toDate(TimeUnix)
51+
ORDER BY (ServiceName, MetricName, toStartOfHour(TimeUnix), cityHash64(Attributes), TimeUnix)`;
52+
53+
const commonFields = (
54+
metricName: string,
55+
minute: number,
56+
service: string,
57+
flags = 0,
58+
) => ({
59+
ResourceAttributes: { 'service.name': service },
60+
ResourceSchemaUrl: '',
61+
ScopeName: '',
62+
ScopeVersion: '',
63+
ScopeAttributes: {},
64+
ScopeDroppedAttrCount: 0,
65+
ScopeSchemaUrl: '',
66+
ServiceName: service,
67+
MetricName: metricName,
68+
MetricDescription: '',
69+
MetricUnit: '',
70+
Attributes: {},
71+
StartTimeUnix: insertTs(0),
72+
TimeUnix: insertTs(minute),
73+
Flags: flags,
74+
});
75+
76+
const metricTables = {
77+
gauge: GAUGE_TABLE,
78+
sum: SUM_TABLE,
79+
histogram: HIST_TABLE,
80+
summary: 'unused',
81+
'exponential histogram': EXP_HIST_TABLE,
82+
};
83+
84+
const runConfig = async (
85+
metricType: MetricsDataType,
86+
metricName: string,
87+
select: { aggFn: string; level?: number },
88+
) => {
89+
const result = await hdxClient.queryChartConfig({
90+
config: {
91+
displayType: DisplayType.Line,
92+
connection: 'test-connection',
93+
metricTables,
94+
from: { databaseName: DATABASE, tableName: '' },
95+
select: [
96+
{
97+
...select,
98+
aggCondition: '',
99+
aggConditionLanguage: 'sql',
100+
valueExpression: 'Value',
101+
alias: 'Value',
102+
metricName,
103+
metricType,
104+
},
105+
],
106+
where: '',
107+
whereLanguage: 'sql',
108+
timestampValueExpression: 'TimeUnix',
109+
dateRange: [new Date('2025-04-14'), new Date('2025-04-16')],
110+
granularity: '1 minute',
111+
} as ChartConfigWithOptDateRange,
112+
metadata,
113+
querySettings: undefined,
114+
});
115+
return new Map(
116+
(result.data as Array<{ __hdx_time_bucket: string; Value: unknown }>).map(
117+
row => [row.__hdx_time_bucket, Number(row.Value)],
118+
),
119+
);
120+
};
121+
122+
beforeAll(async () => {
123+
const host = process.env.CLICKHOUSE_HOST || 'http://localhost:8123';
124+
const username = process.env.CLICKHOUSE_USER || 'default';
125+
const password = process.env.CLICKHOUSE_PASSWORD || '';
126+
127+
client = createClient({ url: host, username, password });
128+
hdxClient = new HdxClickhouseClient({ host, username, password });
129+
130+
await client.command({
131+
query: `CREATE OR REPLACE TABLE ${DATABASE}.${GAUGE_TABLE} (
132+
${COMMON_COLUMNS},
133+
Value Float64
134+
) ${ENGINE}`,
135+
});
136+
await client.command({
137+
query: `CREATE OR REPLACE TABLE ${DATABASE}.${SUM_TABLE} (
138+
${COMMON_COLUMNS},
139+
Value Float64,
140+
AggregationTemporality Int32,
141+
IsMonotonic Bool
142+
) ${ENGINE}`,
143+
});
144+
await client.command({
145+
query: `CREATE OR REPLACE TABLE ${DATABASE}.${HIST_TABLE} (
146+
${COMMON_COLUMNS},
147+
Count UInt64,
148+
Sum Float64,
149+
BucketCounts Array(UInt64),
150+
ExplicitBounds Array(Float64),
151+
Min Float64,
152+
Max Float64,
153+
AggregationTemporality Int32
154+
) ${ENGINE}`,
155+
});
156+
await client.command({
157+
query: `CREATE OR REPLACE TABLE ${DATABASE}.${EXP_HIST_TABLE} (
158+
${COMMON_COLUMNS},
159+
Count UInt64,
160+
Sum Float64,
161+
Scale Int32,
162+
ZeroCount UInt64,
163+
PositiveOffset Int32,
164+
PositiveBucketCounts Array(UInt64),
165+
NegativeOffset Int32,
166+
NegativeBucketCounts Array(UInt64),
167+
Min Float64,
168+
Max Float64,
169+
AggregationTemporality Int32
170+
) ${ENGINE}`,
171+
});
172+
173+
// Two series report 10 every minute; svc-b goes away after minute 4 and
174+
// leaves a marker at minute 5.
175+
const gaugeRow = (
176+
minute: number,
177+
service: string,
178+
value = 10,
179+
flags = 0,
180+
) => ({
181+
...commonFields('gauge.cpu', minute, service, flags),
182+
Value: value,
183+
});
184+
await client.insert({
185+
table: `${DATABASE}.${GAUGE_TABLE}`,
186+
values: [
187+
...[0, 1, 2, 3, 4, 5].map(m => gaugeRow(m, 'svc-a')),
188+
...[0, 1, 2, 3, 4].map(m => gaugeRow(m, 'svc-b')),
189+
gaugeRow(5, 'svc-b', 0, NO_RECORDED_VALUE),
190+
],
191+
format: 'JSONEachRow',
192+
});
193+
194+
const sumRow =
195+
(metricName: string) =>
196+
(minute: number, value: number, flags = 0) => ({
197+
...commonFields(metricName, minute, 'svc-a', flags),
198+
Value: value,
199+
AggregationTemporality: 2,
200+
IsMonotonic: true,
201+
});
202+
// A cumulative counter that goes up by 10 a minute. One scrape fails at
203+
// minute 3, then the same series comes back.
204+
const requests = sumRow('sum.requests');
205+
// A counter that restarts behind a marker, and the same restart without
206+
// one.
207+
const restart = sumRow('sum.restart');
208+
const restartControl = sumRow('sum.restart.control');
209+
await client.insert({
210+
table: `${DATABASE}.${SUM_TABLE}`,
211+
values: [
212+
requests(0, 100),
213+
requests(1, 110),
214+
requests(2, 120),
215+
requests(3, 0, NO_RECORDED_VALUE),
216+
requests(4, 130),
217+
restart(0, 100),
218+
restart(1, 120),
219+
restart(2, 0, NO_RECORDED_VALUE),
220+
restart(3, 5),
221+
restartControl(0, 100),
222+
restartControl(1, 120),
223+
restartControl(3, 5),
224+
],
225+
format: 'JSONEachRow',
226+
});
227+
228+
// Cumulative histogram: 10 observations in (0, 10] by minute 1, a marker
229+
// at minute 2, then 5 new observations in (10, 30] by minute 3.
230+
const histRow = (minute: number, bucketCounts: number[], flags = 0) => ({
231+
...commonFields('hist.latency', minute, 'svc-a', flags),
232+
Count: bucketCounts.reduce((a, b) => a + b, 0),
233+
Sum: 0,
234+
BucketCounts: bucketCounts,
235+
ExplicitBounds: [10, 30],
236+
Min: 0,
237+
Max: 0,
238+
AggregationTemporality: 2,
239+
});
240+
await client.insert({
241+
table: `${DATABASE}.${HIST_TABLE}`,
242+
values: [
243+
histRow(0, [0, 0, 0]),
244+
histRow(1, [10, 0, 0]),
245+
histRow(2, [0, 0, 0], NO_RECORDED_VALUE),
246+
histRow(3, [10, 5, 0]),
247+
],
248+
format: 'JSONEachRow',
249+
});
250+
251+
// Cumulative native histogram at scale 3, where bucket 8 is (2, 2.18] and
252+
// bucket 16 is (4, 4.36]: 10 observations in bucket 8 by minute 1, a
253+
// marker at minute 2, then 5 new observations in bucket 16 by minute 3.
254+
// The receiver writes a stale point with scale 0 and no buckets.
255+
const expHistRow = (minute: number, buckets: number[], flags = 0) => ({
256+
...commonFields('exphist.latency', minute, 'svc-a', flags),
257+
Count: buckets.reduce((a, b) => a + b, 0),
258+
Sum: 0,
259+
Scale: flags ? 0 : 3,
260+
ZeroCount: 0,
261+
PositiveOffset: flags ? 0 : 8,
262+
PositiveBucketCounts: buckets,
263+
NegativeOffset: 0,
264+
NegativeBucketCounts: [],
265+
Min: 0,
266+
Max: 0,
267+
AggregationTemporality: 2,
268+
});
269+
await client.insert({
270+
table: `${DATABASE}.${EXP_HIST_TABLE}`,
271+
values: [
272+
expHistRow(0, []),
273+
expHistRow(1, [10]),
274+
expHistRow(2, [], NO_RECORDED_VALUE),
275+
expHistRow(3, [10, 0, 0, 0, 0, 0, 0, 0, 5]),
276+
],
277+
format: 'JSONEachRow',
278+
});
279+
});
280+
281+
beforeEach(() => {
282+
metadata = new Metadata(hdxClient, new MetadataCache());
283+
});
284+
285+
afterAll(async () => {
286+
for (const table of [GAUGE_TABLE, SUM_TABLE, HIST_TABLE, EXP_HIST_TABLE]) {
287+
await client.command({
288+
query: `DROP TABLE IF EXISTS ${DATABASE}.${table}`,
289+
});
290+
}
291+
await hdxClient.close();
292+
await client.close();
293+
});
294+
295+
it.each([
296+
['min', 10],
297+
['avg', 10],
298+
['count', 1],
299+
])('leaves a gauge marker out of %s', async (aggFn, expected) => {
300+
const values = await runConfig(MetricsDataType.Gauge, 'gauge.cpu', {
301+
aggFn,
302+
});
303+
expect(values.get(bucket(5))).toBe(expected);
304+
});
305+
306+
it('keeps the gauge minute before the marker unchanged', async () => {
307+
const counts = await runConfig(MetricsDataType.Gauge, 'gauge.cpu', {
308+
aggFn: 'count',
309+
});
310+
expect(counts.get(bucket(4))).toBe(2);
311+
});
312+
313+
it('does not count a counter from zero after a marker', async () => {
314+
const values = await runConfig(MetricsDataType.Sum, 'sum.requests', {
315+
aggFn: 'increase',
316+
});
317+
expect(values.get(bucket(4))).toBe(10);
318+
expect([...values.values()].reduce((a, b) => a + b, 0)).toBe(30);
319+
});
320+
321+
// The Sum query clamps a counter decrease to 0, so the restart minute reads
322+
// 0 with and without the marker.
323+
it('treats a counter restart behind a marker like any other counter reset', async () => {
324+
const select = { aggFn: 'increase' };
325+
expect(await runConfig(MetricsDataType.Sum, 'sum.restart', select)).toEqual(
326+
await runConfig(MetricsDataType.Sum, 'sum.restart.control', select),
327+
);
328+
});
329+
330+
it('does not count a histogram from zero after a marker', async () => {
331+
const counts = await runConfig(MetricsDataType.Histogram, 'hist.latency', {
332+
aggFn: 'count',
333+
});
334+
expect(counts.get(bucket(1))).toBe(10);
335+
expect(counts.get(bucket(3))).toBe(5);
336+
});
337+
338+
it('computes histogram quantiles from the observations after a marker only', async () => {
339+
const p50 = await runConfig(MetricsDataType.Histogram, 'hist.latency', {
340+
aggFn: 'quantile',
341+
level: 0.5,
342+
});
343+
expect(p50.get(bucket(3))).toBeGreaterThan(10);
344+
expect(p50.get(bucket(3))).toBeLessThanOrEqual(30);
345+
});
346+
347+
it('does not count an exponential histogram from zero after a marker', async () => {
348+
const counts = await runConfig(
349+
MetricsDataType.ExponentialHistogram,
350+
'exphist.latency',
351+
{ aggFn: 'count' },
352+
);
353+
expect(counts.get(bucket(1))).toBe(10);
354+
expect(counts.get(bucket(3))).toBe(5);
355+
});
356+
357+
it('computes exponential histogram quantiles at the series scale from the observations after a marker only', async () => {
358+
const p50 = await runConfig(
359+
MetricsDataType.ExponentialHistogram,
360+
'exphist.latency',
361+
{ aggFn: 'quantile', level: 0.5 },
362+
);
363+
expect(p50.get(bucket(3))).toBeGreaterThan(4);
364+
expect(p50.get(bucket(3))).toBeLessThanOrEqual(2 ** (17 / 8));
365+
});
366+
});

0 commit comments

Comments
 (0)