diff --git a/products/metrics/backend/metric_query_runner.py b/products/metrics/backend/metric_query_runner.py index 108a04e157b2..832063913297 100644 --- a/products/metrics/backend/metric_query_runner.py +++ b/products/metrics/backend/metric_query_runner.py @@ -416,6 +416,15 @@ def __init__( if aggregation == "histogram_quantile": if quantile is None or not 0.0 < quantile < 1.0: raise ValueError("histogram_quantile requires a quantile in (0, 1)") + # A summary stores quantile->value pairs in histogram_bounds/histogram_counts, + # not bucket counts, and scalar gauge/sum rows carry no distribution at all. + # Reading either as a bucket distribution returns garbage, so require an + # explicit histogram-compatible type rather than only rejecting summary. + if metric_type not in (MetricType.HISTOGRAM.value, MetricType.EXPONENTIAL_HISTOGRAM.value): + raise ValueError( + "histogram_quantile requires metric_type to be histogram or exponential_histogram; " + "summary metrics store quantile pairs (use p95) and scalar metrics have no distribution" + ) self.team = team self.metric_name = metric_name diff --git a/products/metrics/backend/tests/test_metric_query_runner.py b/products/metrics/backend/tests/test_metric_query_runner.py index d59db705c736..d97ffa92fbad 100644 --- a/products/metrics/backend/tests/test_metric_query_runner.py +++ b/products/metrics/backend/tests/test_metric_query_runner.py @@ -141,6 +141,42 @@ def test_rejects_interval_exceeding_row_budget(self): interval="second", ) + @parameterized.expand( + [ + # An untyped request admits summary rows, whose quantile->value pairs + # would be read as bucket counts. + ("untyped", None), + ("summary", "summary"), + # Gauge and sum rows have no bucket distribution to interpolate. + ("gauge", "gauge"), + ("sum", "sum"), + ] + ) + def test_rejects_histogram_quantile_for_non_histogram_types(self, _name: str, metric_type: str | None): + with self.assertRaises(ValueError): + MetricQueryRunner( + team=self.team, + metric_name="request_duration", + aggregation="histogram_quantile", + date_from=timezone.now() - dt.timedelta(hours=1), + date_to=timezone.now(), + quantile=0.95, + metric_type=metric_type, + ) + + @parameterized.expand([("histogram", "histogram"), ("exponential_histogram", "exponential_histogram")]) + def test_accepts_histogram_quantile_for_histogram_types(self, _name: str, metric_type: str): + runner = MetricQueryRunner( + team=self.team, + metric_name="request_duration", + aggregation="histogram_quantile", + date_from=timezone.now() - dt.timedelta(hours=1), + date_to=timezone.now(), + quantile=0.95, + metric_type=metric_type, + ) + self.assertEqual(runner.metric_type, metric_type) + def test_rejects_invalid_regex_filter(self): now = timezone.now() runner = MetricQueryRunner( @@ -1338,6 +1374,12 @@ def test_requires_quantile(self): with self.assertRaises(ValueError): self._run(quantile=1.5) + def test_rejects_summary_metric_type(self): + # A summary stores quantile->value pairs in histogram_bounds/histogram_counts, + # not bucket counts. Interpolating them as a distribution returns garbage. + with self.assertRaisesRegex(ValueError, "summary"): + self._run(metric_type="summary") + def test_group_by_service_name_column(self): # Pass `service_name` to the outer group-by. self._seed_histogram([(self.anchor, [10, 0, 0, 0])], temporality="delta", service_name="svc-a") diff --git a/products/metrics/frontend/components/MetricsClauseRow.tsx b/products/metrics/frontend/components/MetricsClauseRow.tsx index bc108382ddce..f3b22821d46f 100644 --- a/products/metrics/frontend/components/MetricsClauseRow.tsx +++ b/products/metrics/frontend/components/MetricsClauseRow.tsx @@ -28,10 +28,22 @@ const AGGREGATION_OPTIONS: { value: MetricAggregation; label: string }[] = [ { value: 'min', label: 'Min' }, { value: 'max', label: 'Max' }, { value: 'p95', label: 'p95' }, + { value: 'histogram_quantile', label: 'Histogram p95' }, { value: 'rate', label: 'Rate (/s)' }, { value: 'increase', label: 'Increase' }, ] +// The backend reads histogram_quantile from the bucket distribution +// (histogram_bounds/histogram_counts). Only histogram-shaped series carry one; +// summaries store quantile pairs and scalar rows carry none, so offering the +// option for those types produces a 400 or an empty chart. +const HISTOGRAM_QUANTILE_TYPES = new Set(['histogram', 'exponential_histogram']) + +const aggregationOptionsForType = (metricType?: string | null): { value: MetricAggregation; label: string }[] => + metricType && HISTOGRAM_QUANTILE_TYPES.has(metricType) + ? AGGREGATION_OPTIONS + : AGGREGATION_OPTIONS.filter((option) => option.value !== 'histogram_quantile') + /** One query line of the viewer: alias, metric picker, aggregation, filters, and group-by. * Editing any control focuses the row — the samples panel, anomaly badge, and picker * scoping follow the focused (active) clause. */ @@ -142,7 +154,7 @@ export function MetricsClauseRow({ { logic.actions.openMetric(CATALOG_ITEMS[0]) // a histogram expect(metricsViewerLogic.values.activeClause.selectedMetricType).toBe('histogram') - // The type drives the default aggregation: a histogram charts as a percentile. - expect(metricsViewerLogic.values.activeClause.aggregation).toBe('p95') + // The type drives the default aggregation: a histogram charts from its + // bucket distribution, not the cumulative sum in the scalar value column. + expect(metricsViewerLogic.values.activeClause.aggregation).toBe('histogram_quantile') }) }) diff --git a/products/metrics/frontend/components/metricsStarterDashboardLogic.test.ts b/products/metrics/frontend/components/metricsStarterDashboardLogic.test.ts index 1c781850ca41..064fd078b83a 100644 --- a/products/metrics/frontend/components/metricsStarterDashboardLogic.test.ts +++ b/products/metrics/frontend/components/metricsStarterDashboardLogic.test.ts @@ -103,9 +103,12 @@ describe('metricsStarterDashboardLogic', () => { }, ], }) - // The node schema has no 'p95' — the recommended shorthand maps to quantile + 0.95. + // A histogram reads its bucket distribution: quantile over the scalar value + // column would chart a percentile of cumulative observation sums. expect(histogramInsight.query).toMatchObject({ - clauses: [expect.objectContaining({ aggregation: 'quantile', quantile: 0.95, metricType: 'histogram' })], + clauses: [ + expect.objectContaining({ aggregation: 'histogram_quantile', quantile: 0.95, metricType: 'histogram' }), + ], }) }) diff --git a/products/metrics/frontend/components/metricsViewerLogic.test.ts b/products/metrics/frontend/components/metricsViewerLogic.test.ts index a094e8dd699d..6de0b703b9e8 100644 --- a/products/metrics/frontend/components/metricsViewerLogic.test.ts +++ b/products/metrics/frontend/components/metricsViewerLogic.test.ts @@ -98,7 +98,7 @@ describe('metricsViewerLogic', () => { it.each([ ['requests_total', 'increase'], ['queue_depth', 'avg'], - ['request_duration', 'p95'], + ['request_duration', 'histogram_quantile'], ])('selecting %s applies the type-appropriate aggregation %s', (metricName, expected) => { logic.actions.setMetricName(metricName) expect(logic.values.aggregation).toBe(expected) @@ -120,6 +120,9 @@ describe('metricsViewerLogic', () => { // metricsQueryNode is what "Save as insight" persists: a wrong mapping here // silently saves insights that re-run a different query than the viewer showed. + // A histogram must persist as histogram_quantile: ingest stores the cumulative + // observation sum in the scalar value column, so plain quantile/p95 would chart + // a quantile of ever-growing sums instead of the latency distribution. it('maps viewer state to a MetricsQuery node, translating p95 to quantile', () => { logic.actions.setMetricName('request_duration') logic.actions.setGroupByKeys(['container']) @@ -128,14 +131,14 @@ describe('metricsViewerLogic', () => { ) logic.actions.setDateFrom('-24h') - expect(logic.values.aggregation).toBe('p95') + expect(logic.values.aggregation).toBe('histogram_quantile') expect(logic.values.metricsQueryNode).toEqual({ kind: NodeKind.MetricsQuery, clauses: [ { name: 'a', metricName: 'request_duration', - aggregation: 'quantile', + aggregation: 'histogram_quantile', metricType: 'histogram', quantile: 0.95, filters: [{ key: 'namespace', op: 'eq', value: 'posthog' }], @@ -146,6 +149,28 @@ describe('metricsViewerLogic', () => { }) }) + it('maps a saved histogram_quantile node back to viewer vocabulary', () => { + logic.actions.setClauses( + [ + { + name: 'a', + metricName: 'request_duration', + selectedMetricType: 'histogram', + aggregation: 'histogram_quantile', + aggregationExplicitlySet: true, + filterGroup: { type: FilterLogicalOperator.And, values: [] }, + groupByKeys: [], + }, + ], + '' + ) + expect(logic.values.aggregation).toBe('histogram_quantile') + expect(logic.values.queryPayload?.clauses?.[0]).toMatchObject({ + aggregation: 'histogram_quantile', + quantile: 0.95, + }) + }) + it('produces no MetricsQuery node without a metric name', () => { expect(logic.values.metricsQueryNode).toBeNull() }) @@ -310,6 +335,14 @@ describe('metricsViewerLogic', () => { expect(logic.values.aggregation).toBe('avg') }) + it('the late backfill applies histogram_quantile for a histogram metric restored from a link', () => { + metricNamePickerLogic.actions.loadItemsSuccess([]) + logic.actions.setMetricName('request_duration') + expect(logic.values.aggregation).toBe('sum') + metricNamePickerLogic.actions.loadItemsSuccess(PICKER_ITEMS) + expect(logic.values.aggregation).toBe('histogram_quantile') + }) + it('the late backfill leaves an explicitly chosen aggregation alone', () => { metricNamePickerLogic.actions.loadItemsSuccess([]) logic.actions.setMetricName('queue_depth') diff --git a/products/metrics/frontend/components/metricsViewerLogic.tsx b/products/metrics/frontend/components/metricsViewerLogic.tsx index 566dad42129a..f27af5caf8b1 100644 --- a/products/metrics/frontend/components/metricsViewerLogic.tsx +++ b/products/metrics/frontend/components/metricsViewerLogic.tsx @@ -62,8 +62,27 @@ import type { MetricsChartSeries } from './metricsSeries' // A derived type ((typeof METRIC_AGGREGATIONS)[number]) would keep these in sync, but // kea-typegen inlines derived unions into every consumer's generated block — keep the // named alias so those blocks stay stable. -export type MetricAggregation = 'sum' | 'avg' | 'count' | 'min' | 'max' | 'p95' | 'rate' | 'increase' -export const METRIC_AGGREGATIONS: MetricAggregation[] = ['sum', 'avg', 'count', 'min', 'max', 'p95', 'rate', 'increase'] +export type MetricAggregation = + | 'sum' + | 'avg' + | 'count' + | 'min' + | 'max' + | 'p95' + | 'rate' + | 'increase' + | 'histogram_quantile' +export const METRIC_AGGREGATIONS: MetricAggregation[] = [ + 'sum', + 'avg', + 'count', + 'min', + 'max', + 'p95', + 'rate', + 'increase', + 'histogram_quantile', +] /** Narrows an untrusted value (a URL param, a saved link) to an aggregation the backend accepts. */ export const isMetricAggregation = (value: unknown): value is MetricAggregation => @@ -157,13 +176,16 @@ export const DEFAULT_AGGREGATION: MetricAggregation = 'sum' // Cumulative counters (OTel type 'sum') summed raw give meaningless ever-growing // totals — 'increase' is the honest default and is temporality-aware server-side // (delta samples are summed as-is), so it's correct for delta producers too. +// Histograms keep their distribution in histogram_bounds/histogram_counts; the +// scalar value column holds the cumulative observation sum, so only +// 'histogram_quantile' charts the actual distribution (e.g. request latency). export const RECOMMENDED_AGGREGATION_BY_TYPE: Record = { gauge: 'avg', sum: 'increase', counter: 'increase', - histogram: 'p95', + histogram: 'histogram_quantile', summary: 'p95', - exponential_histogram: 'p95', + exponential_histogram: 'histogram_quantile', } export const DEFAULT_DATE_FROM = '-1h' // Kept off the persisted node: a saved query with no `display` renders as a line chart anyway. @@ -261,11 +283,19 @@ export const toKnownMetricType = (metricType: string | undefined): OtelMetricTyp return metricType && known.includes(metricType) ? (metricType as OtelMetricTypeEnumApi) : null } -/** The REST viewer's 'p95' shorthand maps to the schema node's quantile aggregation. */ +/** The REST viewer's 'p95' shorthand maps to the schema node's quantile aggregation; + * 'histogram_quantile' keeps its name and carries the viewer's fixed 0.95 quantile. */ export const nodeAggregationFields = ( aggregation: MetricAggregation -): Pick => - aggregation === 'p95' ? { aggregation: 'quantile', quantile: 0.95 } : { aggregation } +): Pick => { + if (aggregation === 'p95') { + return { aggregation: 'quantile', quantile: 0.95 } + } + if (aggregation === 'histogram_quantile') { + return { aggregation: 'histogram_quantile', quantile: 0.95 } + } + return { aggregation } +} /** Inverse of `nodeAggregationFields`, for reading a saved node back in viewer vocabulary. */ export const viewerAggregationFromNode = (aggregation: MetricsQueryClause['aggregation'] | undefined): string | null => @@ -277,6 +307,8 @@ const clauseToApiClause = (clause: MetricsViewerClause): _MetricClauseApi => { name: clause.name, metricName: clause.metricName.trim(), aggregation: clause.aggregation, + // The backend requires a quantile for histogram_quantile; the viewer fixes it at 0.95. + ...(clause.aggregation === 'histogram_quantile' ? { quantile: 0.95 } : {}), // Pins the OTel type so a name that exists as several types (e.g. a counter // and a gauge) charts only the picked one instead of blending them. ...(clause.selectedMetricType ? { metricType: clause.selectedMetricType } : {}),