Skip to content
Closed
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
9 changes: 9 additions & 0 deletions products/metrics/backend/metric_query_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@


class MetricQueryRunner:
def __init__(

Check warning on line 386 in products/metrics/backend/metric_query_runner.py

View workflow job for this annotation

GitHub Actions / Python code quality (depot-ubuntu-24.04)

lint:complexity

`__init__` has cyclomatic complexity 11 (warn >10)

Check warning on line 386 in products/metrics/backend/metric_query_runner.py

View workflow job for this annotation

GitHub Actions / Python code quality (depot-ubuntu-24.04)

`__init__` has cyclomatic complexity 11 (warn >10)
self,
team: Team,
metric_name: str,
Expand Down Expand Up @@ -416,6 +416,15 @@
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
Expand Down
42 changes: 42 additions & 0 deletions products/metrics/backend/tests/test_metric_query_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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")
Expand Down
14 changes: 13 additions & 1 deletion products/metrics/frontend/components/MetricsClauseRow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,26 @@
{ value: 'min', label: 'Min' },
{ value: 'max', label: 'Max' },
{ value: 'p95', label: 'p95' },
{ value: 'histogram_quantile', label: 'Histogram p95' },

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.

P1 Unsupported aggregation remains selectable

Histogram p95 is offered for every metric type, but the backend rejects it for summaries and cannot derive it from scalar gauge or sum rows. Selecting it for those metrics returns a 400 or an empty chart. Filter this option to histogram and exponential-histogram clauses, as the repository requires the UI to show only supported options.

Rule Used: When implementing new features, ensure that the UI reflects only the supported options to avoid confusion for users, especially during beta testing. (source)

Learned From
PostHog/posthog#32677
PostHog/posthog#32595

Prompt To Fix With AI
This is a comment left during a code review.
Path: products/metrics/frontend/components/MetricsClauseRow.tsx
Line: 31

Comment:
**Unsupported aggregation remains selectable**

`Histogram p95` is offered for every metric type, but the backend rejects it for summaries and cannot derive it from scalar gauge or sum rows. Selecting it for those metrics returns a 400 or an empty chart. Filter this option to histogram and exponential-histogram clauses, as the repository requires the UI to show only supported options.

**Rule Used:** When implementing new features, ensure that the UI reflects only the supported options to avoid confusion for users, especially during beta testing. ([source](https://app.greptile.com/posthog-org-19734/-/custom-context?memory=5d57f0af-0be1-44de-8885-055f27e2885f))

**Learned From**
[PostHog/posthog#32677](https://github.com/PostHog/posthog/pull/32677)
[PostHog/posthog#32595](https://github.com/PostHog/posthog/pull/32595)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

{ 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. */
export function MetricsClauseRow({

Check warning on line 50 in products/metrics/frontend/components/MetricsClauseRow.tsx

View workflow job for this annotation

GitHub Actions / Frontend formatting

lint:complexity

`MetricsClauseRow` has cyclomatic complexity 12 (warn >10)
clause,
index,
isActive,
Expand Down Expand Up @@ -142,7 +154,7 @@
<LemonSelect
size="small"
value={clause.aggregation}
options={AGGREGATION_OPTIONS}
options={aggregationOptionsForType(clause.selectedMetricType)}
onChange={withSelect(setAggregation)}
data-attr="metrics-viewer-aggregation"
disabledReason={disabledReason}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,8 @@ describe('metricsCatalogLogic', () => {
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')
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -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' }),
],
})
})

Expand Down
39 changes: 36 additions & 3 deletions products/metrics/frontend/components/metricsViewerLogic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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'])
Expand All @@ -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' }],
Expand All @@ -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()
})
Expand Down Expand Up @@ -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')
Expand Down
46 changes: 39 additions & 7 deletions products/metrics/frontend/components/metricsViewerLogic.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,27 @@
// 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 =>
Expand Down Expand Up @@ -157,13 +176,16 @@
// 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<string, MetricAggregation> = {
gauge: 'avg',
sum: 'increase',
counter: 'increase',
histogram: 'p95',
histogram: 'histogram_quantile',
summary: 'p95',
exponential_histogram: 'p95',
exponential_histogram: 'histogram_quantile',

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.

P1 Negative buckets produce wrong quantiles

Defaulting exponential histograms to histogram_quantile produces incorrect results for valid distributions containing negative buckets. Ingest emits zero first and then negative buckets without sorting, while the quantile implementation assumes ascending bounds and interpolates between adjacent entries. Normalize or sort exponential bounds with their counts before making this aggregation the default.

Prompt To Fix With AI
This is a comment left during a code review.
Path: products/metrics/frontend/components/metricsViewerLogic.tsx
Line: 188

Comment:
**Negative buckets produce wrong quantiles**

Defaulting exponential histograms to `histogram_quantile` produces incorrect results for valid distributions containing negative buckets. Ingest emits zero first and then negative buckets without sorting, while the quantile implementation assumes ascending bounds and interpolates between adjacent entries. Normalize or sort exponential bounds with their counts before making this aggregation the default.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

}
export const DEFAULT_DATE_FROM = '-1h'
// Kept off the persisted node: a saved query with no `display` renders as a line chart anyway.
Expand Down Expand Up @@ -196,7 +218,7 @@
// Convert one filter-bar chip into the backend's `{key, op, value}` matcher. Filters run with
// scope 'auto' (resource attributes first, datapoint attributes as fallback), so scope is omitted.
// Returns null for chips still being edited (no key/value) or unsupported operators.
const propertyFilterToMetricFilter = (filter: UniversalFilterValue): _MetricFilterApi | null => {

Check warning on line 221 in products/metrics/frontend/components/metricsViewerLogic.tsx

View workflow job for this annotation

GitHub Actions / Frontend formatting

lint:complexity

`propertyFilterToMetricFilter` has cyclomatic complexity 13 (warn >10)
const key = 'key' in filter && filter.key ? String(filter.key) : ''
const operator = 'operator' in filter && filter.operator ? filter.operator : PropertyOperator.Exact
// A non-PropertyOperator value (e.g. an ActionFilter's fields) simply isn't in the map -> null.
Expand Down Expand Up @@ -261,11 +283,19 @@
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<MetricsQueryClause, 'aggregation' | 'quantile'> =>
aggregation === 'p95' ? { aggregation: 'quantile', quantile: 0.95 } : { aggregation }
): Pick<MetricsQueryClause, 'aggregation' | 'quantile'> => {
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 =>
Expand All @@ -277,6 +307,8 @@
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 } : {}),
Expand Down
Loading