From 762880b76aa930f9b5202f0e6999fbf43b8fff5c Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Wed, 26 Aug 2026 20:51:17 +0100 Subject: [PATCH 01/24] feat(insights): add box plots to SQL insights Add a Box plot option to SQL insights, gated behind the sql-box-plot-insight feature flag. Maps one row per X-axis and series pair into grouped boxes, auto-maps conventional names for minimum, percentiles, median, mean, and maximum, and keeps percentile calculation in SQL instead of using limited browser results. Adopted from PR #84558. Rebased onto master to resolve merge conflicts and squashed into a single commit. Generated-By: PostHog Desktop Task-Id: f1e69bea-f4e6-44cf-bd12-dc81f8a435c3 --- frontend/snapshots.yml | 8 +- frontend/src/lib/constants.tsx | 1 + .../projects/team_id/insights/sqlBoxPlot.json | 208 +++++++++++++++ .../Components/BoxPlotSeriesTab.test.tsx | 107 ++++++++ .../Components/BoxPlotSeriesTab.tsx | 88 ++++++ .../Components/Charts/SqlBoxPlot.test.tsx | 147 ++++++++++ .../Components/Charts/SqlBoxPlot.tsx | 115 ++++++++ .../Charts/sqlBoxPlotAdapter.test.ts | 251 ++++++++++++++++++ .../Components/Charts/sqlBoxPlotAdapter.ts | 231 ++++++++++++++++ .../Components/DisplayTab.test.tsx | 48 ++++ .../Components/DisplayTab.tsx | 45 ++-- .../Components/SeriesTab.tsx | 5 + .../Components/TableDisplay.test.tsx | 80 ++++++ .../Components/TableDisplay.tsx | 14 + .../DataVisualization/DataVisualization.tsx | 13 + .../dataVisualizationLogic.test.ts | 34 +++ .../dataVisualizationLogic.ts | 20 ++ frontend/src/queries/schema.json | 36 +++ frontend/src/queries/schema/schema-general.ts | 13 + .../insights/stories/SQLBoxPlot.stories.tsx | 109 ++++++++ packages/quill/packages/charts/AGENTS.md | 2 +- .../src/charts/BoxPlot/BoxPlot.stories.tsx | 7 +- .../src/charts/BoxPlot/BoxPlot.test.tsx | 8 + .../charts/src/charts/BoxPlot/BoxPlot.tsx | 80 ++++-- posthog/schema.py | 16 ++ .../frontend/generated/api.schemas.ts | 13 + .../backend/presentation/insight.py | 11 +- .../backend/tests/api/test_insight_query.py | 18 ++ .../frontend/generated/api.schemas.ts | 13 + services/mcp/src/api/generated.ts | 13 + 30 files changed, 1707 insertions(+), 47 deletions(-) create mode 100644 frontend/src/mocks/fixtures/api/projects/team_id/insights/sqlBoxPlot.json create mode 100644 frontend/src/queries/nodes/DataVisualization/Components/BoxPlotSeriesTab.test.tsx create mode 100644 frontend/src/queries/nodes/DataVisualization/Components/BoxPlotSeriesTab.tsx create mode 100644 frontend/src/queries/nodes/DataVisualization/Components/Charts/SqlBoxPlot.test.tsx create mode 100644 frontend/src/queries/nodes/DataVisualization/Components/Charts/SqlBoxPlot.tsx create mode 100644 frontend/src/queries/nodes/DataVisualization/Components/Charts/sqlBoxPlotAdapter.test.ts create mode 100644 frontend/src/queries/nodes/DataVisualization/Components/Charts/sqlBoxPlotAdapter.ts create mode 100644 frontend/src/queries/nodes/DataVisualization/Components/TableDisplay.test.tsx create mode 100644 frontend/src/scenes/insights/stories/SQLBoxPlot.stories.tsx diff --git a/frontend/snapshots.yml b/frontend/snapshots.yml index a5ca94ae4f56..f80686b967ab 100644 --- a/frontend/snapshots.yml +++ b/frontend/snapshots.yml @@ -661,9 +661,9 @@ snapshots: components-hogcharts-barchart--with-value-labels--light: hash: v1.k794b7964.6aff50b1c147df21230d149c147deb023f6308cd2e01983b58e65461dd4c7207.vLZ6XqXFhSTA3hr10RUEv9vzOgxKWYtUzWj0OvoCTU0 components-hogcharts-boxplot--multi-series-grouped--dark: - hash: v1.k794b7964.100052bfc106ff42b501157a182699d6814f90be488c4e03ff6d7476fdd05cbe.bLaFB-wXyDHV1bbsR0ssdrE6qYaPmFcUTXnYzWDasY0 + hash: v1.k794b7964.fd8a8c11e35ef420a663807a21a6349eba0ae2658c6c3d114763f4629dd45934.d23KBypemM2QY55g8_QWVUc9diJoAL1oNZjK3d3Ebs4 components-hogcharts-boxplot--multi-series-grouped--light: - hash: v1.k794b7964.54b13669ffe5a6341141893094ff6887b677688ab89242bc82217f54129fa7a3.TjWyBhlbZbDBnspp8YhHvJCM5Bu146okpGeZRzWrNKs + hash: v1.k794b7964.016f9887360412c0b22e1b45e677ab2c289961ce86382e4bb7287f7d8c5dd110.cuv1S6OAl-253E0RJgLwIzHtVxH1ji5mzIOT3GTBWnk components-hogcharts-boxplot--no-grid--dark: hash: v1.k794b7964.ec49b9672a800158cc0dd7f1a517dfec0912d03ce7bc8b56c9a08492fdef723f.5tbdsdN5AB0mtLjoNobhw1skLsS4m5Sc6Es7TSdhTWg components-hogcharts-boxplot--no-grid--light: @@ -7032,6 +7032,10 @@ snapshots: hash: v1.k794b7964.e0482531b63e3c6c58bb438b0848b476db1ebadb82736444afb0004e5a7d9a2e.EgJaf86aq6hyYkPzdF8Aydv7rycYiRr_UV8fYh_LIL0 scenes-app-insights-side-panel-actions--unsaved-insight--light: hash: v1.k794b7964.2b3ed2b5ffec763b0f7e13af74c9e9d57df96464976ae6014512a0d7def42ad3.paLp2yPHgkf2-ncauikOAUdR87k6-KhAh-Nm4uyQ380 + scenes-app-insights-sqlboxplot--grouped-series--dark: + hash: v1.k794b7964.9958e584a1eb346036636cec5e7f6aa21367045261cec6f9bc9474570976916f.WYCNc1X4istzHZnAsqYBx9xNFe0Jek1UegIJ-J-eSog + scenes-app-insights-sqlboxplot--grouped-series--light: + hash: v1.k794b7964.fce0c9ba15afa1bfe37728e10742395345a06e2a082d34a145e32c80a41432a8.lF9z6z4b7ptkoQzmfE7tlnr7CJQJ7Ki3rypJuW903Zs scenes-app-insights-sqllinechart--sql-bar-chart-value-labels-quill--dark: hash: v1.k794b7964.482f71c0aadde604594294d6e723590838e5b47f543adf3baf39c5ad2f2a9c3e.bpFdUZ6dFKKinvbM5jklQHy3pQoeURgjrWtjcXipKBw scenes-app-insights-sqllinechart--sql-bar-chart-value-labels-quill--light: diff --git a/frontend/src/lib/constants.tsx b/frontend/src/lib/constants.tsx index 942f53736959..3ea309481cc7 100644 --- a/frontend/src/lib/constants.tsx +++ b/frontend/src/lib/constants.tsx @@ -202,6 +202,7 @@ export const FEATURE_FLAGS = { REPLAY_EXCLUDE_FROM_HIDE_RECORDINGS_MENU: 'replay-exclude-from-hide-recordings-menu', // owner: #team-replay, used to exclude what other people are seeing in Replay SHOW_UPGRADE_TO_MANAGED_ACCOUNT: 'show-upgrade-to-managed-account', // owner: #team-billing, used to give free accounts a way to force upgrade to managed account WEBHOOKS_DENYLIST: 'webhooks-denylist', // owner: #team-ingestion, used to disable webhooks for certain companies + SQL_BOX_PLOT_INSIGHT: 'sql-box-plot-insight', // owner: @pauldambra #team-product-analytics // Legacy flags, TBD if they need to be removed BATCH_EXPORTS_POSTHOG_HTTP: 'posthog-http-batch-exports', // owner: #team-batch-exports diff --git a/frontend/src/mocks/fixtures/api/projects/team_id/insights/sqlBoxPlot.json b/frontend/src/mocks/fixtures/api/projects/team_id/insights/sqlBoxPlot.json new file mode 100644 index 000000000000..b0bf619e33c8 --- /dev/null +++ b/frontend/src/mocks/fixtures/api/projects/team_id/insights/sqlBoxPlot.json @@ -0,0 +1,208 @@ +{ + "id": 31, + "short_id": "boxPlot", + "name": "Latency distribution by plan", + "derived_name": null, + "filters": {}, + "query": { + "kind": "DataVisualizationNode", + "source": { + "kind": "HogQLQuery", + "query": "SELECT\n toStartOfWeek(timestamp) AS bucket,\n properties.plan AS series,\n min(toFloat(properties.latency)) AS min,\n quantile(0.25)(toFloat(properties.latency)) AS p25,\n quantile(0.5)(toFloat(properties.latency)) AS median,\n avg(toFloat(properties.latency)) AS mean,\n quantile(0.75)(toFloat(properties.latency)) AS p75,\n max(toFloat(properties.latency)) AS max\nFROM events\nGROUP BY bucket, series\nORDER BY bucket, series" + }, + "display": "BoxPlot", + "chartSettings": { + "boxPlot": { + "xAxisColumn": "bucket", + "seriesColumn": "series", + "minColumn": "min", + "p25Column": "p25", + "medianColumn": "median", + "meanColumn": "mean", + "p75Column": "p75", + "maxColumn": "max", + "excludeOutliers": true + }, + "showLegend": true, + "leftYAxisSettings": { + "label": "Latency (ms)", + "showGridLines": true + } + } + }, + "order": null, + "deleted": false, + "dashboards": [], + "dashboard_tiles": [], + "last_refresh": "2026-03-04T16:10:59.052768Z", + "cache_target_age": "2026-03-04T22:10:59.052768Z", + "next_allowed_client_refresh": "2026-03-04T16:11:59.052768Z", + "result": [ + [ + "2026-01-05", + "Free", + 80, + 120, + 170, + 195, + 240, + 620 + ], + [ + "2026-01-05", + "Paid", + 45, + 70, + 95, + 110, + 135, + 310 + ], + [ + "2026-01-12", + "Free", + 75, + 115, + 165, + 185, + 225, + 560 + ], + [ + "2026-01-12", + "Paid", + 40, + 65, + 88, + 102, + 125, + 280 + ], + [ + "2026-01-19", + "Free", + 90, + 135, + 190, + 215, + 265, + 710 + ], + [ + "2026-01-19", + "Paid", + 50, + 75, + 105, + 118, + 145, + 340 + ], + [ + "2026-01-26", + "Free", + 70, + 105, + 150, + 172, + 210, + 490 + ], + [ + "2026-01-26", + "Paid", + 38, + 60, + 82, + 96, + 118, + 250 + ] + ], + "hasMore": false, + "columns": [ + "bucket", + "series", + "min", + "p25", + "median", + "mean", + "p75", + "max" + ], + "created_at": "2026-03-04T16:11:08.460573Z", + "created_by": { + "id": 1, + "uuid": "019c66e6-fd77-0000-a6bb-509ac38f988e", + "distinct_id": "otSl3FZiAxw3Vv8GUTeWtZcV3mdpR0lH9pprZmBRKXd", + "first_name": "Employee 427", + "last_name": "", + "email": "test@posthog.com", + "is_email_verified": null, + "hedgehog_config": null, + "role_at_organization": "engineering" + }, + "description": null, + "updated_at": "2026-03-04T16:11:08.460609Z", + "favorited": false, + "saved": true, + "last_modified_at": "2026-03-04T16:11:08.456506Z", + "last_modified_by": { + "id": 1, + "uuid": "019c66e6-fd77-0000-a6bb-509ac38f988e", + "distinct_id": "otSl3FZiAxw3Vv8GUTeWtZcV3mdpR0lH9pprZmBRKXd", + "first_name": "Employee 427", + "last_name": "", + "email": "test@posthog.com", + "is_email_verified": null, + "hedgehog_config": null, + "role_at_organization": "engineering" + }, + "is_sample": false, + "effective_restriction_level": 21, + "effective_privilege_level": 37, + "user_access_level": "manager", + "timezone": "UTC", + "is_cached": true, + "query_status": null, + "hogql": "SELECT\n toStartOfWeek(timestamp) AS bucket,\n properties.plan AS series,\n min(toFloat(properties.latency)) AS min,\n quantile(0.25)(toFloat(properties.latency)) AS p25,\n quantile(0.5)(toFloat(properties.latency)) AS median,\n avg(toFloat(properties.latency)) AS mean,\n quantile(0.75)(toFloat(properties.latency)) AS p75,\n max(toFloat(properties.latency)) AS max\nFROM events\nGROUP BY bucket, series\nORDER BY bucket, series\nLIMIT 101\nOFFSET 0", + "types": [ + [ + "bucket", + "Date" + ], + [ + "series", + "String" + ], + [ + "min", + "Float64" + ], + [ + "p25", + "Float64" + ], + [ + "median", + "Float64" + ], + [ + "mean", + "Float64" + ], + [ + "p75", + "Float64" + ], + [ + "max", + "Float64" + ] + ], + "resolved_date_range": null, + "alerts": [], + "last_viewed_at": "2026-03-04T16:11:30.018817Z", + "tags": [], + "filters_hash": "cache_1_eb04b61fff884f29a4c74600aea3684fb7769ff1fa6e0d4e59865ee187c3b58b" +} diff --git a/frontend/src/queries/nodes/DataVisualization/Components/BoxPlotSeriesTab.test.tsx b/frontend/src/queries/nodes/DataVisualization/Components/BoxPlotSeriesTab.test.tsx new file mode 100644 index 000000000000..c9ed5f94f432 --- /dev/null +++ b/frontend/src/queries/nodes/DataVisualization/Components/BoxPlotSeriesTab.test.tsx @@ -0,0 +1,107 @@ +import '@testing-library/jest-dom' + +import { render, screen, waitFor } from '@testing-library/react' +import userEvent from '@testing-library/user-event' +import { BindLogic } from 'kea' + +import { DataVisualizationNode, HogQLQueryResponse, NodeKind } from '~/queries/schema/schema-general' +import { initKeaTests } from '~/test/init' +import { ChartDisplayType } from '~/types' + +import { dataNodeLogic } from '../../DataNode/dataNodeLogic' +import { DataVisualizationLogicProps, dataVisualizationLogic } from '../dataVisualizationLogic' +import { BoxPlotSeriesTab } from './BoxPlotSeriesTab' + +const query: DataVisualizationNode = { + kind: NodeKind.DataVisualizationNode, + source: { kind: NodeKind.HogQLQuery, query: 'select * from summaries' }, + display: ChartDisplayType.BoxPlot, + chartSettings: { + boxPlot: { + xAxisColumn: 'bucket', + minColumn: 'min', + p25Column: 'p25', + medianColumn: 'median', + meanColumn: 'mean', + p75Column: 'p75', + maxColumn: 'max', + }, + }, +} + +const cachedResults: HogQLQueryResponse = { + results: [['Mon', 1, 2, 3, 4, 5, 6, 0]], + columns: ['bucket', 'min', 'p25', 'median', 'mean', 'p75', 'max', 'alternate_min'], + types: [ + ['bucket', 'String'], + ['min', 'Float64'], + ['p25', 'Float64'], + ['median', 'Float64'], + ['mean', 'Float64'], + ['p75', 'Float64'], + ['max', 'Float64'], + ['alternate_min', 'Float64'], + ], +} + +describe('BoxPlotSeriesTab', () => { + it('shows box plot roles and saves a changed statistic column', async () => { + initKeaTests() + const setQuery = jest.fn() + let currentQuery = query + const props: DataVisualizationLogicProps = { + key: 'box-plot-series-tab', + query: currentQuery, + cachedResults, + dataNodeCollectionId: 'box-plot-series-tab', + setQuery: (setter) => { + currentQuery = setter(currentQuery) + setQuery(currentQuery) + }, + } + + dataNodeLogic({ + key: props.key, + query: query.source, + cachedResults, + dataNodeCollectionId: props.dataNodeCollectionId, + }).mount() + dataVisualizationLogic(props).mount() + + const { container } = render( + + + + ) + + expect(screen.getByText('25th percentile')).toBeInTheDocument() + expect(screen.getByText('75th percentile')).toBeInTheDocument() + + const minimumSelect = container.querySelector('[data-attr="box-plot-minColumn"]') + if (!(minimumSelect instanceof HTMLElement)) { + throw new Error('Expected the minimum column selector') + } + + const user = userEvent.setup() + await user.click(minimumSelect) + await user.click(await screen.findByText('alternate_min')) + + await waitFor(() => expect(currentQuery.chartSettings?.boxPlot?.minColumn).toBe('alternate_min')) + + const xAxisSelect = container.querySelector('[data-attr="box-plot-x-axis-column"]') + if (!(xAxisSelect instanceof HTMLElement)) { + throw new Error('Expected the X-axis column selector') + } + + await user.click(xAxisSelect) + const noneOption = (await screen.findAllByText('None')).find( + (element) => !element.closest('[data-attr="box-plot-series-column"]') + ) + if (!noneOption) { + throw new Error('Expected the None option') + } + await user.click(noneOption) + + await waitFor(() => expect(currentQuery.chartSettings?.boxPlot?.xAxisColumn).toBeNull()) + }) +}) diff --git a/frontend/src/queries/nodes/DataVisualization/Components/BoxPlotSeriesTab.tsx b/frontend/src/queries/nodes/DataVisualization/Components/BoxPlotSeriesTab.tsx new file mode 100644 index 000000000000..1c1626b68e01 --- /dev/null +++ b/frontend/src/queries/nodes/DataVisualization/Components/BoxPlotSeriesTab.tsx @@ -0,0 +1,88 @@ +import { useActions, useValues } from 'kea' + +import { LemonBanner, LemonLabel, LemonSelect, LemonTag } from '@posthog/lemon-ui' + +import { BoxPlotSettings } from '~/queries/schema/schema-general' + +import { Column, dataVisualizationLogic } from '../dataVisualizationLogic' +import { BOX_PLOT_STATISTICS } from './Charts/sqlBoxPlotAdapter' + +const NONE_COLUMN = '__posthog_box_plot_none__' + +export const BoxPlotSeriesTab = (): JSX.Element => { + const { chartSettings, columns, numericalColumns, responseLoading } = useValues(dataVisualizationLogic) + const { updateChartSettings } = useActions(dataVisualizationLogic) + const settings = chartSettings.boxPlot ?? {} + + const updateSettings = (updates: Partial): void => { + updateChartSettings({ boxPlot: { ...settings, ...updates } }) + } + + const toColumnOption = ({ name, type }: Column): { value: string; label: JSX.Element } => ({ + value: name, + label: ( +
+ {name} + + {type.name} + +
+ ), + }) + + const columnOptions = columns.map(toColumnOption) + const numericalOptions = numericalColumns.map(toColumnOption) + const optionalColumnOptions = [{ value: NONE_COLUMN, label: 'None' }, ...columnOptions] + const disabledReason = responseLoading ? 'Query loading...' : undefined + + return ( +
+ + Return one row per box. Calculate the minimum, percentiles, mean, and maximum in SQL. + + +
+ X-axis + updateSettings({ xAxisColumn: value === NONE_COLUMN ? null : value })} + /> +
Optional when the query returns one row.
+
+ +
+ Series + updateSettings({ seriesColumn: value === NONE_COLUMN ? null : value })} + /> +
Optional. Each value becomes a separate series.
+
+ +
+ {BOX_PLOT_STATISTICS.map(({ setting, label }) => ( +
+ {label} + updateSettings({ [setting]: value })} + /> +
+ ))} +
+
+ ) +} diff --git a/frontend/src/queries/nodes/DataVisualization/Components/Charts/SqlBoxPlot.test.tsx b/frontend/src/queries/nodes/DataVisualization/Components/Charts/SqlBoxPlot.test.tsx new file mode 100644 index 000000000000..1fea84c68c3d --- /dev/null +++ b/frontend/src/queries/nodes/DataVisualization/Components/Charts/SqlBoxPlot.test.tsx @@ -0,0 +1,147 @@ +import '@testing-library/jest-dom' + +import { cleanup, render, screen, waitFor } from '@testing-library/react' + +import type { BoxPlotConfig, BoxPlotSeries } from '@posthog/quill-charts' + +import { BoxPlotSettings, ChartSettings } from '~/queries/schema/schema-general' +import { initKeaTests } from '~/test/init' + +import { Column } from '../../dataVisualizationLogic' +import { SqlBoxPlot } from './SqlBoxPlot' + +let latestBoxPlotProps: { labels: string[]; series: BoxPlotSeries[]; config?: BoxPlotConfig } | null = null + +jest.mock('posthog-js', () => ({ + __esModule: true, + default: { capture: jest.fn(), get_session_id: jest.fn(() => 'session-1') }, +})) + +jest.mock('@posthog/quill-charts', () => ({ + ...jest.requireActual('@posthog/quill-charts'), + BoxPlot: (props: { labels: string[]; series: BoxPlotSeries[]; config?: BoxPlotConfig }): JSX.Element => { + latestBoxPlotProps = props + return
+ }, +})) + +const posthog = jest.requireMock('posthog-js').default as { capture: jest.Mock } + +const columns: Column[] = [ + { name: 'bucket', label: 'bucket', dataIndex: 0, type: { name: 'STRING', isNumerical: false } }, + { name: 'series', label: 'series', dataIndex: 1, type: { name: 'STRING', isNumerical: false } }, + { name: 'min', label: 'min', dataIndex: 2, type: { name: 'FLOAT', isNumerical: true } }, + { name: 'p25', label: 'p25', dataIndex: 3, type: { name: 'FLOAT', isNumerical: true } }, + { name: 'median', label: 'median', dataIndex: 4, type: { name: 'FLOAT', isNumerical: true } }, + { name: 'mean', label: 'mean', dataIndex: 5, type: { name: 'FLOAT', isNumerical: true } }, + { name: 'p75', label: 'p75', dataIndex: 6, type: { name: 'FLOAT', isNumerical: true } }, + { name: 'max', label: 'max', dataIndex: 7, type: { name: 'FLOAT', isNumerical: true } }, +] + +const boxPlotSettings: BoxPlotSettings = { + xAxisColumn: 'bucket', + seriesColumn: 'series', + minColumn: 'min', + p25Column: 'p25', + medianColumn: 'median', + meanColumn: 'mean', + p75Column: 'p75', + maxColumn: 'max', + excludeOutliers: false, +} + +const chartSettings: ChartSettings = { boxPlot: boxPlotSettings } + +describe('SqlBoxPlot', () => { + beforeEach(() => { + initKeaTests() + latestBoxPlotProps = null + posthog.capture.mockClear() + }) + + afterEach(() => { + cleanup() + }) + + it('renders grouped boxes with the standard axis defaults', async () => { + render( + + ) + + await screen.findByTestId('mock-sql-box-plot') + expect(posthog.capture).not.toHaveBeenCalled() + expect(latestBoxPlotProps).toMatchObject({ + labels: ['Mon'], + series: [ + { key: 'Free', label: 'Free' }, + { key: 'Paid', label: 'Paid' }, + ], + config: { + showGrid: true, + showAxisLines: { x: true, y: true }, + }, + }) + }) + + it('captures unrendered items once per chart and session while keeping valid boxes', async () => { + const { rerender } = render( + + ) + + await screen.findByTestId('mock-sql-box-plot') + expect(latestBoxPlotProps).toMatchObject({ + labels: ['Tue'], + series: [{ key: 'Free', label: 'Free' }], + }) + await waitFor(() => + expect(posthog.capture).toHaveBeenCalledWith('sql box plot items not rendered', { + unrendered_item_count: 1, + total_item_count: 2, + reasons: { missingStatistic: 1, invalidOrder: 0, meanOutsideRange: 0 }, + }) + ) + + rerender( + + ) + await waitFor(() => expect(posthog.capture).toHaveBeenCalledTimes(1)) + }) + + it('explains how to fix missing column mappings', () => { + render( + + ) + + expect(screen.getByText('Select a column for Median.')).toBeInTheDocument() + expect(latestBoxPlotProps).toBeNull() + }) +}) diff --git a/frontend/src/queries/nodes/DataVisualization/Components/Charts/SqlBoxPlot.tsx b/frontend/src/queries/nodes/DataVisualization/Components/Charts/SqlBoxPlot.tsx new file mode 100644 index 000000000000..0614ee94fa95 --- /dev/null +++ b/frontend/src/queries/nodes/DataVisualization/Components/Charts/SqlBoxPlot.tsx @@ -0,0 +1,115 @@ +import clsx from 'clsx' +import posthog from 'posthog-js' +import { useEffect, useMemo } from 'react' + +import { BoxPlot } from '@posthog/quill-charts' +import type { BoxPlotConfig } from '@posthog/quill-charts' + +import { useChartConfig, useChartTheme } from 'lib/charts/hooks' + +import { ChartSettings } from '~/queries/schema/schema-general' + +import { makeChartErrorHandler } from 'products/product_analytics/frontend/insights/trends/shared/chartErrorHandler' + +import { Column } from '../../dataVisualizationLogic' +import { buildSqlBoxPlotModel } from './sqlBoxPlotAdapter' + +const handleChartError = makeChartErrorHandler('sql-box-plot') +const capturedUnrenderedItems = new Set() + +export interface SqlBoxPlotProps { + rows: unknown[][] + columns: Column[] + chartSettings: ChartSettings + analyticsKey: string + presetChartHeight?: boolean + className?: string +} + +export const SqlBoxPlot = ({ + rows, + columns, + chartSettings, + analyticsKey, + presetChartHeight, + className, +}: SqlBoxPlotProps): JSX.Element => { + const theme = useChartTheme() + const model = useMemo( + () => buildSqlBoxPlotModel(rows, columns, chartSettings.boxPlot ?? {}), + [rows, columns, chartSettings.boxPlot] + ) + const skippedRowCount = Object.values(model.skippedRows).reduce((total, count) => total + count, 0) + useEffect(() => { + const chartSessionKey = `${analyticsKey}:${posthog.get_session_id?.() ?? 'unknown'}` + if (skippedRowCount > 0 && !capturedUnrenderedItems.has(chartSessionKey)) { + capturedUnrenderedItems.add(chartSessionKey) + // pinned: analytics event name - renaming breaks dashboards + posthog.capture('sql box plot items not rendered', { + unrendered_item_count: skippedRowCount, + total_item_count: rows.length, + reasons: model.skippedRows, + }) + } + }, [analyticsKey, model.skippedRows, rows.length, skippedRowCount]) + + const yAxisSettings = chartSettings.leftYAxisSettings + const config = useChartConfig( + () => ({ + yScaleType: yAxisSettings?.scale === 'logarithmic' ? 'log' : 'linear', + xAxisLabel: chartSettings.xAxisLabel, + yAxisLabel: yAxisSettings?.label, + hideXAxis: chartSettings.showXAxisTicks === false, + hideYAxis: yAxisSettings?.showTicks === false, + showGrid: yAxisSettings?.showGridLines ?? true, + showAxisLines: { + x: chartSettings.showXAxisBorder ?? true, + y: chartSettings.showYAxisBorder ?? true, + }, + tooltip: { pinnable: true, placement: 'cursor' }, + legend: { show: chartSettings.showLegend ?? false, position: 'top' }, + }), + [chartSettings, yAxisSettings] + ) + + const containerClassName = clsx( + className, + 'rounded bg-surface-primary flex flex-1 items-center justify-center p-3', + { 'h-[60vh]': presetChartHeight, 'h-full': !presetChartHeight } + ) + + if (model.error) { + return ( +
+ {model.error} +
+ ) + } + + if (model.series.length === 0) { + return ( +
+ No boxes to plot. Check that your query returns rows. +
+ ) + } + + return ( +
+ +
+ ) +} diff --git a/frontend/src/queries/nodes/DataVisualization/Components/Charts/sqlBoxPlotAdapter.test.ts b/frontend/src/queries/nodes/DataVisualization/Components/Charts/sqlBoxPlotAdapter.test.ts new file mode 100644 index 000000000000..24fca251eadc --- /dev/null +++ b/frontend/src/queries/nodes/DataVisualization/Components/Charts/sqlBoxPlotAdapter.test.ts @@ -0,0 +1,251 @@ +import { BoxPlotSettings } from '~/queries/schema/schema-general' + +import { Column } from '../../dataVisualizationLogic' +import { buildSqlBoxPlotModel, getAutoBoxPlotSettings } from './sqlBoxPlotAdapter' + +const columns: Column[] = [ + { name: 'bucket', label: 'bucket', dataIndex: 0, type: { name: 'STRING', isNumerical: false } }, + { name: 'series', label: 'series', dataIndex: 1, type: { name: 'STRING', isNumerical: false } }, + { name: 'min', label: 'min', dataIndex: 2, type: { name: 'FLOAT', isNumerical: true } }, + { name: 'p25', label: 'p25', dataIndex: 3, type: { name: 'FLOAT', isNumerical: true } }, + { name: 'median', label: 'median', dataIndex: 4, type: { name: 'FLOAT', isNumerical: true } }, + { name: 'mean', label: 'mean', dataIndex: 5, type: { name: 'FLOAT', isNumerical: true } }, + { name: 'p75', label: 'p75', dataIndex: 6, type: { name: 'FLOAT', isNumerical: true } }, + { name: 'max', label: 'max', dataIndex: 7, type: { name: 'FLOAT', isNumerical: true } }, +] + +const settings: BoxPlotSettings = { + xAxisColumn: 'bucket', + seriesColumn: 'series', + minColumn: 'min', + p25Column: 'p25', + medianColumn: 'median', + meanColumn: 'mean', + p75Column: 'p75', + maxColumn: 'max', +} + +describe('sqlBoxPlotAdapter', () => { + test('builds grouped series and leaves missing combinations empty', () => { + const model = buildSqlBoxPlotModel( + [ + ['Mon', 'Free', 1, 2, 3, 4, 5, 6], + ['Tue', 'Paid', 10, 20, 30, 40, 50, 60], + ['Mon', 'Paid', 7, 8, 9, 10, 11, 12], + ], + columns, + settings + ) + + expect(model.error).toBeNull() + expect(model.labels).toEqual(['Mon', 'Tue']) + expect(model.series).toEqual([ + { + key: 'Free', + label: 'Free', + data: [{ min: 1, p25: 2, median: 3, mean: 4, p75: 5, max: 6 }, null], + }, + { + key: 'Paid', + label: 'Paid', + data: [ + { min: 7, p25: 8, median: 9, mean: 10, p75: 11, max: 12 }, + { min: 10, p25: 20, median: 30, mean: 40, p75: 50, max: 60 }, + ], + }, + ]) + }) + + test.each([ + { + name: 'missing mappings', + rows: [['Mon', 'Free', 1, 2, 3, 4, 5, 6]], + boxPlotSettings: { ...settings, medianColumn: undefined }, + error: 'Select a column for Median.', + }, + { + name: 'duplicate bucket and series pairs', + rows: [ + ['Mon', 'Free', 1, 2, 3, 4, 5, 6], + ['Mon', 'Free', 1, 2, 3, 4, 5, 6], + ], + boxPlotSettings: settings, + error: 'Rows 1 and 2 use the same X-axis and series values. Return one row for each box.', + }, + { + name: 'several rows without an x-axis or series', + rows: [ + ['Mon', 'Free', 1, 2, 3, 4, 5, 6], + ['Tue', 'Free', 1, 2, 3, 4, 5, 6], + ], + boxPlotSettings: { ...settings, xAxisColumn: undefined, seriesColumn: undefined }, + error: 'Select an X-axis column when the query returns more than one row.', + }, + { + name: 'different x-axis values with the same display label', + rows: [ + [null, 'Free', 1, 2, 3, 4, 5, 6], + ['[No value]', 'Free', 1, 2, 3, 4, 5, 6], + ], + boxPlotSettings: settings, + error: 'Row 2 has an X-axis value that displays as "[No value]", but another value uses the same label. Cast them to distinct strings in SQL.', + }, + { + name: 'different series values with the same display label', + rows: [ + ['Mon', null, 1, 2, 3, 4, 5, 6], + ['Mon', '[No value]', 1, 2, 3, 4, 5, 6], + ], + boxPlotSettings: settings, + error: 'Row 2 has a series value that displays as "[No value]", but another value uses the same label. Cast them to distinct strings in SQL.', + }, + ])('reports $name', ({ rows, boxPlotSettings, error }) => { + expect(buildSqlBoxPlotModel(rows, columns, boxPlotSettings).error).toBe(error) + }) + + test.each([ + { + name: 'missing statistic', + invalidRow: ['Mon', 'Free', 1, null, 3, 4, 5, 6], + skippedRows: { missingStatistic: 1, invalidOrder: 0, meanOutsideRange: 0 }, + }, + { + name: 'invalid statistic order', + invalidRow: ['Mon', 'Free', 1, 4, 3, 3, 5, 6], + skippedRows: { missingStatistic: 0, invalidOrder: 1, meanOutsideRange: 0 }, + }, + { + name: 'mean outside range', + invalidRow: ['Mon', 'Free', 1, 2, 3, 9, 5, 6], + skippedRows: { missingStatistic: 0, invalidOrder: 0, meanOutsideRange: 1 }, + }, + ])('skips a box with $name without hiding valid boxes', ({ invalidRow, skippedRows }) => { + const model = buildSqlBoxPlotModel([invalidRow, ['Tue', 'Free', 1, 2, 3, 4, 5, 6]], columns, settings) + + expect(model.error).toBeNull() + expect(model.labels).toEqual(['Tue']) + expect(model.series).toEqual([ + { + key: 'Free', + label: 'Free', + data: [{ min: 1, p25: 2, median: 3, mean: 4, p75: 5, max: 6 }], + }, + ]) + expect(model.skippedRows).toEqual(skippedRows) + }) + + test('rejects result shapes that would create a large sparse matrix', () => { + const rows = Array.from({ length: 101 }, (_, label) => + Array.from({ length: 100 }, (_, series) => [label, series, 1, 2, 3, 4, 5, 6]) + ).flat() + + expect(buildSqlBoxPlotModel(rows, columns, settings).error).toBe( + 'The box plot has too many X-axis and series combinations. Reduce the query result.' + ) + }) + + test('rejects more series than the series limit even when the cell matrix stays small', () => { + const rows = Array.from({ length: 201 }, (_, series) => ['Mon', series, 1, 2, 3, 4, 5, 6]) + + expect(buildSqlBoxPlotModel(rows, columns, settings).error).toBe( + 'The box plot has too many series. Reduce the number of distinct series values in the query result.' + ) + }) + + test('uses one distribution when the query returns one row without grouping columns', () => { + const model = buildSqlBoxPlotModel( + [[1, 2, 3, 4, 5, 6]], + columns.slice(2).map((column, dataIndex) => ({ ...column, dataIndex })), + { + minColumn: 'min', + p25Column: 'p25', + medianColumn: 'median', + meanColumn: 'mean', + p75Column: 'p75', + maxColumn: 'max', + } + ) + + expect(model.labels).toEqual(['Distribution']) + expect(model.series).toEqual([ + { + key: 'Distribution', + label: 'Distribution', + data: [{ min: 1, p25: 2, median: 3, mean: 4, p75: 5, max: 6 }], + }, + ]) + }) + + test('groups by series when the query returns one row per series without an x-axis', () => { + const model = buildSqlBoxPlotModel( + [ + ['Mon', 'Free', 1, 2, 3, 4, 5, 6], + ['Mon', 'Paid', 7, 8, 9, 10, 11, 12], + ], + columns, + { ...settings, xAxisColumn: undefined } + ) + + expect(model.error).toBeNull() + expect(model.labels).toEqual(['Distribution']) + expect(model.series).toEqual([ + { + key: 'Free', + label: 'Free', + data: [{ min: 1, p25: 2, median: 3, mean: 4, p75: 5, max: 6 }], + }, + { + key: 'Paid', + label: 'Paid', + data: [{ min: 7, p25: 8, median: 9, mean: 10, p75: 11, max: 12 }], + }, + ]) + }) + + test.each([ + { excludeOutliers: true, expectedMin: -4, expectedMax: 12 }, + { excludeOutliers: false, expectedMin: -100, expectedMax: 100 }, + ])( + 'sets whiskers to $expectedMin and $expectedMax when excludeOutliers is $excludeOutliers', + ({ excludeOutliers, expectedMin, expectedMax }) => { + const model = buildSqlBoxPlotModel([['Mon', 'Free', -100, 2, 3, 4, 6, 100]], columns, { + ...settings, + excludeOutliers, + }) + + expect(model.series[0].data[0]).toEqual({ + min: expectedMin, + p25: 2, + median: 3, + mean: 4, + p75: 6, + max: expectedMax, + }) + } + ) + + test.each([ + { + name: 'valid choices', + current: { xAxisColumn: 'bucket', meanColumn: 'median' }, + expectedGrouping: { xAxisColumn: 'bucket', seriesColumn: 'series' }, + expectedMean: 'median', + }, + { + name: 'explicit ungrouped choices', + current: { xAxisColumn: null, seriesColumn: null }, + expectedGrouping: { xAxisColumn: null, seriesColumn: null }, + expectedMean: 'mean', + }, + ])('auto-maps conventional aliases and preserves $name', ({ current, expectedGrouping, expectedMean }) => { + expect(getAutoBoxPlotSettings(columns, current)).toEqual({ + ...expectedGrouping, + minColumn: 'min', + p25Column: 'p25', + medianColumn: 'median', + meanColumn: expectedMean, + p75Column: 'p75', + maxColumn: 'max', + }) + }) +}) diff --git a/frontend/src/queries/nodes/DataVisualization/Components/Charts/sqlBoxPlotAdapter.ts b/frontend/src/queries/nodes/DataVisualization/Components/Charts/sqlBoxPlotAdapter.ts new file mode 100644 index 000000000000..6cf1d276f34f --- /dev/null +++ b/frontend/src/queries/nodes/DataVisualization/Components/Charts/sqlBoxPlotAdapter.ts @@ -0,0 +1,231 @@ +import type { BoxPlotDatum, BoxPlotSeries } from '@posthog/quill-charts' + +import { BoxPlotSettings } from '~/queries/schema/schema-general' + +interface BoxPlotColumn { + name: string + dataIndex: number + type: { isNumerical: boolean } +} + +export type SqlBoxPlotSkippedRowReason = 'missingStatistic' | 'invalidOrder' | 'meanOutsideRange' + +export interface SqlBoxPlotModel { + labels: string[] + series: BoxPlotSeries[] + error: string | null + skippedRows: Record +} + +export type BoxPlotStatisticColumn = keyof Pick< + BoxPlotSettings, + 'minColumn' | 'p25Column' | 'medianColumn' | 'meanColumn' | 'p75Column' | 'maxColumn' +> + +type BoxPlotValue = keyof Pick + +export const BOX_PLOT_STATISTICS: { + setting: BoxPlotStatisticColumn + value: BoxPlotValue + label: string + aliases: string[] +}[] = [ + { setting: 'minColumn', value: 'min', label: 'Minimum', aliases: ['min', 'minimum'] }, + { setting: 'p25Column', value: 'p25', label: '25th percentile', aliases: ['p25', 'q1'] }, + { setting: 'medianColumn', value: 'median', label: 'Median', aliases: ['median', 'p50'] }, + { setting: 'meanColumn', value: 'mean', label: 'Mean', aliases: ['mean', 'avg', 'average'] }, + { setting: 'p75Column', value: 'p75', label: '75th percentile', aliases: ['p75', 'q3'] }, + { setting: 'maxColumn', value: 'max', label: 'Maximum', aliases: ['max', 'maximum'] }, +] + +const MAX_BOX_PLOT_CELLS = 10_000 +// The cell cap alone permits thousands of series under a single X-axis value, so limit series +// independently because each one adds a legend row and per-hover work. Matches MAX_SERIES in +// sqlLineGraphAdapter. +const MAX_BOX_PLOT_SERIES = 200 + +const emptySkippedRows = (): Record => ({ + missingStatistic: 0, + invalidOrder: 0, + meanOutsideRange: 0, +}) + +const emptyModel = (error: string | null = null): SqlBoxPlotModel => ({ + labels: [], + series: [], + error, + skippedRows: emptySkippedRows(), +}) + +const findColumn = ( + columns: BoxPlotColumn[], + name: string | null | undefined, + numerical = false +): BoxPlotColumn | undefined => { + if (!name) { + return undefined + } + return columns.find((column) => column.name === name && (!numerical || column.type.isNumerical)) +} + +const findAliasedColumn = (columns: BoxPlotColumn[], aliases: string[], numerical = false): BoxPlotColumn | undefined => + columns.find((column) => aliases.includes(column.name.toLowerCase()) && (!numerical || column.type.isNumerical)) + +export const getAutoBoxPlotSettings = (columns: BoxPlotColumn[], current: BoxPlotSettings = {}): BoxPlotSettings => { + const next = { ...current } + + if (current.xAxisColumn !== null && !findColumn(columns, current.xAxisColumn)) { + next.xAxisColumn = findAliasedColumn(columns, ['label', 'bucket', 'date', 'day'])?.name + } + if (current.seriesColumn !== null && !findColumn(columns, current.seriesColumn)) { + next.seriesColumn = findAliasedColumn(columns, ['series', 'breakdown'])?.name + } + + for (const statistic of BOX_PLOT_STATISTICS) { + if (!findColumn(columns, current[statistic.setting], true)) { + next[statistic.setting] = findAliasedColumn(columns, statistic.aliases, true)?.name + } + } + + return next +} + +const groupingIdentity = (value: unknown): string => JSON.stringify([typeof value, value ?? null]) + +const finiteNumber = (value: unknown): number | null => { + if (value === null || value === undefined || value === '') { + return null + } + const number = typeof value === 'number' ? value : Number(value) + return Number.isFinite(number) ? number : null +} + +export const buildSqlBoxPlotModel = ( + rows: unknown[][], + columns: BoxPlotColumn[], + settings: BoxPlotSettings +): SqlBoxPlotModel => { + const statisticColumns = BOX_PLOT_STATISTICS.map((statistic) => ({ + ...statistic, + column: findColumn(columns, settings[statistic.setting], true), + })) + const missingStatistic = statisticColumns.find(({ column }) => !column) + if (missingStatistic) { + return emptyModel(`Select a column for ${missingStatistic.label}.`) + } + + if (rows.length === 0) { + return emptyModel() + } + + const xAxisColumn = findColumn(columns, settings.xAxisColumn) + const seriesColumn = findColumn(columns, settings.seriesColumn) + if (!xAxisColumn && !seriesColumn && rows.length > 1) { + return emptyModel('Select an X-axis column when the query returns more than one row.') + } + + const labels: string[] = [] + const labelSet = new Set() + const seriesLabels: string[] = [] + const seriesLabelSet = new Set() + const dataBySeries = new Map>() + const skippedRows = emptySkippedRows() + const xIdentityByLabel = new Map() + const seriesIdentityByLabel = new Map() + const rowByPair = new Map() + + for (const [rowIndex, row] of rows.entries()) { + const nullableValues = Object.fromEntries( + statisticColumns.map((statistic) => [statistic.value, finiteNumber(row[statistic.column!.dataIndex])]) + ) as Record + if (Object.values(nullableValues).some((value) => value === null)) { + skippedRows.missingStatistic++ + continue + } + + const values = nullableValues as Record + if ( + !( + values.min <= values.p25 && + values.p25 <= values.median && + values.median <= values.p75 && + values.p75 <= values.max + ) + ) { + skippedRows.invalidOrder++ + continue + } + if (values.mean < values.min || values.mean > values.max) { + skippedRows.meanOutsideRange++ + continue + } + + const xValue = xAxisColumn ? row[xAxisColumn.dataIndex] : 'Distribution' + const seriesValue = seriesColumn ? row[seriesColumn.dataIndex] : 'Distribution' + const label = String(xValue ?? '[No value]') + const seriesLabel = String(seriesValue ?? '[No value]') + const xIdentity = groupingIdentity(xValue) + const seriesIdentity = groupingIdentity(seriesValue) + + if (xIdentityByLabel.has(label) && xIdentityByLabel.get(label) !== xIdentity) { + return emptyModel( + `Row ${rowIndex + 1} has an X-axis value that displays as "${label}", but another value uses the same label. Cast them to distinct strings in SQL.` + ) + } + if (seriesIdentityByLabel.has(seriesLabel) && seriesIdentityByLabel.get(seriesLabel) !== seriesIdentity) { + return emptyModel( + `Row ${rowIndex + 1} has a series value that displays as "${seriesLabel}", but another value uses the same label. Cast them to distinct strings in SQL.` + ) + } + xIdentityByLabel.set(label, xIdentity) + seriesIdentityByLabel.set(seriesLabel, seriesIdentity) + + const pairKey = JSON.stringify([xIdentity, seriesIdentity]) + const previousRow = rowByPair.get(pairKey) + if (previousRow !== undefined) { + return emptyModel( + `Rows ${previousRow + 1} and ${rowIndex + 1} use the same X-axis and series values. Return one row for each box.` + ) + } + rowByPair.set(pairKey, rowIndex) + + if (!labelSet.has(label)) { + labels.push(label) + labelSet.add(label) + } + if (!seriesLabelSet.has(seriesLabel)) { + seriesLabels.push(seriesLabel) + seriesLabelSet.add(seriesLabel) + if (seriesLabelSet.size > MAX_BOX_PLOT_SERIES) { + return emptyModel( + 'The box plot has too many series. Reduce the number of distinct series values in the query result.' + ) + } + } + if (labelSet.size * seriesLabelSet.size > MAX_BOX_PLOT_CELLS) { + return emptyModel('The box plot has too many X-axis and series combinations. Reduce the query result.') + } + + const iqr = values.p75 - values.p25 + const excludeOutliers = settings.excludeOutliers !== false + const datum = { + ...values, + min: excludeOutliers ? Math.max(values.min, values.p25 - 1.5 * iqr) : values.min, + max: excludeOutliers ? Math.min(values.max, values.p75 + 1.5 * iqr) : values.max, + } + const seriesData = dataBySeries.get(seriesLabel) ?? new Map() + seriesData.set(label, datum) + dataBySeries.set(seriesLabel, seriesData) + } + + return { + labels, + series: seriesLabels.map((seriesLabel) => ({ + key: seriesLabel, + label: seriesLabel, + data: labels.map((label) => dataBySeries.get(seriesLabel)?.get(label) ?? null), + })), + error: null, + skippedRows, + } +} diff --git a/frontend/src/queries/nodes/DataVisualization/Components/DisplayTab.test.tsx b/frontend/src/queries/nodes/DataVisualization/Components/DisplayTab.test.tsx index 3d92bb18adac..28e80824d02d 100644 --- a/frontend/src/queries/nodes/DataVisualization/Components/DisplayTab.test.tsx +++ b/frontend/src/queries/nodes/DataVisualization/Components/DisplayTab.test.tsx @@ -130,6 +130,54 @@ describe('DisplayTab', () => { ) }) }) + it('offers box plot settings and hides unsupported controls', async () => { + initKeaTests() + + const key = 'display-tab-box-plot-test' + let query: DataVisualizationNode = { + kind: NodeKind.DataVisualizationNode, + source: { + kind: NodeKind.HogQLQuery, + query: 'select * from summaries', + }, + display: ChartDisplayType.BoxPlot, + chartSettings: { boxPlot: { excludeOutliers: true } }, + } + + const props: DataVisualizationLogicProps = { + key, + query, + dataNodeCollectionId: key, + setQuery: (setter) => { + query = setter(query) + }, + } + + dataVisualizationLogic(props).mount() + displayLogic({ key }).mount() + + render( + + + + + + ) + + const user = userEvent.setup() + + expect(await screen.findByText('Y-axis')).toBeInTheDocument() + expect(screen.queryByText('Right Y-axis')).not.toBeInTheDocument() + expect(screen.queryByText('Goals')).not.toBeInTheDocument() + expect(screen.queryByText('Show total row')).not.toBeInTheDocument() + + await user.click(screen.getByText('Exclude outliers')) + await user.click(screen.getByText('Y-axis')) + expect(screen.queryByText('Begin at zero')).not.toBeInTheDocument() + + await waitFor(() => expect(query.chartSettings?.boxPlot?.excludeOutliers).toBe(false)) + }) + it('offers scatter axis settings and drops the panels a scatter has no support for', async () => { initKeaTests() diff --git a/frontend/src/queries/nodes/DataVisualization/Components/DisplayTab.tsx b/frontend/src/queries/nodes/DataVisualization/Components/DisplayTab.tsx index fee2e17fd653..ba71ca7c5549 100644 --- a/frontend/src/queries/nodes/DataVisualization/Components/DisplayTab.tsx +++ b/frontend/src/queries/nodes/DataVisualization/Components/DisplayTab.tsx @@ -43,12 +43,13 @@ export const DisplayTab = (): JSX.Element => { const isStackedBarChart = effectiveVisualizationType === ChartDisplayType.ActionsStackedBar const isPieChart = effectiveVisualizationType === ChartDisplayType.ActionsPie const isScatterPlot = effectiveVisualizationType === ChartDisplayType.ScatterPlot + const isBoxPlot = effectiveVisualizationType === ChartDisplayType.BoxPlot const isLineChart = effectiveVisualizationType === ChartDisplayType.ActionsLineGraph || effectiveVisualizationType === ChartDisplayType.ActionsAreaGraph const renderYAxisSettings = (name: 'leftYAxisSettings' | 'rightYAxisSettings'): JSX.Element => { - const leftPlaceholder = isScatterPlot ? 'Y-axis label' : 'Left Y-axis label' + const leftPlaceholder = isScatterPlot || isBoxPlot ? 'Y-axis label' : 'Left Y-axis label' const labelPlaceholder = name === 'leftYAxisSettings' ? leftPlaceholder : 'Right Y-axis label' return ( @@ -87,17 +88,19 @@ export const DisplayTab = (): JSX.Element => { }} /> - { - updateChartSettings({ [name]: { startAtZero: value } }) - }} - /> + {!isBoxPlot && ( + { + updateChartSettings({ [name]: { startAtZero: value } }) + }} + /> + )} { updateChartSettings({ showLegend: value }) }} /> + {isBoxPlot && ( + { + updateChartSettings({ boxPlot: { excludeOutliers: value } }) + }} + /> + )} {isPieChart ? ( <>
@@ -181,7 +194,7 @@ export const DisplayTab = (): JSX.Element => { }} /> )} - {!isScatterPlot && ( + {!isScatterPlot && !isBoxPlot && ( <> { !isPieChart ? { key: 'left-y-axis', - header: isScatterPlot ? 'Y-axis' : 'Left Y-axis', + header: isScatterPlot || isBoxPlot ? 'Y-axis' : 'Left Y-axis', className: 'p-2 flex flex-col gap-2', content: renderYAxisSettings('leftYAxisSettings'), } : null, // A scatter has one gutter per axis, so there is no second Y axis to configure. - !isPieChart && !isScatterPlot + !isPieChart && !isScatterPlot && !isBoxPlot ? { key: 'right-y-axis', header: 'Right Y-axis', @@ -337,7 +350,7 @@ export const DisplayTab = (): JSX.Element => { ), } : null, - !isPieChart && !isScatterPlot + !isPieChart && !isScatterPlot && !isBoxPlot ? { key: 'goals', header: ( diff --git a/frontend/src/queries/nodes/DataVisualization/Components/SeriesTab.tsx b/frontend/src/queries/nodes/DataVisualization/Components/SeriesTab.tsx index 601eb6ad4e3d..65fd519bbe78 100644 --- a/frontend/src/queries/nodes/DataVisualization/Components/SeriesTab.tsx +++ b/frontend/src/queries/nodes/DataVisualization/Components/SeriesTab.tsx @@ -27,6 +27,7 @@ import { ResultCustomizationBy } from '~/queries/schema/schema-general' import { ChartDisplayType } from '~/types' import { AxisSeries, Column, dataVisualizationLogic } from '../dataVisualizationLogic' +import { BoxPlotSeriesTab } from './BoxPlotSeriesTab' import { HeatmapSeriesTab } from './Heatmap/HeatmapSeriesTab' import { AxisBreakdownSeries, BREAKDOWN_LIMIT_LABEL, seriesBreakdownLogic } from './seriesBreakdownLogic' import { getAvailableSeriesBreakdownColumns } from './seriesBreakdownUtils' @@ -70,6 +71,10 @@ export const SeriesTab = (): JSX.Element => { return } + if (effectiveVisualizationType === ChartDisplayType.BoxPlot) { + return + } + if (showTableSettings) { return (
diff --git a/frontend/src/queries/nodes/DataVisualization/Components/TableDisplay.test.tsx b/frontend/src/queries/nodes/DataVisualization/Components/TableDisplay.test.tsx new file mode 100644 index 000000000000..c4599c78ec3b --- /dev/null +++ b/frontend/src/queries/nodes/DataVisualization/Components/TableDisplay.test.tsx @@ -0,0 +1,80 @@ +import '@testing-library/jest-dom' + +import { cleanup, render, screen, waitFor } from '@testing-library/react' +import userEvent from '@testing-library/user-event' +import { BindLogic } from 'kea' + +import { FEATURE_FLAGS } from 'lib/constants' +import { featureFlagLogic } from 'lib/logic/featureFlagLogic' + +import { DataVisualizationNode, HogQLQueryResponse, NodeKind } from '~/queries/schema/schema-general' +import { initKeaTests } from '~/test/init' +import { ChartDisplayType } from '~/types' + +import { dataNodeLogic } from '../../DataNode/dataNodeLogic' +import { DataVisualizationLogicProps, dataVisualizationLogic } from '../dataVisualizationLogic' +import { TableDisplay } from './TableDisplay' + +const cachedResults: HogQLQueryResponse = { + results: [['Mon', 1, 2, 3, 4, 5, 6]], + columns: ['bucket', 'min', 'p25', 'median', 'mean', 'p75', 'max'], + types: [ + ['bucket', 'String'], + ['min', 'Float64'], + ['p25', 'Float64'], + ['median', 'Float64'], + ['mean', 'Float64'], + ['p75', 'Float64'], + ['max', 'Float64'], + ], +} + +describe('TableDisplay', () => { + afterEach(() => { + cleanup() + featureFlagLogic.unmount() + }) + + it('offers box plots and saves the selected display when the feature is enabled', async () => { + initKeaTests() + featureFlagLogic.mount() + featureFlagLogic.actions.setFeatureFlags([FEATURE_FLAGS.SQL_BOX_PLOT_INSIGHT], { + [FEATURE_FLAGS.SQL_BOX_PLOT_INSIGHT]: true, + }) + + let query: DataVisualizationNode = { + kind: NodeKind.DataVisualizationNode, + source: { kind: NodeKind.HogQLQuery, query: 'select * from summaries' }, + display: ChartDisplayType.ActionsTable, + } + const props: DataVisualizationLogicProps = { + key: 'table-display-box-plot', + query, + cachedResults, + dataNodeCollectionId: 'table-display-box-plot', + setQuery: (setter) => { + query = setter(query) + }, + } + + dataNodeLogic({ + key: props.key, + query: query.source, + cachedResults, + dataNodeCollectionId: props.dataNodeCollectionId, + }).mount() + dataVisualizationLogic(props).mount() + + render( + + + + ) + + const user = userEvent.setup() + await user.click(screen.getByTestId('chart-filter')) + await user.click(await screen.findByText('Box plot')) + + await waitFor(() => expect(query.display).toBe(ChartDisplayType.BoxPlot)) + }) +}) diff --git a/frontend/src/queries/nodes/DataVisualization/Components/TableDisplay.tsx b/frontend/src/queries/nodes/DataVisualization/Components/TableDisplay.tsx index afa177922e80..dfbf5f151e74 100644 --- a/frontend/src/queries/nodes/DataVisualization/Components/TableDisplay.tsx +++ b/frontend/src/queries/nodes/DataVisualization/Components/TableDisplay.tsx @@ -3,7 +3,9 @@ import { useActions, useValues } from 'kea' import { IconGraph, IconLifecycle, IconPieChart, IconScatter, IconTrends } from '@posthog/icons' import { LemonSelect, LemonSelectOptions, LemonSelectProps } from '@posthog/lemon-ui' +import { FEATURE_FLAGS } from 'lib/constants' import { Icon123, IconAreaChart, IconHeatmap, IconTableChart } from 'lib/lemon-ui/icons' +import { featureFlagLogic } from 'lib/logic/featureFlagLogic' import { ChartDisplayType } from '~/types' @@ -14,6 +16,7 @@ interface TableDisplayProps extends Pick, 'di export const TableDisplay = ({ disabledReason }: TableDisplayProps): JSX.Element => { const { setVisualizationType } = useActions(dataVisualizationLogic) const { autoVisualizationType, columns, numericalColumns, visualizationType } = useValues(dataVisualizationLogic) + const { featureFlags } = useValues(featureFlagLogic) const canDisplayContinuousChart = columns.length > 1 && numericalColumns.length > 0 // Both scatter axes are numeric measures, so one numeric column can't fill both. @@ -119,6 +122,17 @@ export const TableDisplay = ({ disabledReason }: TableDisplayProps): JSX.Element ? 'Requires at least two numeric columns, one for each axis' : undefined, }, + ...(featureFlags[FEATURE_FLAGS.SQL_BOX_PLOT_INSIGHT] + ? [ + { + value: ChartDisplayType.BoxPlot, + icon: , + label: 'Box plot', + disabledReason: + numericalColumns.length < 6 ? 'Requires six numeric summary columns' : undefined, + }, + ] + : []), { value: ChartDisplayType.TwoDimensionalHeatmap, icon: , diff --git a/frontend/src/queries/nodes/DataVisualization/DataVisualization.tsx b/frontend/src/queries/nodes/DataVisualization/DataVisualization.tsx index 1b932ec36ee9..99e25bd1d5cf 100644 --- a/frontend/src/queries/nodes/DataVisualization/DataVisualization.tsx +++ b/frontend/src/queries/nodes/DataVisualization/DataVisualization.tsx @@ -35,6 +35,7 @@ import { ElapsedTime } from '../DataNode/ElapsedTime' import { Reload } from '../DataNode/Reload' import { QueryFeature } from '../DataTable/queryFeatures' import { PieChart } from './Components/Charts/PieChart' +import { SqlBoxPlot } from './Components/Charts/SqlBoxPlot' import { SqlChart } from './Components/Charts/SqlChart' import { SqlScatterGraph } from './Components/Charts/SqlScatterGraph' import { TwoDimensionalHeatmap } from './Components/Heatmap/TwoDimensionalHeatmap' @@ -181,6 +182,7 @@ function InternalDataTableVisualization(props: DataTableVisualizationProps): JSX isChartSettingsPanelOpen, xData, yData, + columns, chartSettings, dashboardId, dataVisualizationProps, @@ -299,6 +301,17 @@ function InternalDataTableVisualization(props: DataTableVisualizationProps): JSX presetChartHeight={presetChartHeight} /> ) + } else if (effectiveVisualizationType === ChartDisplayType.BoxPlot) { + const rows = ('results' in response ? response.results : 'result' in response ? response.result : []) ?? [] + component = ( + + ) } else if (effectiveVisualizationType === ChartDisplayType.TwoDimensionalHeatmap) { component = } else if (effectiveVisualizationType === ChartDisplayType.BoldNumber) { diff --git a/frontend/src/queries/nodes/DataVisualization/dataVisualizationLogic.test.ts b/frontend/src/queries/nodes/DataVisualization/dataVisualizationLogic.test.ts index 5941e85a2d65..aeb1a333c80d 100644 --- a/frontend/src/queries/nodes/DataVisualization/dataVisualizationLogic.test.ts +++ b/frontend/src/queries/nodes/DataVisualization/dataVisualizationLogic.test.ts @@ -136,6 +136,40 @@ describe('dataVisualizationLogic', () => { }) }) + it('auto-maps box plot columns when the chart is selected', async () => { + dataNodeLogic({ key: testKey, query: defaultQuery.source, dataNodeCollectionId }).actions.setResponse({ + columns: ['bucket', 'series', 'min', 'p25', 'median', 'mean', 'p75', 'max'], + types: [ + ['bucket', 'Date'], + ['series', 'String'], + ['min', 'Float64'], + ['p25', 'Float64'], + ['median', 'Float64'], + ['mean', 'Float64'], + ['p75', 'Float64'], + ['max', 'Float64'], + ], + results: [['2026-01-01', 'Free', 1, 2, 3, 4, 5, 6]], + }) + + logic.actions.setVisualizationType(ChartDisplayType.BoxPlot) + + await expectLogic(logic).toMatchValues({ + chartSettings: expect.objectContaining({ + boxPlot: { + xAxisColumn: 'bucket', + seriesColumn: 'series', + minColumn: 'min', + p25Column: 'p25', + medianColumn: 'median', + meanColumn: 'mean', + p75Column: 'p75', + maxColumn: 'max', + }, + }), + }) + }) + it('resets axes when y-axis columns are no longer numerical', async () => { const dataNode = dataNodeLogic({ key: testKey, query: defaultQuery.source, dataNodeCollectionId }) diff --git a/frontend/src/queries/nodes/DataVisualization/dataVisualizationLogic.ts b/frontend/src/queries/nodes/DataVisualization/dataVisualizationLogic.ts index 15008ebc1f94..ed05870a686f 100644 --- a/frontend/src/queries/nodes/DataVisualization/dataVisualizationLogic.ts +++ b/frontend/src/queries/nodes/DataVisualization/dataVisualizationLogic.ts @@ -59,6 +59,7 @@ import type { } from '../../schema/schema-general' import { dataNodeLogic } from '../DataNode/dataNodeLogic' import { QueryFeature, getQueryFeatures } from '../DataTable/queryFeatures' +import { getAutoBoxPlotSettings } from './Components/Charts/sqlBoxPlotAdapter' import { ColumnScalar, FORMATTING_TEMPLATES } from './types' export enum SideBarTab { @@ -376,6 +377,13 @@ const mergeChartSettings = (state: ChartSettings, settings: ChartSettings): Char ...settings.scatter, } : undefined, + boxPlot: + state.boxPlot || settings.boxPlot + ? { + ...state.boxPlot, + ...settings.boxPlot, + } + : undefined, leftYAxisSettings: state.leftYAxisSettings || settings.leftYAxisSettings ? { @@ -1854,6 +1862,12 @@ export const dataVisualizationLogic = kea([ applyScatterXAxis(actions, values.columns, values.selectedXAxis, values.selectedYAxis) } + if (visualizationType === ChartDisplayType.BoxPlot) { + actions.updateChartSettings({ + boxPlot: getAutoBoxPlotSettings(values.columns, values.chartSettings.boxPlot), + }) + } + const isAutoHeatmap = visualizationType === ChartDisplayType.Auto && getAutoVisualizationType(values.columns, values.response) === ChartDisplayType.TwoDimensionalHeatmap @@ -1971,6 +1985,12 @@ export const dataVisualizationLogic = kea([ applyAutoHeatmapSettings(actions, value, values.chartSettings.heatmap ?? {}) } + if (values.effectiveVisualizationType === ChartDisplayType.BoxPlot) { + actions.updateChartSettings({ + boxPlot: getAutoBoxPlotSettings(value, values.chartSettings.boxPlot), + }) + } + // The generic setup above only lands a numeric x for a scatter by luck (a DATE column or a // non-numeric first column). Resolve it explicitly so an all-numeric query, a column change, // or a persisted/assistant-created scatter still gets a plottable x axis. diff --git a/frontend/src/queries/schema.json b/frontend/src/queries/schema.json index 0fe22fe95d85..2d76bf37dd9c 100644 --- a/frontend/src/queries/schema.json +++ b/frontend/src/queries/schema.json @@ -6051,6 +6051,39 @@ "required": ["day", "label", "min", "p25", "median", "p75", "max", "mean"], "type": "object" }, + "BoxPlotSettings": { + "additionalProperties": false, + "properties": { + "excludeOutliers": { + "type": "boolean" + }, + "maxColumn": { + "type": "string" + }, + "meanColumn": { + "type": "string" + }, + "medianColumn": { + "type": "string" + }, + "minColumn": { + "type": "string" + }, + "p25Column": { + "type": "string" + }, + "p75Column": { + "type": "string" + }, + "seriesColumn": { + "type": ["string", "null"] + }, + "xAxisColumn": { + "type": ["string", "null"] + } + }, + "type": "object" + }, "Breakdown": { "additionalProperties": false, "properties": { @@ -15810,6 +15843,9 @@ "ChartSettings": { "additionalProperties": false, "properties": { + "boxPlot": { + "$ref": "#/definitions/BoxPlotSettings" + }, "chartStyle": { "$ref": "#/definitions/ChartStyle", "description": "Chart rendering style overrides (line shape). Only applies to line and area charts." diff --git a/frontend/src/queries/schema/schema-general.ts b/frontend/src/queries/schema/schema-general.ts index c00b98c07ed7..e91b74e0674d 100644 --- a/frontend/src/queries/schema/schema-general.ts +++ b/frontend/src/queries/schema/schema-general.ts @@ -1307,6 +1307,18 @@ export interface ScatterChartSettings { showBestFit?: boolean } +export interface BoxPlotSettings { + xAxisColumn?: string | null + seriesColumn?: string | null + minColumn?: string + p25Column?: string + medianColumn?: string + meanColumn?: string + p75Column?: string + maxColumn?: string + excludeOutliers?: boolean +} + export interface YAxisSettings { label?: string scale?: 'linear' | 'logarithmic' @@ -1341,6 +1353,7 @@ export interface ChartSettings { heatmap?: HeatmapSettings pie?: PieChartSettings scatter?: ScatterChartSettings + boxPlot?: BoxPlotSettings /** Per-breakdown-value color customizations. Keyed by the raw breakdown column value. */ resultCustomizations?: Record /** Chart rendering style overrides (line shape). Only applies to line and area charts. */ diff --git a/frontend/src/scenes/insights/stories/SQLBoxPlot.stories.tsx b/frontend/src/scenes/insights/stories/SQLBoxPlot.stories.tsx new file mode 100644 index 000000000000..87d7ccea29b7 --- /dev/null +++ b/frontend/src/scenes/insights/stories/SQLBoxPlot.stories.tsx @@ -0,0 +1,109 @@ +import { Decorator, Meta, StoryObj } from '@storybook/react' +import { waitFor } from '@testing-library/dom' +import userEvent from '@testing-library/user-event' +import { useEffect, useRef } from 'react' + +import { FEATURE_FLAGS } from 'lib/constants' +import { createInsightStory } from 'scenes/insights/__mocks__/createInsightScene' + +import { mswDecorator } from '~/mocks/browser' +import { AccessControlLevel, AccessControlResourceType } from '~/types' + +import __sqlBoxPlot from '../../../mocks/fixtures/api/projects/team_id/insights/sqlBoxPlot.json' + +const availableSources = { + Postgres: { name: 'Postgres', iconPath: '/static/services/postgres.png', fields: [], caption: '', featured: true }, + Stripe: { name: 'Stripe', iconPath: '/static/services/stripe.png', fields: [], caption: '', featured: true }, + GoogleAds: { + name: 'GoogleAds', + iconPath: '/static/services/google-ads.png', + fields: [], + caption: '', + featured: true, + }, +} + +type Story = StoryObj<{}> + +const grantWarehouseAccess: Decorator = function GrantWarehouseAccess(Story): JSX.Element { + const appContext = (window as any).POSTHOG_APP_CONTEXT + const originalAccess = useRef() + if (appContext && originalAccess.current === undefined) { + originalAccess.current = appContext.resource_access_control + appContext.resource_access_control = { + ...appContext.resource_access_control, + [AccessControlResourceType.WarehouseObjects]: AccessControlLevel.Editor, + } + } + useEffect( + () => () => { + if (appContext) { + appContext.resource_access_control = originalAccess.current + } + }, + [appContext] + ) + return +} + +const meta: Meta = { + title: 'Scenes-App/Insights/SQLBoxPlot', + parameters: { + layout: 'fullscreen', + featureFlags: [FEATURE_FLAGS.SQL_BOX_PLOT_INSIGHT], + testOptions: { + snapshotBrowsers: ['chromium'], + viewport: { width: 1300, height: 720 }, + waitForSelector: '[data-attr="sql-box-plot"]', + }, + viewMode: 'story', + mockDate: '2026-02-02', + }, + decorators: [ + grantWarehouseAccess, + mswDecorator({ + get: { + '/api/projects/:team_id/groups_types': [], + '/api/projects/:team_id/query_tab_state/user': () => [200, null], + '/api/projects/:team_id/external_data_sources/connections': [], + '/api/projects/:team_id/external_data_sources/direct_connection_options': [], + '/api/environments/:team_id/external_data_sources/wizard': availableSources, + }, + }), + ], +} + +export default meta + +export const GroupedSeries: Story = createInsightStory(__sqlBoxPlot as any) + +export const EditOptions: Story = { + render: createInsightStory(__sqlBoxPlot as any, 'edit'), + parameters: { + ...meta.parameters, + testOptions: { + ...meta.parameters?.testOptions, + waitForSelector: '[data-attr="box-plot-minColumn"]', + }, + }, + play: async ({ canvasElement }): Promise => { + await waitFor( + async () => { + if (canvasElement.querySelector('[data-attr="box-plot-minColumn"]')) { + return + } + const settingsButton = canvasElement.querySelector( + '[data-attr="sql-editor-visualization-settings-button"]' + ) + if (!settingsButton) { + throw new Error('Visualization settings button not ready') + } + await userEvent.click(settingsButton) + if (!canvasElement.querySelector('[data-attr="box-plot-minColumn"]')) { + throw new Error('Box plot settings not ready') + } + }, + { timeout: 10_000 } + ) + }, +} diff --git a/packages/quill/packages/charts/AGENTS.md b/packages/quill/packages/charts/AGENTS.md index cc377af56ab9..e5a469dc90ec 100644 --- a/packages/quill/packages/charts/AGENTS.md +++ b/packages/quill/packages/charts/AGENTS.md @@ -15,7 +15,7 @@ Quick-reference for AI agents using `@posthog/quill-charts`. Canvas-rendered cha | TimeSeriesBarChart | Same x-axis handling, bar rendering; supports per-series `yAxisId` axes | | TimeSeriesComboChart | Mixed bar + line/area on a time x-axis — `ComboChart` plus the time-series chrome (date x-axis, goal lines, legend, value labels) | | PieChart | Part-of-whole, one value per series; `innerRadiusRatio` for donut + `centerLabel`; on-slice labels via `showLabelOnSlice`/`showValueOnSlice`, positioned with `labelRadiusRatio` (0=center, 1=rim) and gated by `minSlicePercentForLabel`; `sliceValueDisplay` picks what the numeric line holds — `'value'`, `'percent'`, or `'both'` for `352 (18.4%)` (defaults to `'percent'` when `isPercent` is set, else `'value'`); `config.legend` for the built-in legend (a toggled-off slice is removed and the rest rescale to the full circle) | -| BoxPlot | Distribution summaries — `{ min, p25, median, mean, p75, max }` per label | +| BoxPlot | Distribution summaries — `{ min, p25, median, mean, p75, max }` per label; supports `config.legend` for grouped series | | Heatmap | 2D density grid (e.g. latency over time) — `xLabels` × `yLabels` (row 0 at the bottom), `cells[row][col]` counts → color intensity on one accent (log ramp by default, `colorScale: 'linear'` to opt out); single-cell tooltip resolved from the cursor, `onCellClick` reports `{ xIndex, yIndex, value }` | | Sparkline | Tiny inline trend, no axes — a gradient-filled line (default) or stacked bars via `type: 'bar'`; a flat `number[]` + `color`, or full `series` for multi-series; tooltip off by default, opt in with a `tooltip` render prop | | MetricCard | Headline number + sparkline + change pill (dashboard stat tiles) | diff --git a/packages/quill/packages/charts/src/charts/BoxPlot/BoxPlot.stories.tsx b/packages/quill/packages/charts/src/charts/BoxPlot/BoxPlot.stories.tsx index c4591ae005d2..ffdac25973aa 100644 --- a/packages/quill/packages/charts/src/charts/BoxPlot/BoxPlot.stories.tsx +++ b/packages/quill/packages/charts/src/charts/BoxPlot/BoxPlot.stories.tsx @@ -92,7 +92,12 @@ export const MultiSeriesGrouped: Story = { const theme = useReactiveTheme() return ( - + ) }, diff --git a/packages/quill/packages/charts/src/charts/BoxPlot/BoxPlot.test.tsx b/packages/quill/packages/charts/src/charts/BoxPlot/BoxPlot.test.tsx index 881d457b6efc..c7b8ba156ad1 100644 --- a/packages/quill/packages/charts/src/charts/BoxPlot/BoxPlot.test.tsx +++ b/packages/quill/packages/charts/src/charts/BoxPlot/BoxPlot.test.tsx @@ -37,6 +37,14 @@ describe('BoxPlot', () => { expect(chart.seriesCount).toBe(2) }) + it('renders a legend when requested', () => { + const { container } = renderHogChart( + + ) + const buttons = container.querySelectorAll('[data-attr="hog-chart-box-plot-legend"] button') + expect(Array.from(buttons, (button) => button.textContent)).toEqual(['A', 'B']) + }) + it('renders y-axis ticks for the value range that spans whiskers', () => { const series: BoxPlotSeries[] = [ { diff --git a/packages/quill/packages/charts/src/charts/BoxPlot/BoxPlot.tsx b/packages/quill/packages/charts/src/charts/BoxPlot/BoxPlot.tsx index 7a24009543d5..8549d50d15bf 100644 --- a/packages/quill/packages/charts/src/charts/BoxPlot/BoxPlot.tsx +++ b/packages/quill/packages/charts/src/charts/BoxPlot/BoxPlot.tsx @@ -1,14 +1,25 @@ import React, { useCallback, useMemo } from 'react' -import { drawBoxes, drawBoxHighlight, drawGrid, type DrawContext } from '../../core/canvas-renderer' +import { ChartLegend } from '../../components/Legend/ChartLegend' +import { useChartLegend } from '../../components/Legend/useChartLegend' +import { + drawAxes, + drawBoxes, + drawBoxHighlight, + drawGrid, + resolveAxisLineColor, + type DrawContext, +} from '../../core/canvas-renderer' import { Chart } from '../../core/Chart' import { ChartErrorBoundary } from '../../core/ChartErrorBoundary' import { dimColor } from '../../core/color-utils' import { createBarScales, type BarScaleSet, yTickCountForHeight } from '../../core/scales' +import { resolveAxisLines } from '../../core/types' import type { ChartConfig, ChartDimensions, ChartDrawArgs, + ChartLegendConfig, ChartScales, ChartTheme, CreateScalesFn, @@ -50,6 +61,7 @@ export type BoxPlotTooltipContext = TooltipContext { + legend?: ChartLegendConfig /** Mean marker radius in CSS pixels. Defaults to 3. */ meanRadius?: number /** Whisker cap width as a fraction of the box width. Defaults to 0.6. */ @@ -115,12 +127,13 @@ function BoxPlotInner({ const { yScaleType = 'linear', showGrid = false, + showAxisLines = false, meanRadius = 3, whiskerCapRatio = 0.6, boxStrokeWidth = 1.5, } = config ?? {} - - const grouped = series.filter((s) => !s.visibility?.excluded).length > 1 + const { x: xAxisLine, y: yAxisLine } = resolveAxisLines(showAxisLines) + const axisLines = useMemo(() => ({ x: xAxisLine, y: yAxisLine }), [xAxisLine, yAxisLine]) const adaptedSeries = useMemo>[]>( () => @@ -138,19 +151,22 @@ function BoxPlotInner({ [series, labels.length] ) + const { visibleSeries, legendProps } = useChartLegend(adaptedSeries, theme, config?.legend) + const grouped = visibleSeries.filter((s) => !s.visibility?.excluded).length > 1 + /** Synthetic series carrying min/max samples so the y-domain spans every whisker, not just - * the medians on `adaptedSeries.data`. Fed to `createBarScales` (as `stackedSeries`) for - * the d3 scale and to `Chart` (as `valueRangeSeries`) for `useChartMargins` tick sizing — - * one source, two call sites. */ + * the medians. Fed to `createBarScales` (as `stackedSeries`) for the d3 scale and to `Chart` + * (as `valueRangeSeries`) for `useChartMargins` tick sizing, so hidden series cannot affect + * either calculation. */ const valueRangeSeries = useMemo(() => { const out: Series[] = [] - for (const s of series) { + for (const s of visibleSeries) { if (s.visibility?.excluded) { continue } const mins: number[] = [] const maxs: number[] = [] - for (const datum of s.data) { + for (const datum of s.meta?.datums ?? []) { if (!datum) { continue } @@ -169,7 +185,7 @@ function BoxPlotInner({ } } return out - }, [series]) + }, [visibleSeries]) const { datumsByKey, seriesByKey } = useMemo(() => { const datums = new Map() @@ -237,11 +253,13 @@ function BoxPlotInner({ labels: drawLabels, } + const axisLineStyle = axisLines.x || axisLines.y if (showGrid) { drawGrid(baseDrawCtx, { gridColor: theme.gridColor, gridDash: theme.gridDashPattern, orientation: 'vertical', + frame: !axisLineStyle, }) } @@ -269,8 +287,16 @@ function BoxPlotInner({ lineWidth: boxStrokeWidth, }) } + + if (axisLineStyle) { + drawAxes(baseDrawCtx, { + axisColor: resolveAxisLineColor(theme), + xLine: axisLines.x, + yLine: axisLines.y, + }) + } }, - [showGrid, meanRadius, whiskerCapRatio, boxStrokeWidth] + [showGrid, axisLines, meanRadius, whiskerCapRatio, boxStrokeWidth] ) const drawHover = useCallback( @@ -371,21 +397,23 @@ function BoxPlotInner({ ) return ( - > - series={adaptedSeries} - labels={labels} - config={{ ...config, axisOrientation: 'vertical' }} - theme={theme} - createScales={createScales} - drawStatic={drawStatic} - drawHover={drawHover} - tooltip={renderTooltip} - onPointClick={onPointClick} - valueRangeSeries={valueRangeSeries.length > 0 ? valueRangeSeries : undefined} - className={className} - dataAttr={dataAttr} - > - {children} - + + > + series={visibleSeries} + labels={labels} + config={{ ...config, axisOrientation: 'vertical' }} + theme={theme} + createScales={createScales} + drawStatic={drawStatic} + drawHover={drawHover} + tooltip={renderTooltip} + onPointClick={onPointClick} + valueRangeSeries={valueRangeSeries.length > 0 ? valueRangeSeries : undefined} + className={className} + dataAttr={dataAttr} + > + {children} + + ) } diff --git a/posthog/schema.py b/posthog/schema.py index abbad6a784c7..a838cce3fabd 100644 --- a/posthog/schema.py +++ b/posthog/schema.py @@ -888,6 +888,21 @@ class BiasRisk(BaseModel): ) +class BoxPlotSettings(BaseModel): + model_config = ConfigDict( + extra="forbid", + ) + excludeOutliers: bool | None = None + maxColumn: str | None = None + meanColumn: str | None = None + medianColumn: str | None = None + minColumn: str | None = None + p25Column: str | None = None + p75Column: str | None = None + seriesColumn: str | None = None + xAxisColumn: str | None = None + + class BreakdownValue(BaseModel): model_config = ConfigDict( extra="forbid", @@ -15080,6 +15095,7 @@ class ChartSettings(BaseModel): model_config = ConfigDict( extra="forbid", ) + boxPlot: BoxPlotSettings | None = None chartStyle: ChartStyle | None = Field( default=None, description=("Chart rendering style overrides (line shape). Only applies to line and area charts."), diff --git a/products/dashboards/frontend/generated/api.schemas.ts b/products/dashboards/frontend/generated/api.schemas.ts index f69379c62e56..30d7db58b423 100644 --- a/products/dashboards/frontend/generated/api.schemas.ts +++ b/products/dashboards/frontend/generated/api.schemas.ts @@ -8625,6 +8625,18 @@ export interface DataTableNodeApi { version?: number | null } +export interface BoxPlotSettingsApi { + excludeOutliers?: boolean | null + maxColumn?: string | null + meanColumn?: string | null + medianColumn?: string | null + minColumn?: string | null + p25Column?: string | null + p75Column?: string | null + seriesColumn?: string | null + xAxisColumn?: string | null +} + export interface HeatmapGradientStopApi { color: string value: number @@ -8771,6 +8783,7 @@ export interface ChartAxisApi { export type ChartSettingsApiResultCustomizations = { [key: string]: ResultCustomizationByValueApi } | null export interface ChartSettingsApi { + boxPlot?: BoxPlotSettingsApi | null /** Chart rendering style overrides (line shape). Only applies to line and area charts. */ chartStyle?: ChartStyleApi | null goalLines?: GoalLineApi[] | null diff --git a/products/product_analytics/backend/presentation/insight.py b/products/product_analytics/backend/presentation/insight.py index 7754e702f8d6..61fc08b3335e 100644 --- a/products/product_analytics/backend/presentation/insight.py +++ b/products/product_analytics/backend/presentation/insight.py @@ -1522,7 +1522,16 @@ def validate_query(self, value: dict[str, Any] | None) -> dict[str, Any]: # Already-wrapped node → use as-is for wrapped_cls in (schema.DataVisualizationNode, schema.InsightVizNode): try: - return wrapped_cls.model_validate(value).model_dump(exclude_none=True, mode="json") + wrapped_node = wrapped_cls.model_validate(value) + normalized_query = wrapped_node.model_dump(exclude_none=True, mode="json") + if isinstance(wrapped_node, schema.DataVisualizationNode): + box_plot = wrapped_node.chartSettings.boxPlot if wrapped_node.chartSettings else None + if box_plot is not None: + normalized_box_plot = normalized_query.setdefault("chartSettings", {}).setdefault("boxPlot", {}) + for field in ("xAxisColumn", "seriesColumn"): + if field in box_plot.model_fields_set and getattr(box_plot, field) is None: + normalized_box_plot[field] = None + return normalized_query except PydanticValidationError: pass diff --git a/products/product_analytics/backend/tests/api/test_insight_query.py b/products/product_analytics/backend/tests/api/test_insight_query.py index 0ef7364fc39c..4dfbcb1f03d5 100644 --- a/products/product_analytics/backend/tests/api/test_insight_query.py +++ b/products/product_analytics/backend/tests/api/test_insight_query.py @@ -2,6 +2,8 @@ from posthog.test.base import APIBaseTest, ClickhouseTestMixin, QueryMatchingTest +from django.test import SimpleTestCase + from parameterized import parameterized from rest_framework import status @@ -12,11 +14,27 @@ from products.product_analytics.backend.presentation.insight import ( AUTO_WRAPPED_INSIGHT_QUERY_KINDS, BARE_RENDERED_INSIGHT_VIZ_SOURCE_KINDS, + MCPInsightSerializer, ) from ee.api.test.base import LicensedTestMixin +class TestMCPInsightSerializer(SimpleTestCase): + def test_preserves_explicit_box_plot_grouping_nulls(self) -> None: + normalized_query = MCPInsightSerializer().validate_query( + { + "kind": "DataVisualizationNode", + "source": {"kind": "HogQLQuery", "query": "select 1"}, + "chartSettings": {"boxPlot": {"xAxisColumn": None, "seriesColumn": None}}, + } + ) + + box_plot = normalized_query["chartSettings"]["boxPlot"] + assert box_plot["xAxisColumn"] is None + assert box_plot["seriesColumn"] is None + + class TestInsight(ClickhouseTestMixin, LicensedTestMixin, APIBaseTest, QueryMatchingTest): maxDiff = None diff --git a/products/product_analytics/frontend/generated/api.schemas.ts b/products/product_analytics/frontend/generated/api.schemas.ts index 64a689ea4c0c..50deeae9c378 100644 --- a/products/product_analytics/frontend/generated/api.schemas.ts +++ b/products/product_analytics/frontend/generated/api.schemas.ts @@ -7787,6 +7787,18 @@ export interface DataTableNodeApi { version?: number | null } +export interface BoxPlotSettingsApi { + excludeOutliers?: boolean | null + maxColumn?: string | null + meanColumn?: string | null + medianColumn?: string | null + minColumn?: string | null + p25Column?: string | null + p75Column?: string | null + seriesColumn?: string | null + xAxisColumn?: string | null +} + export interface HeatmapGradientStopApi { color: string value: number @@ -7933,6 +7945,7 @@ export interface ChartAxisApi { export type ChartSettingsApiResultCustomizations = { [key: string]: ResultCustomizationByValueApi } | null export interface ChartSettingsApi { + boxPlot?: BoxPlotSettingsApi | null /** Chart rendering style overrides (line shape). Only applies to line and area charts. */ chartStyle?: ChartStyleApi | null goalLines?: GoalLineApi[] | null diff --git a/services/mcp/src/api/generated.ts b/services/mcp/src/api/generated.ts index c58803c12256..674b1515d61c 100644 --- a/services/mcp/src/api/generated.ts +++ b/services/mcp/src/api/generated.ts @@ -8452,6 +8452,18 @@ export namespace Schemas { version?: number | null; } + export interface BoxPlotSettings { + excludeOutliers?: boolean | null; + maxColumn?: string | null; + meanColumn?: string | null; + medianColumn?: string | null; + minColumn?: string | null; + p25Column?: string | null; + p75Column?: string | null; + seriesColumn?: string | null; + xAxisColumn?: string | null; + } + export interface HeatmapGradientStop { color: string; value: number; @@ -8607,6 +8619,7 @@ export namespace Schemas { export type ChartSettingsResultCustomizations = {[key: string]: ResultCustomizationByValue} | null; export interface ChartSettings { + boxPlot?: BoxPlotSettings | null; /** Chart rendering style overrides (line shape). Only applies to line and area charts. */ chartStyle?: ChartStyle | null; goalLines?: GoalLine[] | null; From f9ed93001408fb731c62f0e003c09568bbdcf351 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Wed, 26 Aug 2026 21:08:44 +0100 Subject: [PATCH 02/24] refactor(insights): simplify SQL box plot display and chart code Extract the repeated single-Y-axis boolean into a named isSingleAxisChart flag in the display tab, and reuse one height-class expression for the box plot chart's empty, error, and success states. No behavior change. Generated-By: PostHog Desktop Task-Id: f1e69bea-f4e6-44cf-bd12-dc81f8a435c3 --- .../Components/Charts/SqlBoxPlot.tsx | 6 ++++-- .../DataVisualization/Components/DisplayTab.tsx | 12 +++++++----- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/frontend/src/queries/nodes/DataVisualization/Components/Charts/SqlBoxPlot.tsx b/frontend/src/queries/nodes/DataVisualization/Components/Charts/SqlBoxPlot.tsx index 0614ee94fa95..595cbb31df4b 100644 --- a/frontend/src/queries/nodes/DataVisualization/Components/Charts/SqlBoxPlot.tsx +++ b/frontend/src/queries/nodes/DataVisualization/Components/Charts/SqlBoxPlot.tsx @@ -72,10 +72,12 @@ export const SqlBoxPlot = ({ [chartSettings, yAxisSettings] ) + const heightClass = presetChartHeight ? 'h-[60vh]' : 'h-full' + const containerClassName = clsx( className, 'rounded bg-surface-primary flex flex-1 items-center justify-center p-3', - { 'h-[60vh]': presetChartHeight, 'h-full': !presetChartHeight } + heightClass ) if (model.error) { @@ -99,7 +101,7 @@ export const SqlBoxPlot = ({ className={clsx( className, 'rounded bg-surface-primary w-full grow relative overflow-hidden flex flex-col p-3', - { 'h-[60vh]': presetChartHeight, 'h-full': !presetChartHeight } + heightClass )} > { const isPieChart = effectiveVisualizationType === ChartDisplayType.ActionsPie const isScatterPlot = effectiveVisualizationType === ChartDisplayType.ScatterPlot const isBoxPlot = effectiveVisualizationType === ChartDisplayType.BoxPlot + // Scatter and box plots have a single Y axis, so there is no separate right axis to configure. + const isSingleAxisChart = isScatterPlot || isBoxPlot const isLineChart = effectiveVisualizationType === ChartDisplayType.ActionsLineGraph || effectiveVisualizationType === ChartDisplayType.ActionsAreaGraph const renderYAxisSettings = (name: 'leftYAxisSettings' | 'rightYAxisSettings'): JSX.Element => { - const leftPlaceholder = isScatterPlot || isBoxPlot ? 'Y-axis label' : 'Left Y-axis label' + const leftPlaceholder = isSingleAxisChart ? 'Y-axis label' : 'Left Y-axis label' const labelPlaceholder = name === 'leftYAxisSettings' ? leftPlaceholder : 'Right Y-axis label' return ( @@ -194,7 +196,7 @@ export const DisplayTab = (): JSX.Element => { }} /> )} - {!isScatterPlot && !isBoxPlot && ( + {!isSingleAxisChart && ( <> { !isPieChart ? { key: 'left-y-axis', - header: isScatterPlot || isBoxPlot ? 'Y-axis' : 'Left Y-axis', + header: isSingleAxisChart ? 'Y-axis' : 'Left Y-axis', className: 'p-2 flex flex-col gap-2', content: renderYAxisSettings('leftYAxisSettings'), } : null, // A scatter has one gutter per axis, so there is no second Y axis to configure. - !isPieChart && !isScatterPlot && !isBoxPlot + !isPieChart && !isSingleAxisChart ? { key: 'right-y-axis', header: 'Right Y-axis', @@ -350,7 +352,7 @@ export const DisplayTab = (): JSX.Element => { ), } : null, - !isPieChart && !isScatterPlot && !isBoxPlot + !isPieChart && !isSingleAxisChart ? { key: 'goals', header: ( From 59d49d3beec3243ac15cbe85e25a20f92a290310 Mon Sep 17 00:00:00 2001 From: "posthog[bot]" <206114724+posthog[bot]@users.noreply.github.com> Date: Wed, 26 Aug 2026 22:31:16 +0000 Subject: [PATCH 03/24] chore(visual): update storybook baselines 6 updated Run: 15e93842-9e54-48e9-b856-6eced8bb9c0e Co-authored-by: pauldambra <984817+pauldambra@users.noreply.github.com> --- frontend/snapshots.yml | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/frontend/snapshots.yml b/frontend/snapshots.yml index f80686b967ab..e86cd63ff7ac 100644 --- a/frontend/snapshots.yml +++ b/frontend/snapshots.yml @@ -2689,9 +2689,9 @@ snapshots: insights-boldnumber--empty-result--light: hash: v1.k794b7964.2d0c17643991833246321ec917a7b3a49228fb7b217b6bdb8742bbebe1164a5d.bFD6Zu0IU06yXEaw5GNlDjBhvFh8vqBKvXRQwCS53gs insights-boxplot--default--dark: - hash: v1.k794b7964.11bcd18191a129b7b821de0f214b9d5fe9169c98ac2d809c636f87648943d802.DWBO44Qbzd87d8axYy5zm4MSecWPDXxD1I3ZTUUHuUc + hash: v1.k794b7964.0703a7022dee7396b3f005ebe10aeb376345086ef4c7fc2458108ce87351123d.Ps1FFHDhHcNR2ApWX_ypYngS_xpINJtwB9EH-9n8vLU insights-boxplot--default--light: - hash: v1.k794b7964.80daaf49ad0c7b82eb9071a207eeadd6db945e06743ba4b64690613f5b70265c.nDmyP88KGm6rnkO-8qNwFQM205eMFRUngrx5R4BjIws + hash: v1.k794b7964.0e1fc3799ec1876e6ccca4da11c8ebc63836acbe5cbe7453912a385be0564dc5.Pxd_3RMZ6szO03ZwKl6_6UT1AW7TiK_1WllTaLSn60k insights-funnelbarhorizontalchart--breakdown--dark: hash: v1.k794b7964.e1339725b2bf6fd5c152865170f77320dbc22dc941a23fdc81694f3a4b17f884.poYcV8iWzKuJI0od7oM9G9gjUXrgP38w7bGlQushUjg insights-funnelbarhorizontalchart--breakdown--light: @@ -7032,10 +7032,14 @@ snapshots: hash: v1.k794b7964.e0482531b63e3c6c58bb438b0848b476db1ebadb82736444afb0004e5a7d9a2e.EgJaf86aq6hyYkPzdF8Aydv7rycYiRr_UV8fYh_LIL0 scenes-app-insights-side-panel-actions--unsaved-insight--light: hash: v1.k794b7964.2b3ed2b5ffec763b0f7e13af74c9e9d57df96464976ae6014512a0d7def42ad3.paLp2yPHgkf2-ncauikOAUdR87k6-KhAh-Nm4uyQ380 + scenes-app-insights-sqlboxplot--edit-options--dark: + hash: v1.k794b7964.70df9d8ffa57acac2d95f47d3298bb62ca009f1f4b556717ee0bbe319ab7754e.VwWfKL8TJDDpSmrV3mWOD0VkvzSnBrYJTFtSRpxlQcw + scenes-app-insights-sqlboxplot--edit-options--light: + hash: v1.k794b7964.836958043bd3b8203146f802f75a29f5edb8ad4c056a5e2cd01d75a14fa453bb.lDduPTYwYFGPpa1wZHBJEHrKfyxy9jVNMI9-pUQAAjE scenes-app-insights-sqlboxplot--grouped-series--dark: - hash: v1.k794b7964.9958e584a1eb346036636cec5e7f6aa21367045261cec6f9bc9474570976916f.WYCNc1X4istzHZnAsqYBx9xNFe0Jek1UegIJ-J-eSog + hash: v1.k794b7964.0062f9e095cb6266619d2886be6973be68c7fc906f50d8b37064390de6757656.frURXaRskPxT8wSqipNvLkFVNmgq3uiSKORwD_fLWDA scenes-app-insights-sqlboxplot--grouped-series--light: - hash: v1.k794b7964.fce0c9ba15afa1bfe37728e10742395345a06e2a082d34a145e32c80a41432a8.lF9z6z4b7ptkoQzmfE7tlnr7CJQJ7Ki3rypJuW903Zs + hash: v1.k794b7964.d6214c2e48859cedc4c7dc30552657668a7e9a1a7f92f4643b3d2dbe56df2d57.ytDzXwguLj-27G1jRP28BG-bv7d6q1z4CgNsAmbpqgw scenes-app-insights-sqllinechart--sql-bar-chart-value-labels-quill--dark: hash: v1.k794b7964.482f71c0aadde604594294d6e723590838e5b47f543adf3baf39c5ad2f2a9c3e.bpFdUZ6dFKKinvbM5jklQHy3pQoeURgjrWtjcXipKBw scenes-app-insights-sqllinechart--sql-bar-chart-value-labels-quill--light: From 358adbea8f4c439b32b6236d34c42db641eec26c Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 27 Aug 2026 10:44:44 +0200 Subject: [PATCH 04/24] chore(devex): add a CI prior-art lookup for ideas already tried CI ideas get re-proposed because the evidence against them is scattered across closed PRs and revert commits nobody greps. The same handful keeps coming back: xdist in the shards, BuildKit cache mounts, sharding the E2E suite, coverage-based test selection. Adds docs/internal/ci-things-already-tried.md, keyed by the proposal rather than the symptom, because the lookup happens when someone has an idea, not when they are debugging. Each entry carries the verdict, the date, the measurement, and a line of alternate phrasings so a differently-worded grep still lands. Verdicts are not bans. Every entry states the specific blocker so a reader can check whether it still holds; runner sizes and tooling move. Seeded from Julian's merged, reverted, and abandoned PRs since Oct 2025. Narrowed with local git: revert titles, plus lines added in one PR and removed in a later one across the CI hot files. That cut 847 PRs to 333 candidates, then to the entries here. The five CI skills that would otherwise carry duplicate copies get a one-line pointer instead. --- .../skills/authoring-ci-workflows/SKILL.md | 2 + .agents/skills/debugging-ci-failures/SKILL.md | 2 + .../skills/depot-container-builds/SKILL.md | 2 + .agents/skills/fixing-flaky-tests/SKILL.md | 2 + .../skills/maintaining-python-tests/SKILL.md | 2 + docs/internal/ci-things-already-tried.md | 308 ++++++++++++++++++ 6 files changed, 318 insertions(+) create mode 100644 docs/internal/ci-things-already-tried.md diff --git a/.agents/skills/authoring-ci-workflows/SKILL.md b/.agents/skills/authoring-ci-workflows/SKILL.md index b2d3fdd940cb..1721bf6d0b76 100644 --- a/.agents/skills/authoring-ci-workflows/SKILL.md +++ b/.agents/skills/authoring-ci-workflows/SKILL.md @@ -9,6 +9,8 @@ description: > # Authoring CI workflows +Before you propose a change to CI, check [things already tried](../../../docs/internal/ci-things-already-tried.md) for the idea. It records what was measured, and why some good-sounding changes were reverted or rejected. + Conventions for `.github/workflows/**` and `.github/actions/**`. The linters own the mechanical rules (below); this skill is the **judgment calls** they can't enforce. diff --git a/.agents/skills/debugging-ci-failures/SKILL.md b/.agents/skills/debugging-ci-failures/SKILL.md index 76e8b2eb0e0f..a6b3345383a6 100644 --- a/.agents/skills/debugging-ci-failures/SKILL.md +++ b/.agents/skills/debugging-ci-failures/SKILL.md @@ -15,6 +15,8 @@ description: > # Debugging PostHog CI failures +Before you propose a change to CI, check [things already tried](../../../docs/internal/ci-things-already-tried.md) for the idea. It records what was measured, and why some good-sounding changes were reverted or rejected. + Find the first meaningful failure, classify it, reproduce the smallest useful case locally when appropriate, and report the result. Avoid public-visible or irreversible actions unless the user explicitly asks. diff --git a/.agents/skills/depot-container-builds/SKILL.md b/.agents/skills/depot-container-builds/SKILL.md index 03b89fa41c81..ceac7567a308 100644 --- a/.agents/skills/depot-container-builds/SKILL.md +++ b/.agents/skills/depot-container-builds/SKILL.md @@ -12,6 +12,8 @@ description: > # Depot Container Builds +Before you propose a change to CI, check [things already tried](../../../docs/internal/ci-things-already-tried.md) for the idea. It records what was measured, and why some good-sounding changes were reverted or rejected. + Depot runs Docker image builds on remote high-performance builders (16 CPU, 32 GB RAM, NVMe SSD cache). `depot build` is a drop-in replacement for `docker build` / `docker buildx build`. `depot bake` replaces `docker buildx bake`. ## Project Selection for Multi-Org Users diff --git a/.agents/skills/fixing-flaky-tests/SKILL.md b/.agents/skills/fixing-flaky-tests/SKILL.md index 3ea897e79c2c..07fea3c611dc 100644 --- a/.agents/skills/fixing-flaky-tests/SKILL.md +++ b/.agents/skills/fixing-flaky-tests/SKILL.md @@ -9,6 +9,8 @@ description: > # Fixing flaky tests +Before you propose a change to how the suite runs in CI, check [things already tried](../../../docs/internal/ci-things-already-tried.md). It records measured verdicts on test parallelism, sharding, and coverage-based selection, so a rejected approach is not rebuilt. + Three non-negotiables, in order: 1. **Reproduce before you fix.** A fix for a failure you never observed is a guess. diff --git a/.agents/skills/maintaining-python-tests/SKILL.md b/.agents/skills/maintaining-python-tests/SKILL.md index 38647c440a0b..628823755615 100644 --- a/.agents/skills/maintaining-python-tests/SKILL.md +++ b/.agents/skills/maintaining-python-tests/SKILL.md @@ -6,6 +6,8 @@ description: > # Maintaining Python tests +Before you propose a change to how the suite runs in CI, check [things already tried](../../../docs/internal/ci-things-already-tried.md). It records measured verdicts on test parallelism, sharding, and coverage-based selection, so a rejected approach is not rebuilt. + Use this skill for an existing Python test suite. Use `/writing-tests` before adding or substantially changing coverage. Use `/fixing-flaky-tests` when intermittent failure is the main problem. The goal is not a smaller test count. The goal is a suite that catches the same realistic regressions with less compute, less waiting, and less maintenance. diff --git a/docs/internal/ci-things-already-tried.md b/docs/internal/ci-things-already-tried.md new file mode 100644 index 000000000000..d421a3fc2c7c --- /dev/null +++ b/docs/internal/ci-things-already-tried.md @@ -0,0 +1,308 @@ +# CI: things already tried + +A lookup list for one question: **someone has an idea for CI or the dev environment. Was it already tried?** + +Most CI ideas here are good ideas. They were tried because they sounded right. +The value of this file is the part that is expensive to rediscover: what happened when someone actually built it, and why the result did not match the pitch. + +## How to use this + +Search before you build, not after. + +```bash +rg -i "xdist|parallel" docs/internal/ci-things-already-tried.md +``` + +Entries are titled as the **proposal**, in the words someone would use to propose it. +They are not titled by the symptom that eventually showed up. +Each entry ends with _Also asked as_, which exists so a grep for different wording still lands. + +**A verdict is not a ban.** Every entry carries the date and the specific reason it failed. +Read the reason and check whether it still holds. Runner sizes, prices, and tooling all move. +If the blocker is gone, say so in the PR and try again. + +## Verdicts + +| Verdict | Meaning | +| ------------ | ----------------------------------------------------- | +| `rejected` | Built and measured. The result did not justify it. | +| `reverted` | Shipped to master, then pulled back out. | +| `superseded` | The problem was real. A different approach solved it. | +| `abandoned` | Started, never finished. No verdict was ever reached. | +| `open` | Good idea, still unfinished. Worth picking up. | + +## Adding an entry + +Add one when you close a PR without merging it, or when you revert something. +Title it as the idea. State the verdict, the date, and the measurement. +One entry, five lines, is worth more than a design doc nobody opens. + +--- + +## Test parallelism and sharding + +### Run pytest-xdist inside the backend CI shards + +**Verdict: rejected** · Oct 2025 · [#38927](https://github.com/PostHog/posthog/pull/38927) + +Measured across 53 shards with `-n 4`. +Wall time fell from about 15 minutes to about 9, a 42.8% speedup that was statistically solid. +CPU cost rose from 1,572 to 3,908 core-minutes, about 2.5x. + +The speedup was real. The price was the problem. +Buying wall-clock with a 2.5x compute multiplier did not clear the bar. + +`pytest-xdist` is still a dev dependency, so it works locally. It is not wired into the CI shards. + +_Also asked as:_ parallelize tests within a shard, `-n auto`, use the idle cores on the runner, why is each shard single-process + +### Shard the Playwright E2E suite + +**Verdict: reverted** · Feb 2026 · [#46774](https://github.com/PostHog/posthog/pull/46774), reverted by [#46853](https://github.com/PostHog/posthog/pull/46853) + +Four shards were added to narrow the retry scope for flaky tests. +The setup cost per shard was the thing that was missed: about 7.5 minutes, of which migrations alone are 3 minutes. + +All 110 tests run in about 4 minutes with 6 workers. +So 4 shards spent roughly 22 extra minutes of CPU per run to save about 3 minutes of wall clock. + +Retrying a shard also replays the 7.5 minute setup, so a "fast" shard retry was not much faster than rerunning everything. + +The workflow still carries an inline note pointing at this decision. See the `runs-on` comment in `.github/workflows/ci-e2e-playwright.yml`. + +_Also asked as:_ split the E2E tests across runners, parallelize Playwright, reduce flaky retry scope by sharding + +### Use Bazel to scope product tests + +**Verdict: abandoned** · opened Dec 2025, closed Mar 2026 · [#43397](https://github.com/PostHog/posthog/pull/43397) + +Products would opt into Bazel targets so a product-only change could skip the legacy pytest jobs. +The branch went stale and was closed without a verdict, so this is not evidence that Bazel cannot work. + +The same goal was met another way. Product tests moved to Turborepo in [#46971](https://github.com/PostHog/posthog/pull/46971), and file-level backend selection followed later. + +_Also asked as:_ Bazel, build graph for test selection, only run tests for the product I changed + +## Test selection + +### Exclude `products/**/backend/**` from the backend paths filter + +**Verdict: reverted** · Mar 2026 · [#50137](https://github.com/PostHog/posthog/pull/50137), reverted by [#50181](https://github.com/PostHog/posthog/pull/50181) + +The filter left product backend changes to `contract-check`, which decides whether Django tests are needed. + +That assumed products are isolated. Most were not. +Core code imports product views, serializers, and models directly, through `posthog/api/__init__.py`, `posthog/tasks/`, and migrations. +`contract-check` only watches facade files, so it could not see those crossings. + +The lesson generalizes: a path filter that skips tests is a claim about the import graph. Check the graph before making the claim. + +_Also asked as:_ narrow the backend paths filter, skip Django tests for product-only changes, trust contract-check + +### Certify a facade by reading its `__all__` + +**Verdict: superseded** · Jul 2026 · [#71127](https://github.com/PostHog/posthog/pull/71127), replaced by [#71486](https://github.com/PostHog/posthog/pull/71486) + +The detector tried to prove a facade does not re-export internals by inspecting `__all__`. + +Two things broke it. The detector grew a new hole in every review round. +More basically, most facade modules declare no `__all__` at all, so it could only ever certify the part of the surface that was advertised. + +The replacement writes the rule into `products/architecture.md` and makes `contract-check` inputs narrow-or-nothing instead of a per-file glob list. + +_Also asked as:_ detect facade leaks, check `__all__`, verify a product is really isolated + +### Select tests from pytest-testmon coverage data + +**Verdict: superseded** · Apr 2026 · [#56370](https://github.com/PostHog/posthog/pull/56370) + +This collected the data rather than wiring the selection. +52 shard artifacts were merged into a map of 28,322 tests over 3,691 production files, about 1.3M mappings. + +The selectivity was strong: a single changed file triggered a median of 45 tests, a 99.8% skip rate. + +Two findings matter more than the numbers. +There were no high-confidence stale tests, so a testmon-driven cleanup had nothing to delete. +And 1,020 tests appeared to touch no production code, but nearly all were false positives from mock-heavy async code, property-based tests, and migration-rule tests. Testmon cannot trace through those. + +Backend test selection later shipped from a different mechanism. See [#85530](https://github.com/PostHog/posthog/pull/85530) and [#88265](https://github.com/PostHog/posthog/pull/88265). + +_Also asked as:_ coverage-based test selection, testmon, find stale tests from coverage, only run affected tests + +### Disable pytest's `unraisableexception` and `threadexception` plugins + +**Verdict: open, and already approved** · Jul 2026 · [#70886](https://github.com/PostHog/posthog/pull/70886) + +Every pytest session pays several full-heap `gc.collect()` passes at cleanup. +Those plugins run them only to report `__del__` and thread exceptions as warnings. +`addopts` already sets `-p no:warnings`, so those warnings can never become failures. The passes are pure teardown cost. + +Measured on a fixed 320-test benchmark: 24.7s to 21.8s. + +The PR was reviewed and approved. It then went stale and closed without merging. +It is worth reopening as-is. + +This is the entry to read before reaching for a different fix to pytest teardown cost. +A related attempt in [#88759](https://github.com/PostHog/posthog/pull/88759) tried to reorder a GC freeze around the same cost, by deleting the `gc.unfreeze()` in `pytest_unconfigure`. +That unfreeze is not incidental. It was added in [#62707](https://github.com/PostHog/posthog/pull/62707) after the Temporal shards segfaulted with exit 139, and CI reproduced the same crash on #88759. +Frozen objects skip the final cyclic collections of `Py_FinalizeEx`, so their finalizers run in late teardown, after extension modules are gone. + +_Also asked as:_ pytest teardown is slow, reduce gc.collect at session end, speed up pytest cleanup, why does the shard hang after tests pass + +## Docker and image builds + +### Apply BuildKit cache mounts to the Dockerfile + +**Verdict: rejected** · Oct 2025 · [#39700](https://github.com/PostHog/posthog/pull/39700) + +Cache mounts were added for apt, pip, uv, node, and Playwright, following Depot's published guidance. + +Measured against master with a warm cache, it was slower. +Backend changes went from 52.7s to 57.5s, about 9% slower. Frontend changes went from 55.5s to 62.2s, about 12% slower. + +The reason is workload shape. Cache mounts add 5 to 7 seconds of overhead even on a hit, and they only pay off when dependencies change. +About 95% of PRs change code, not dependencies. So the change taxed the common case to speed up the rare one. + +_Also asked as:_ `--mount=type=cache`, speed up Docker builds, follow Depot cache best practices + +### Move source COPY to the end of the Dockerfile + +**Verdict: rejected** · Oct 2025 · [#39695](https://github.com/PostHog/posthog/pull/39695) + +It was already there. The Dockerfile's layer order was already correct, so the change was a no-op. + +Worth remembering as a class of idea: confirm the current state before optimizing it. + +_Also asked as:_ improve Docker layer caching, reorder Dockerfile layers + +### Chase Docker Hub credentials when CI hits pull rate limits + +**Verdict: superseded by the real cause** · Aug 2026 · [#81963](https://github.com/PostHog/posthog/pull/81963) + +CI failed with `toomanyrequests: You have reached your unauthenticated pull rate limit`. +At peak this hit roughly 45% of backend CI jobs, against a baseline of zero. + +The message points at authentication, and that is what made it expensive. +`docker login` succeeded the whole time, and `Login Succeeded` was accurate. + +The actual cause was a Docker Hub **billing lapse**. A lapsed plan removes entitlement, so Docker issues anonymous-class pull tokens while still accepting the login. + +If this wording appears again, check the plan status before re-plumbing secrets. + +_Also asked as:_ Docker Hub rate limit in CI, unauthenticated pull limit, DOCKERHUB secret is wrong + +## CI orchestration + +### Skip Storybook and E2E on bot snapshot-only commits + +**Verdict: reverted** · Mar 2026 · [#49997](https://github.com/PostHog/posthog/pull/49997), reverted by [#51212](https://github.com/PostHog/posthog/pull/51212) + +Shipped, then pulled back a week later. + +_Also asked as:_ skip CI for snapshot commits, ignore bot commits in CI, don't rerun visual tests for the snapshot bot + +### Force-cancel backend CI when pytest hangs on cancellation + +**Verdict: reverted** · Apr 2026 · [#54261](https://github.com/PostHog/posthog/pull/54261), reverted by [#54685](https://github.com/PostHog/posthog/pull/54685) + +A watchdog job was added to force-cancel a run when pytest would not exit. +It lasted one day. + +_Also asked as:_ cancel watchdog, kill hung CI jobs, pytest ignores SIGTERM + +### Add a CI step that nudges humans to self-assign on bot PRs + +**Verdict: rejected** · Jun 2026 · [#62111](https://github.com/PostHog/posthog/pull/62111) + +Bot-authored PRs cannot be auto-assigned, since the bot account is not a team member. +A CI step tried to nudge a human into claiming them. + +It was dropped for a lighter approach. A CI step has to guess who is behind a bot PR. +The agent opening the PR already knows, so the guidance moved into the PR template instead. + +_Also asked as:_ auto-assign bot PRs, find the human behind an agent PR, nudge for ownership + +## Dev environment + +### Run a dmypy daemon for fast local type checking + +**Verdict: rejected** · Oct 2025 · [#39319](https://github.com/PostHog/posthog/pull/39319) + +A pre-commit hook used the daemon when it was already running, giving roughly 0.6 to 1.7s checks once warm. + +The warm-up never got cheaper, so the first check still paid full cost. +Starting the daemon from mprocs was tried too, and then commits hung while the daemon was still warming. + +_Also asked as:_ dmypy, speed up mypy locally, type-check on commit, mypy daemon + +### Run `uv sync` on every flox re-activate + +**Verdict: rejected** · Feb 2026 · [#49183](https://github.com/PostHog/posthog/pull/49183) + +Shell profiles would sync dependencies on every shell start, about 660ms when already current. + +It was dropped as trying too hard. Profiles run in subshells too, so the cost is paid far more often than the problem occurs. + +_Also asked as:_ auto-sync deps, keep the venv current automatically, uv sync in the shell profile + +### Upgrade Python past what flox's uv can install + +**Verdict: reverted** · Oct 2025 · [#40286](https://github.com/PostHog/posthog/pull/40286), reverted by [#40290](https://github.com/PostHog/posthog/pull/40290) + +3.12.12 needs uv 0.9.2 or newer. Flox pinned uv 0.8.23, which could only fetch up to 3.12.10. +Anyone without 3.12.12 already installed could not build the environment. + +The constraint is the shape to remember, not the versions. The repo is now on 3.13.13, so this specific pin is long gone. +Before bumping Python, check what the flox-pinned uv can actually fetch. + +_Also asked as:_ bump Python, upgrade the interpreter, why is Python pinned to an exact version + +### Replace Unit and Uvicorn with Granian everywhere at once + +**Verdict: superseded** · Oct 2025 · [#40450](https://github.com/PostHog/posthog/pull/40450), replaced by [#40847](https://github.com/PostHog/posthog/pull/40847) + +The straight swap was closed for a dual-mode version that defaults to Unit and enables Granian behind `USE_GRANIAN=true`. +Same work, safer rollout. Granian is a dependency today. + +_Also asked as:_ migrate to Granian, replace Nginx Unit, unify the ASGI server + +### Swap the object storage service from MinIO to SeaweedFS + +**Verdict: eventually shipped, after several failed attempts** · first attempt Mar 2026 · [#49827](https://github.com/PostHog/posthog/pull/49827) + +Worth knowing that earlier attempts by other people also failed before this one. +It has since landed. Both S3-compatible stores in the dev and CI stack are SeaweedFS now, and `AGENTS.md` treats new MinIO dependencies as off-limits. + +Read this entry as evidence that a repeatedly failed migration can still be the right call. + +_Also asked as:_ remove MinIO, SeaweedFS, replace the object storage container + +## Product isolation + +### Use `logs` as the first product to isolate behind a facade + +**Verdict: superseded** · Jun 2026 · [#63184](https://github.com/PostHog/posthog/pull/63184) + +`logs` was picked as the field test. Core imported its models, query runner, celery task, and temporal wiring, so it was a genuinely hard case. + +The field test moved to `web_analytics` in [#63535](https://github.com/PostHog/posthog/pull/63535), and the tooling and skill were consolidated in [#63193](https://github.com/PostHog/posthog/pull/63193). +The note on closing was that `logs` can be re-cut from that tooling quickly if it is still wanted. +The doctrine that came out of this line of work landed later, in [#71486](https://github.com/PostHog/posthog/pull/71486). + +_Also asked as:_ which product should we isolate first, facade migration example, isolate logs + +## Splitting work into PRs + +### Split closely coupled layers into separate stacked PRs + +**Verdict: rejected for this case** · Aug 2026 · [#87643](https://github.com/PostHog/posthog/pull/87643), folded into [#87644](https://github.com/PostHog/posthog/pull/87644) + +Two layers of a change read well as two stories but not as two diffs. +Both rewrote the same four modules, sometimes the same lines. +The upper layer deleted a block the lower layer was fixing, and re-signatured a function the lower layer was splitting. + +Every fix had to be replayed through those collisions, and the replay repeats on each restack. + +Split by diff surface, not by narrative. If two layers touch the same lines, one PR is cheaper to review than two. + +_Also asked as:_ should I stack these, split this PR, break the change into reviewable layers From 4c5b6eb5fc79157a9330e81ed4b7e48691c6c1bf Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 27 Aug 2026 10:46:59 +0200 Subject: [PATCH 05/24] chore(devex): rewrite the CI prior-art doc in Simplified Technical English The first draft read like PR prose: idioms, passive constructions, and long sentences that carry two ideas. Agents and non-native speakers both parse that worse, and this file exists to be scanned fast during a lookup. Rewrites to ASD-STE100 writing rules while keeping full vocabulary: active voice, simple tenses, one idea per sentence, consistent terms for the same thing. Now averages under 10 words per sentence with none over 25. No entries, verdicts, or PR links changed. --- docs/internal/ci-things-already-tried.md | 256 ++++++++++++----------- 1 file changed, 129 insertions(+), 127 deletions(-) diff --git a/docs/internal/ci-things-already-tried.md b/docs/internal/ci-things-already-tried.md index d421a3fc2c7c..6e23bc116aa9 100644 --- a/docs/internal/ci-things-already-tried.md +++ b/docs/internal/ci-things-already-tried.md @@ -1,41 +1,41 @@ # CI: things already tried -A lookup list for one question: **someone has an idea for CI or the dev environment. Was it already tried?** +This file answers one question. **You have an idea for CI or the dev environment. Did someone try it before?** -Most CI ideas here are good ideas. They were tried because they sounded right. -The value of this file is the part that is expensive to rediscover: what happened when someone actually built it, and why the result did not match the pitch. +Most of the ideas here are good ideas. People tried them because they sounded correct. +This file records the part that costs the most to find again. It records what happened when someone built the idea, and why the result did not agree with the proposal. -## How to use this +## How to use this file -Search before you build, not after. +Search before you build. ```bash rg -i "xdist|parallel" docs/internal/ci-things-already-tried.md ``` -Entries are titled as the **proposal**, in the words someone would use to propose it. -They are not titled by the symptom that eventually showed up. -Each entry ends with _Also asked as_, which exists so a grep for different wording still lands. +Each entry has the title of the **proposal**. The title uses the words that a person uses to propose the idea. +The title does not use the symptom that appeared later. +Each entry ends with _Also asked as_. This line gives other words for the same idea, so that a different search finds the entry. -**A verdict is not a ban.** Every entry carries the date and the specific reason it failed. -Read the reason and check whether it still holds. Runner sizes, prices, and tooling all move. -If the blocker is gone, say so in the PR and try again. +**A verdict is not a prohibition.** Each entry gives the date and the specific reason for the result. +Read the reason. Then examine if the reason is still correct. Runner sizes, prices, and tools change. +If the reason is no longer correct, write this in the PR and try the idea again. ## Verdicts -| Verdict | Meaning | -| ------------ | ----------------------------------------------------- | -| `rejected` | Built and measured. The result did not justify it. | -| `reverted` | Shipped to master, then pulled back out. | -| `superseded` | The problem was real. A different approach solved it. | -| `abandoned` | Started, never finished. No verdict was ever reached. | -| `open` | Good idea, still unfinished. Worth picking up. | +| Verdict | Meaning | +| ------------ | --------------------------------------------------------------- | +| `rejected` | Someone built the idea and measured it. The result was too bad. | +| `reverted` | The change went to master. Then someone removed it. | +| `superseded` | The problem was real. A different solution replaced this one. | +| `abandoned` | Someone started the work and stopped. There is no verdict. | +| `open` | The idea is good. The work is incomplete. You can continue it. | -## Adding an entry +## Add an entry -Add one when you close a PR without merging it, or when you revert something. -Title it as the idea. State the verdict, the date, and the measurement. -One entry, five lines, is worth more than a design doc nobody opens. +Add an entry when you close a PR and do not merge it. Add an entry when you revert a change. +Give the entry the title of the idea. Then give the verdict, the date, and the measurement. +An entry of five lines has more value than a design document that nobody opens. --- @@ -45,14 +45,14 @@ One entry, five lines, is worth more than a design doc nobody opens. **Verdict: rejected** · Oct 2025 · [#38927](https://github.com/PostHog/posthog/pull/38927) -Measured across 53 shards with `-n 4`. -Wall time fell from about 15 minutes to about 9, a 42.8% speedup that was statistically solid. -CPU cost rose from 1,572 to 3,908 core-minutes, about 2.5x. +The test used 53 shards and `-n 4`. +Wall time decreased from approximately 15 minutes to approximately 9 minutes. This is a speed increase of 42.8%, and the measurement is statistically strong. +CPU cost increased from 1,572 to 3,908 core-minutes. This is a factor of approximately 2.5. -The speedup was real. The price was the problem. -Buying wall-clock with a 2.5x compute multiplier did not clear the bar. +The speed increase is real. The cost is the problem. +A factor of 2.5 in compute is too much for 3 minutes of wall time. -`pytest-xdist` is still a dev dependency, so it works locally. It is not wired into the CI shards. +`pytest-xdist` is still a development dependency, and it operates correctly on a local machine. CI does not use it in the shards. _Also asked as:_ parallelize tests within a shard, `-n auto`, use the idle cores on the runner, why is each shard single-process @@ -60,26 +60,28 @@ _Also asked as:_ parallelize tests within a shard, `-n auto`, use the idle cores **Verdict: reverted** · Feb 2026 · [#46774](https://github.com/PostHog/posthog/pull/46774), reverted by [#46853](https://github.com/PostHog/posthog/pull/46853) -Four shards were added to narrow the retry scope for flaky tests. -The setup cost per shard was the thing that was missed: about 7.5 minutes, of which migrations alone are 3 minutes. +The change added four shards. The purpose was a smaller retry set for unreliable tests. -All 110 tests run in about 4 minutes with 6 workers. -So 4 shards spent roughly 22 extra minutes of CPU per run to save about 3 minutes of wall clock. +The setup cost of each shard is the item that the proposal did not include. Each shard needs approximately 7.5 minutes of setup. The migrations alone need 3 minutes. -Retrying a shard also replays the 7.5 minute setup, so a "fast" shard retry was not much faster than rerunning everything. +All 110 tests complete in approximately 4 minutes with 6 workers. +Thus four shards used approximately 22 more minutes of CPU in each run, and decreased wall time by approximately 3 minutes. -The workflow still carries an inline note pointing at this decision. See the `runs-on` comment in `.github/workflows/ci-e2e-playwright.yml`. +A shard retry also repeats the 7.5 minutes of setup. Thus a retry of one shard is not much faster than a retry of all the tests. + +The workflow contains a comment about this decision. Read the comment near `runs-on` in `.github/workflows/ci-e2e-playwright.yml`. _Also asked as:_ split the E2E tests across runners, parallelize Playwright, reduce flaky retry scope by sharding ### Use Bazel to scope product tests -**Verdict: abandoned** · opened Dec 2025, closed Mar 2026 · [#43397](https://github.com/PostHog/posthog/pull/43397) +**Verdict: abandoned** · Dec 2025 to Mar 2026 · [#43397](https://github.com/PostHog/posthog/pull/43397) + +Each product could select Bazel targets. Then a change to one product could skip the legacy pytest jobs. -Products would opt into Bazel targets so a product-only change could skip the legacy pytest jobs. -The branch went stale and was closed without a verdict, so this is not evidence that Bazel cannot work. +The branch became inactive, and the stale bot closed it. Nobody measured the result. Thus this entry is not evidence against Bazel. -The same goal was met another way. Product tests moved to Turborepo in [#46971](https://github.com/PostHog/posthog/pull/46971), and file-level backend selection followed later. +A different solution achieved the same goal. The product tests moved to Turborepo in [#46971](https://github.com/PostHog/posthog/pull/46971). File-level backend selection came later. _Also asked as:_ Bazel, build graph for test selection, only run tests for the product I changed @@ -89,26 +91,26 @@ _Also asked as:_ Bazel, build graph for test selection, only run tests for the p **Verdict: reverted** · Mar 2026 · [#50137](https://github.com/PostHog/posthog/pull/50137), reverted by [#50181](https://github.com/PostHog/posthog/pull/50181) -The filter left product backend changes to `contract-check`, which decides whether Django tests are needed. +The filter gave the decision to `contract-check`. `contract-check` decides if the Django tests are necessary for a change in a product. -That assumed products are isolated. Most were not. -Core code imports product views, serializers, and models directly, through `posthog/api/__init__.py`, `posthog/tasks/`, and migrations. -`contract-check` only watches facade files, so it could not see those crossings. +This assumes that the products are isolated. Most products were not isolated. +Core code imports product views, serializers, and models directly. It imports them through `posthog/api/__init__.py`, `posthog/tasks/`, and the migrations. +`contract-check` examines only the facade files. Thus it cannot see these imports. -The lesson generalizes: a path filter that skips tests is a claim about the import graph. Check the graph before making the claim. +The rule is general. A path filter that skips tests makes a statement about the import graph. Examine the import graph before you make the statement. _Also asked as:_ narrow the backend paths filter, skip Django tests for product-only changes, trust contract-check -### Certify a facade by reading its `__all__` +### Certify a facade with its `__all__` **Verdict: superseded** · Jul 2026 · [#71127](https://github.com/PostHog/posthog/pull/71127), replaced by [#71486](https://github.com/PostHog/posthog/pull/71486) -The detector tried to prove a facade does not re-export internals by inspecting `__all__`. +The detector read `__all__` to prove that a facade does not re-export internal names. -Two things broke it. The detector grew a new hole in every review round. -More basically, most facade modules declare no `__all__` at all, so it could only ever certify the part of the surface that was advertised. +Two problems stopped it. Each review round found a new gap in the detector. +Also, most facade modules do not declare `__all__`. Thus the detector could certify only the part of the surface that the module declares. -The replacement writes the rule into `products/architecture.md` and makes `contract-check` inputs narrow-or-nothing instead of a per-file glob list. +The replacement puts the rule in `products/architecture.md`. It also makes the `contract-check` inputs narrow or absent, instead of a list of file globs. _Also asked as:_ detect facade leaks, check `__all__`, verify a product is really isolated @@ -116,38 +118,38 @@ _Also asked as:_ detect facade leaks, check `__all__`, verify a product is reall **Verdict: superseded** · Apr 2026 · [#56370](https://github.com/PostHog/posthog/pull/56370) -This collected the data rather than wiring the selection. -52 shard artifacts were merged into a map of 28,322 tests over 3,691 production files, about 1.3M mappings. +This PR collected the data. It did not connect the selection to CI. +The merge of 52 shard artifacts gave a map of 28,322 tests over 3,691 production files. The map has approximately 1.3 million entries. -The selectivity was strong: a single changed file triggered a median of 45 tests, a 99.8% skip rate. +The selectivity was high. One changed file caused a median of 45 tests. This is a skip rate of 99.8%. -Two findings matter more than the numbers. -There were no high-confidence stale tests, so a testmon-driven cleanup had nothing to delete. -And 1,020 tests appeared to touch no production code, but nearly all were false positives from mock-heavy async code, property-based tests, and migration-rule tests. Testmon cannot trace through those. +Two results have more importance than these numbers. +First, there were no stale tests with high confidence. Thus a cleanup from this data had nothing to delete. +Second, 1,020 tests appeared to touch no production code. Almost all of these results are false. They come from code with many mocks, from property-based tests, and from tests of migration rules. Testmon cannot trace these paths. -Backend test selection later shipped from a different mechanism. See [#85530](https://github.com/PostHog/posthog/pull/85530) and [#88265](https://github.com/PostHog/posthog/pull/88265). +Backend test selection came later from a different mechanism. Read [#85530](https://github.com/PostHog/posthog/pull/85530) and [#88265](https://github.com/PostHog/posthog/pull/88265). _Also asked as:_ coverage-based test selection, testmon, find stale tests from coverage, only run affected tests -### Disable pytest's `unraisableexception` and `threadexception` plugins +### Disable the pytest `unraisableexception` and `threadexception` plugins -**Verdict: open, and already approved** · Jul 2026 · [#70886](https://github.com/PostHog/posthog/pull/70886) +**Verdict: open, and approved** · Jul 2026 · [#70886](https://github.com/PostHog/posthog/pull/70886) -Every pytest session pays several full-heap `gc.collect()` passes at cleanup. -Those plugins run them only to report `__del__` and thread exceptions as warnings. -`addopts` already sets `-p no:warnings`, so those warnings can never become failures. The passes are pure teardown cost. +Each pytest session runs several full-heap `gc.collect()` passes at cleanup. +These plugins run the passes only to report `__del__` exceptions and thread exceptions as warnings. +`addopts` already sets `-p no:warnings`. Thus these warnings cannot become failures, and the passes give no value. -Measured on a fixed 320-test benchmark: 24.7s to 21.8s. +A fixed benchmark of 320 tests decreased from 24.7 seconds to 21.8 seconds. -The PR was reviewed and approved. It then went stale and closed without merging. -It is worth reopening as-is. +A reviewer approved the PR. The branch then became inactive, and the stale bot closed it. +You can open this PR again without changes. -This is the entry to read before reaching for a different fix to pytest teardown cost. -A related attempt in [#88759](https://github.com/PostHog/posthog/pull/88759) tried to reorder a GC freeze around the same cost, by deleting the `gc.unfreeze()` in `pytest_unconfigure`. -That unfreeze is not incidental. It was added in [#62707](https://github.com/PostHog/posthog/pull/62707) after the Temporal shards segfaulted with exit 139, and CI reproduced the same crash on #88759. -Frozen objects skip the final cyclic collections of `Py_FinalizeEx`, so their finalizers run in late teardown, after extension modules are gone. +Read this entry before you try a different solution for the pytest cleanup cost. +[#88759](https://github.com/PostHog/posthog/pull/88759) tried a different solution. It deleted the `gc.unfreeze()` in `pytest_unconfigure`. +That call is necessary. [#62707](https://github.com/PostHog/posthog/pull/62707) added it after the Temporal shards stopped with a segmentation fault and exit code 139. CI made the same crash again on #88759. +Frozen objects do not get the final cyclic collections of `Py_FinalizeEx`. Thus their finalizers run late in the teardown, after Python removes the extension modules. -_Also asked as:_ pytest teardown is slow, reduce gc.collect at session end, speed up pytest cleanup, why does the shard hang after tests pass +_Also asked as:_ pytest teardown is slow, reduce gc.collect at session end, speed up pytest cleanup, why does the shard hang after the tests pass ## Docker and image builds @@ -155,154 +157,154 @@ _Also asked as:_ pytest teardown is slow, reduce gc.collect at session end, spee **Verdict: rejected** · Oct 2025 · [#39700](https://github.com/PostHog/posthog/pull/39700) -Cache mounts were added for apt, pip, uv, node, and Playwright, following Depot's published guidance. +The change added cache mounts for apt, pip, uv, node, and Playwright. It followed the documented guidance from Depot. -Measured against master with a warm cache, it was slower. -Backend changes went from 52.7s to 57.5s, about 9% slower. Frontend changes went from 55.5s to 62.2s, about 12% slower. +A measurement against master with a warm cache showed a slower build. +A backend change increased from 52.7 to 57.5 seconds. This is approximately 9% slower. A frontend change increased from 55.5 to 62.2 seconds. This is approximately 12% slower. -The reason is workload shape. Cache mounts add 5 to 7 seconds of overhead even on a hit, and they only pay off when dependencies change. -About 95% of PRs change code, not dependencies. So the change taxed the common case to speed up the rare one. +The workload is the reason. A cache mount adds 5 to 7 seconds of overhead, even when the cache has the data. A cache mount gives a benefit only when the dependencies change. +Approximately 95% of PRs change code and do not change dependencies. Thus the change made the frequent case slower to make the rare case faster. _Also asked as:_ `--mount=type=cache`, speed up Docker builds, follow Depot cache best practices -### Move source COPY to the end of the Dockerfile +### Move the source COPY to the end of the Dockerfile **Verdict: rejected** · Oct 2025 · [#39695](https://github.com/PostHog/posthog/pull/39695) -It was already there. The Dockerfile's layer order was already correct, so the change was a no-op. +The source COPY was already at the end. The layer order in the Dockerfile was already correct. Thus the change did nothing. -Worth remembering as a class of idea: confirm the current state before optimizing it. +Remember this type of proposal. Examine the current state before you optimize it. _Also asked as:_ improve Docker layer caching, reorder Dockerfile layers -### Chase Docker Hub credentials when CI hits pull rate limits +### Examine the Docker Hub credentials when CI reports a pull rate limit -**Verdict: superseded by the real cause** · Aug 2026 · [#81963](https://github.com/PostHog/posthog/pull/81963) +**Verdict: the cause was different** · Aug 2026 · [#81963](https://github.com/PostHog/posthog/pull/81963) -CI failed with `toomanyrequests: You have reached your unauthenticated pull rate limit`. -At peak this hit roughly 45% of backend CI jobs, against a baseline of zero. +CI failed with this message: `toomanyrequests: You have reached your unauthenticated pull rate limit`. +At the maximum, this failure occurred in approximately 45% of the backend CI jobs. The usual rate is zero. -The message points at authentication, and that is what made it expensive. -`docker login` succeeded the whole time, and `Login Succeeded` was accurate. +The message indicates an authentication problem. This is why the diagnosis took a long time. +`docker login` was successful during all of this period, and the message `Login Succeeded` was correct. -The actual cause was a Docker Hub **billing lapse**. A lapsed plan removes entitlement, so Docker issues anonymous-class pull tokens while still accepting the login. +The true cause was a lapse in the Docker Hub subscription. A lapsed plan removes the entitlement. Docker then issues anonymous tokens for the pulls, but it continues to accept the login. -If this wording appears again, check the plan status before re-plumbing secrets. +If you see this message again, examine the subscription status before you change the secrets. _Also asked as:_ Docker Hub rate limit in CI, unauthenticated pull limit, DOCKERHUB secret is wrong ## CI orchestration -### Skip Storybook and E2E on bot snapshot-only commits +### Skip Storybook and E2E for snapshot-only commits from the bot **Verdict: reverted** · Mar 2026 · [#49997](https://github.com/PostHog/posthog/pull/49997), reverted by [#51212](https://github.com/PostHog/posthog/pull/51212) -Shipped, then pulled back a week later. +The change went to master. One week later, a revert removed it. _Also asked as:_ skip CI for snapshot commits, ignore bot commits in CI, don't rerun visual tests for the snapshot bot -### Force-cancel backend CI when pytest hangs on cancellation +### Force-cancel the backend CI run when pytest does not stop **Verdict: reverted** · Apr 2026 · [#54261](https://github.com/PostHog/posthog/pull/54261), reverted by [#54685](https://github.com/PostHog/posthog/pull/54685) -A watchdog job was added to force-cancel a run when pytest would not exit. -It lasted one day. +The change added a watchdog job. The job cancels a run when pytest does not exit. +The change stayed in master for one day. _Also asked as:_ cancel watchdog, kill hung CI jobs, pytest ignores SIGTERM -### Add a CI step that nudges humans to self-assign on bot PRs +### Add a CI step that asks a person to take a bot PR **Verdict: rejected** · Jun 2026 · [#62111](https://github.com/PostHog/posthog/pull/62111) -Bot-authored PRs cannot be auto-assigned, since the bot account is not a team member. -A CI step tried to nudge a human into claiming them. +CI cannot assign a PR from a bot automatically, because the bot account is not a member of the team. +The CI step asked a person to take the PR. -It was dropped for a lighter approach. A CI step has to guess who is behind a bot PR. -The agent opening the PR already knows, so the guidance moved into the PR template instead. +A different solution replaced it. A CI step must guess which person controls a bot PR. +The agent that opens the PR already knows this person. Thus the instruction moved to the PR template. _Also asked as:_ auto-assign bot PRs, find the human behind an agent PR, nudge for ownership ## Dev environment -### Run a dmypy daemon for fast local type checking +### Run a dmypy daemon for fast local type checks **Verdict: rejected** · Oct 2025 · [#39319](https://github.com/PostHog/posthog/pull/39319) -A pre-commit hook used the daemon when it was already running, giving roughly 0.6 to 1.7s checks once warm. +A pre-commit hook used the daemon if the daemon was already active. A warm daemon gives a check of approximately 0.6 to 1.7 seconds. -The warm-up never got cheaper, so the first check still paid full cost. -Starting the daemon from mprocs was tried too, and then commits hung while the daemon was still warming. +The warm-up time did not decrease. Thus the first check still has the full cost. +A start of the daemon from mprocs was also tested. The commits then stopped and waited, because the daemon was still warm. _Also asked as:_ dmypy, speed up mypy locally, type-check on commit, mypy daemon -### Run `uv sync` on every flox re-activate +### Run `uv sync` at each flox re-activation **Verdict: rejected** · Feb 2026 · [#49183](https://github.com/PostHog/posthog/pull/49183) -Shell profiles would sync dependencies on every shell start, about 660ms when already current. +The shell profiles synchronize the dependencies at each start of a shell. This needs approximately 660 ms when the dependencies are current. -It was dropped as trying too hard. Profiles run in subshells too, so the cost is paid far more often than the problem occurs. +The reason for the rejection is the frequency. The profiles also run in subshells. Thus the cost occurs much more frequently than the problem. _Also asked as:_ auto-sync deps, keep the venv current automatically, uv sync in the shell profile -### Upgrade Python past what flox's uv can install +### Upgrade Python to a version that the flox uv cannot install **Verdict: reverted** · Oct 2025 · [#40286](https://github.com/PostHog/posthog/pull/40286), reverted by [#40290](https://github.com/PostHog/posthog/pull/40290) -3.12.12 needs uv 0.9.2 or newer. Flox pinned uv 0.8.23, which could only fetch up to 3.12.10. -Anyone without 3.12.12 already installed could not build the environment. +Python 3.12.12 needs uv 0.9.2 or later. Flox pinned uv 0.8.23, and that version can get Python 3.12.10 at the maximum. +Thus a person without Python 3.12.12 on the local machine could not build the environment. -The constraint is the shape to remember, not the versions. The repo is now on 3.13.13, so this specific pin is long gone. -Before bumping Python, check what the flox-pinned uv can actually fetch. +Remember the constraint, not the versions. The repository now uses Python 3.13.13, and this pin is obsolete. +Before you increase the Python version, examine which versions the pinned flox uv can get. _Also asked as:_ bump Python, upgrade the interpreter, why is Python pinned to an exact version -### Replace Unit and Uvicorn with Granian everywhere at once +### Replace Unit and Uvicorn with Granian in one step **Verdict: superseded** · Oct 2025 · [#40450](https://github.com/PostHog/posthog/pull/40450), replaced by [#40847](https://github.com/PostHog/posthog/pull/40847) -The straight swap was closed for a dual-mode version that defaults to Unit and enables Granian behind `USE_GRANIAN=true`. -Same work, safer rollout. Granian is a dependency today. +A dual-mode PR replaced the direct exchange. The dual mode keeps Unit as the default and starts Granian when `USE_GRANIAN=true`. +The work is the same, and the rollout is safer. Granian is a dependency today. _Also asked as:_ migrate to Granian, replace Nginx Unit, unify the ASGI server -### Swap the object storage service from MinIO to SeaweedFS +### Change the object storage service from MinIO to SeaweedFS -**Verdict: eventually shipped, after several failed attempts** · first attempt Mar 2026 · [#49827](https://github.com/PostHog/posthog/pull/49827) +**Verdict: shipped, after several unsuccessful attempts** · first attempt Mar 2026 · [#49827](https://github.com/PostHog/posthog/pull/49827) -Worth knowing that earlier attempts by other people also failed before this one. -It has since landed. Both S3-compatible stores in the dev and CI stack are SeaweedFS now, and `AGENTS.md` treats new MinIO dependencies as off-limits. +Other people made earlier attempts, and those attempts also failed. +The change is complete now. The dev stack and CI use SeaweedFS for both S3-compatible stores. `AGENTS.md` prohibits new dependencies on MinIO. -Read this entry as evidence that a repeatedly failed migration can still be the right call. +This entry is evidence that a migration can be correct after several failures. _Also asked as:_ remove MinIO, SeaweedFS, replace the object storage container ## Product isolation -### Use `logs` as the first product to isolate behind a facade +### Use `logs` as the first product behind a facade **Verdict: superseded** · Jun 2026 · [#63184](https://github.com/PostHog/posthog/pull/63184) -`logs` was picked as the field test. Core imported its models, query runner, celery task, and temporal wiring, so it was a genuinely hard case. +`logs` was the first candidate for the field test. Core imports its models, its query runner, its celery task, and its temporal wiring. Thus `logs` is a difficult example. -The field test moved to `web_analytics` in [#63535](https://github.com/PostHog/posthog/pull/63535), and the tooling and skill were consolidated in [#63193](https://github.com/PostHog/posthog/pull/63193). -The note on closing was that `logs` can be re-cut from that tooling quickly if it is still wanted. -The doctrine that came out of this line of work landed later, in [#71486](https://github.com/PostHog/posthog/pull/71486). +The field test moved to `web_analytics` in [#63535](https://github.com/PostHog/posthog/pull/63535). [#63193](https://github.com/PostHog/posthog/pull/63193) collected the tools and the skill. +The note at the closure says that the tools can isolate `logs` quickly, if the team still wants this. +The doctrine from this work came later, in [#71486](https://github.com/PostHog/posthog/pull/71486). _Also asked as:_ which product should we isolate first, facade migration example, isolate logs -## Splitting work into PRs +## PR structure -### Split closely coupled layers into separate stacked PRs +### Put two closely coupled layers in two stacked PRs -**Verdict: rejected for this case** · Aug 2026 · [#87643](https://github.com/PostHog/posthog/pull/87643), folded into [#87644](https://github.com/PostHog/posthog/pull/87644) +**Verdict: rejected for this change** · Aug 2026 · [#87643](https://github.com/PostHog/posthog/pull/87643), merged into [#87644](https://github.com/PostHog/posthog/pull/87644) -Two layers of a change read well as two stories but not as two diffs. -Both rewrote the same four modules, sometimes the same lines. -The upper layer deleted a block the lower layer was fixing, and re-signatured a function the lower layer was splitting. +The two layers read well as two stories. They did not work as two diffs. +Both layers changed the same four modules, and sometimes the same lines. +The upper layer deleted a block that the lower layer corrected. It also changed the signature of a function that the lower layer divided. -Every fix had to be replayed through those collisions, and the replay repeats on each restack. +Each correction in the lower layer needed a repeat through these collisions. The repeat occurs again at each restack. -Split by diff surface, not by narrative. If two layers touch the same lines, one PR is cheaper to review than two. +Divide a change by its diff surface, not by its story. If two layers touch the same lines, one PR needs less review effort than two. _Also asked as:_ should I stack these, split this PR, break the change into reviewable layers From 823432b5a8802891233bedbff1e44505b6be9b32 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 27 Aug 2026 10:52:35 +0200 Subject: [PATCH 06/24] chore(devex): cover django perf and the runner trial in the prior-art doc Paul's original ask in #team-devex named three areas: CI, django perf, and tests. The first pass covered CI and tests only. Adds a Django performance section that points at django-startup-time.md as the deep source, plus the three proposals that keep coming back: squashing the migration history (#48267, effect on timing too small for the blocker work), pydantic defer_build (reverted, relocated cost onto the first query after a deploy), and dropping pydantic from the generated schema (250+ model_validate callers make it non-local; the import cost was solved by evicting the module from django.setup instead). Also adds the Blacksmith runner trial (#54559 to #57991). Nothing in the repo recorded why the shadow existed or why it went away, and compare-ci-runners.py is still sitting there marked legacy. Vendor benchmark numbers stay out. They are unpublished internal measurements of a named third party, and the trial's own conclusion was that cache-state asymmetry, not runner speed, drove the large gaps. --- docs/internal/ci-things-already-tried.md | 57 ++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/docs/internal/ci-things-already-tried.md b/docs/internal/ci-things-already-tried.md index 6e23bc116aa9..80342eddce62 100644 --- a/docs/internal/ci-things-already-tried.md +++ b/docs/internal/ci-things-already-tried.md @@ -195,6 +195,21 @@ _Also asked as:_ Docker Hub rate limit in CI, unauthenticated pull limit, DOCKER ## CI orchestration +### Move CI from the Depot runners to Blacksmith + +**Verdict: rejected** · Apr 2026 to May 2026 · [#54559](https://github.com/PostHog/posthog/pull/54559), removed by [#57991](https://github.com/PostHog/posthog/pull/57991) + +The trial did not do a direct exchange. It ran a Blacksmith shadow of most compute jobs on the same commit, behind the `BLACKSMITH_SHADOW_ENABLED` variable. Each shadow used `continue-on-error`, and no shadow was a required check. + +The comparison was not conclusive. Some jobs looked much slower on the shadow runner. The step-level data showed a difference in the cache state between the two providers, not a difference in compute speed. The trial also ran for approximately 16 hours, which is too short for the jobs whose medians were close. + +The team kept Depot. [#57991](https://github.com/PostHog/posthog/pull/57991) removed the shadow workflow and the matrix branches. +It kept `.github/scripts/compare-ci-runners.py` and marked the file as legacy. That script produced the numbers of the trial. + +If you propose this again, equalize the caches first. Otherwise the next trial measures the cache plumbing again, not the runners. + +_Also asked as:_ change CI provider, Blacksmith, cheaper runners, are the Depot runners slow + ### Skip Storybook and E2E for snapshot-only commits from the bot **Verdict: reverted** · Mar 2026 · [#49997](https://github.com/PostHog/posthog/pull/49997), reverted by [#51212](https://github.com/PostHog/posthog/pull/51212) @@ -279,6 +294,48 @@ This entry is evidence that a migration can be correct after several failures. _Also asked as:_ remove MinIO, SeaweedFS, replace the object storage container +## Django performance + +[docs/internal/django-startup-time.md](django-startup-time.md) is the deep source for this area. It has a Traps section that records the failure modes of each mechanism. +The entries below give the proposals that people repeat. + +### Squash the Django migration history + +**Verdict: rejected** · Feb 2026 to Mar 2026 · [#48267](https://github.com/PostHog/posthog/pull/48267) + +The PR added a squash planner, a policy for opaque operations, and 65 squashed migrations across the historical range. A zero-to-head migration on a fresh database was successful, and the schema comparison found no structural difference. + +The problem is the value. The PR reports that the effect on the timing was small and noisy. The work to resolve each blocker is large, and the reviews are difficult. + +The migration replay in CI is a real cost. A different change must decrease it. + +_Also asked as:_ squash the migrations, compress the migration history, why are there so many migrations, speed up the migration replay + +### Build the generated pydantic schema lazily with `defer_build` + +**Verdict: reverted** · [docs/internal/django-startup-time.md](django-startup-time.md) + +This removed approximately 400 ms of core-schema construction from each `django.setup()`. The round-trip tests were all successful. + +Two problems stopped it. First, the deferred builds move to the first `/query` of each web worker after a deploy. A warm-up loop for those builds measured approximately 2.5 times more expensive than the eager construction. +Second, the query runners construct the response models directly. This does no validation, so it does not start the lazy build. `model_dump()` then sends a mock serializer into pydantic-core and raises `TypeError: 'MockValSer' object cannot be converted to 'SchemaSerializer'`. This is a 500 error in any process. + +A different solution removed the cost. `django.setup()` no longer imports `posthog.schema` at all. + +_Also asked as:_ `defer_build`, make the schema import lazy, pydantic model build is slow at startup + +### Replace pydantic in the generated schema with plain dataclasses + +**Verdict: not viable today** + +`posthog/schema.py` has more than 1,000 generated classes. `hogli build:schema` generates the file from the TypeScript types with pydantic tooling. +More than 250 files call `model_validate`, and the API layer depends on this validation. Thus a change of the model library is not a local change. + +The import cost is solved. `posthog.schema` costs approximately 2 seconds to import, but `django.setup()` no longer loads it. The enums also moved to `posthog.schema_enums`, which imports in approximately 20 ms. +Import a model from `posthog.schema` inside the method that uses it. Take the enums from `posthog.schema_enums`. + +_Also asked as:_ remove pydantic, use dataclasses for the schema, the schema import is slow, why is `posthog.schema` so big + ## Product isolation ### Use `logs` as the first product behind a facade From a0454fe1790adc1e6a475c72ba21725b8f437126 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 27 Aug 2026 10:54:17 +0200 Subject: [PATCH 07/24] chore(devex): record that snob test selection is live An agent recently read CI, found no pytest-snob in pyproject.toml, and concluded the selector was dead. It isn't: pytest-snob is an inline PEP 723 dependency of tools/snob_backend_test_selection_shadow.py, so it never appears in the dependency file. The narrow scope added to the confusion. Selection ran on drafts only for a while, deliberately, until the merge queue was stable. #85530 extended it to ready PRs and #88265 folded Django and product selection into one job. This is the exact question the doc exists to answer, so it gets an entry rather than a correction in a thread. --- docs/internal/ci-things-already-tried.md | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/docs/internal/ci-things-already-tried.md b/docs/internal/ci-things-already-tried.md index 80342eddce62..29e10defeab5 100644 --- a/docs/internal/ci-things-already-tried.md +++ b/docs/internal/ci-things-already-tried.md @@ -127,10 +127,26 @@ Two results have more importance than these numbers. First, there were no stale tests with high confidence. Thus a cleanup from this data had nothing to delete. Second, 1,020 tests appeared to touch no production code. Almost all of these results are false. They come from code with many mocks, from property-based tests, and from tests of migration rules. Testmon cannot trace these paths. -Backend test selection came later from a different mechanism. Read [#85530](https://github.com/PostHog/posthog/pull/85530) and [#88265](https://github.com/PostHog/posthog/pull/88265). +Backend test selection came later from a different mechanism. It uses the Snob import graph. Read the next entry. _Also asked as:_ coverage-based test selection, testmon, find stale tests from coverage, only run affected tests +### Snob is in CI, but CI does not use it + +**Verdict: CI does use it.** The scope was narrow on purpose. + +`tools/snob_backend_test_selection_shadow.py` selects the Django test subset for a PR. It combines the Snob import graph with Django-aware heuristics. + +Two things make this look inactive: + +`pytest-snob` is an inline PEP 723 dependency of that script. It is not in `pyproject.toml`. Thus a search of the dependency file finds nothing. +The selection was also active for draft PRs only for some time. The team wanted a stable merge queue first. + +[#85530](https://github.com/PostHog/posthog/pull/85530) then extended the selection to PRs that are ready for review. [#88265](https://github.com/PostHog/posthog/pull/88265) put the Django selection and the product selection in one job. +Read the comment at the top of `.github/workflows/ci-backend.yml` for the current rules. + +_Also asked as:_ snob, is test selection on, why does CI run all the tests, do we select tests on PRs + ### Disable the pytest `unraisableexception` and `threadexception` plugins **Verdict: open, and approved** · Jul 2026 · [#70886](https://github.com/PostHog/posthog/pull/70886) From 36cdae9989f07f2a4c0a25f09faad89720b83705 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 27 Aug 2026 11:00:31 +0200 Subject: [PATCH 08/24] fix(devex): size product shards against the right headroom PRODUCT_SAFETY_FACTOR was a single 1.3 covering two unrelated risks. On a packed bucket the products run sequentially, so the job's wall is the sum of its parts and there is no mean-versus-max gap to cover. The 30% markup there only bought extra buckets, each paying the base overhead again. On a split product the factor is load-bearing: sizing solves for the mean shard while the run's wall is the max, and nothing else bridges that. Split it in two. Buckets get 1.1, covering durations-map error alone. Splits get a margin interpolated from the measured max/mean at p90, which rises with the shard count because the max is an order statistic over more shards. Resolving that needs a fixed point, seeded at no margin so a product that fits one shard is not split in two by its own headroom. Product test jobs now carry legs, one turbo invocation each with its own pytest-split flags, so a split product's last shard can share a job with whole products without --splits/--group leaking onto them. The packer can seed from those remainders. It does not fire yet: with the margin reserved on every shard there is almost no modeled spare, and the real spare, a trailing-shard taper, is not something the sizer can see today. Landing the mechanism now keeps it inert until the margin drops. DEDICATED_BUCKET_PRODUCTS drops batch-exports. It was listed for an async-fixture teardown hang; it now runs at the median product failure rate with no job near the timeout, and the branch was unreachable regardless since the split path is checked first. The set stays as the extension point, with the criterion for re-adding written down. Also corrects the end-to-end claim in the sizing comment. The pre-shard preamble measures about 5.5 minutes, so a 12-minute shard target puts a full PR run near 19 minutes, not 15. Claude-Session: https://claude.ai/code/session_01EJ1y3UzXR1BKtukE886ks9 --- .github/scripts/turbo-discover-sizing.test.js | 55 +++++- .github/scripts/turbo-discover.js | 167 ++++++++++++++---- .github/workflows/ci-backend.yml | 33 +++- 3 files changed, 203 insertions(+), 52 deletions(-) diff --git a/.github/scripts/turbo-discover-sizing.test.js b/.github/scripts/turbo-discover-sizing.test.js index 557ee10e8cf8..efb995549ce8 100644 --- a/.github/scripts/turbo-discover-sizing.test.js +++ b/.github/scripts/turbo-discover-sizing.test.js @@ -7,7 +7,7 @@ const test = require('node:test') const assert = require('node:assert/strict') -const { pruneDeadDurations, getSegmentDuration, calculateShards, resolveProductSizing, buildMatrix, PRODUCT_JOB_OVERHEAD_SECONDS, PRODUCT_SAFETY_FACTOR, TARGET_WALL_SECONDS } = require('./turbo-discover.js') +const { pruneDeadDurations, getSegmentDuration, calculateShards, resolveProductSizing, buildMatrix, productSplitShards, splitImbalanceFactor, PRODUCT_JOB_OVERHEAD_SECONDS, TARGET_WALL_SECONDS } = require('./turbo-discover.js') // A path that exists in every checkout, so the existence check is deterministic. const LIVE_FILE = '.github/scripts/turbo-discover.js' @@ -95,8 +95,8 @@ test('buildMatrix splits a product to the shared wall target', () => { const matrix = buildMatrix(['big-one'], union, true) - // 2000s of work, with the safety factor, over a (target - overhead) budget per shard. - assert.equal(matrix.length, Math.ceil((2000 * PRODUCT_SAFETY_FACTOR) / (TARGET_WALL_SECONDS - O))) + // 2000s of work, with the imbalance margin, over a (target - overhead) budget. + assert.equal(matrix.length, productSplitShards(2000)) assert.match(matrix[0].group, /^big-one \(1\/\d+\)$/) }) @@ -107,5 +107,52 @@ test('buildMatrix leaves a small product packed', () => { assert.equal(matrix.length, 1) assert.equal(matrix[0].group, 'small-one') - assert.equal(matrix[0].pytest_args, '') + assert.deepEqual(matrix[0].legs, [{ filters: '--filter=@posthog/products-small-one', pytest_args: '' }]) +}) + +test('splitImbalanceFactor rises with the split count and never marks up a single shard', () => { + assert.equal(splitImbalanceFactor(1), 1) + assert.ok(splitImbalanceFactor(2) < splitImbalanceFactor(6)) + assert.ok(splitImbalanceFactor(6) < splitImbalanceFactor(9)) + // Clamped, not extrapolated, past the measured range. + assert.equal(splitImbalanceFactor(40), splitImbalanceFactor(9)) +}) + +test('a product that fits one shard is packed, not split by its own margin', () => { + // 300s of work sits under the (target - overhead) budget, so the margin must + // not be what pushes it over into a two-way split. + const union = {} + for (let i = 0; i < 10; i++) { + union[`products/mid_one/backend/test_${i}.py::test_${i}`] = 30 + } + + assert.ok(300 <= TARGET_WALL_SECONDS - PRODUCT_JOB_OVERHEAD_SECONDS) + assert.equal(productSplitShards(300), 1) + + const matrix = buildMatrix(['mid-one'], union, true) + + assert.equal(matrix.length, 1) + assert.equal(matrix[0].group, 'mid-one') +}) + +test("a split product's last shard absorbs a small product without leaking split flags", () => { + const union = {} + for (let i = 0; i < 11; i++) { + union[`products/big_one/backend/test_${i}.py::test_${i}`] = 30 + } + union['products/small_one/backend/test_s.py::test_s'] = 60 + + assert.equal(productSplitShards(330), 2) + + const matrix = buildMatrix(['big-one', 'small-one'], union, true) + + // Two jobs, not three: small-one rides along in the lighter second shard. + assert.equal(matrix.length, 2) + const shared = matrix.find((entry) => entry.group.includes('small-one')) + assert.equal(shared.group, 'big-one (2/2), small-one') + assert.equal(shared.legs.length, 2) + assert.match(shared.legs[0].pytest_args, /--splits 2 --group 2/) + // The whole product runs in its own leg, so it never sees --splits/--group. + assert.equal(shared.legs[1].filters, '--filter=@posthog/products-small-one') + assert.equal(shared.legs[1].pytest_args, '') }) diff --git a/.github/scripts/turbo-discover.js b/.github/scripts/turbo-discover.js index 82acf41bbabd..301026b645d7 100644 --- a/.github/scripts/turbo-discover.js +++ b/.github/scripts/turbo-discover.js @@ -40,26 +40,45 @@ const { analyzeSchemaImpact, readBaseSchema } = require('./schema-impact') const { loadContractSurfaces } = require('./trunk-impacted-targets') // --- Product shard sizing (same Amdahl shape as Django below) --- -// Each product is atomic for packing, but unlike Django the test pool isn't -// fungible across products — bin-pack products into target-sized shards, and -// multi-shard split any single product that overflows on its own. +// The test pool is not fungible across products, so a product is the unit of +// work: bin-pack products into target-sized jobs, and multi-shard split any +// single product that overflows on its own. A job runs what it holds +// sequentially, so its wall is the sum of its parts, not the max. // One flat wall-clock target for every test shard, Django and products alike. // Predictability is the point: a dev who kicks off CI knows what a shard costs // without knowing which segment it is. Sizing solves wall = overhead + work/n // for n, so the target is a promise about the PR lane (where the overheads below // are fitted); master pays extra overhead (full migration replay) on top. -// A full run's wall is discovery plus the slowest of its shards, and with many -// shards packed to one target the slowest lands a few minutes above it, so a -// 12-minute shard target puts a full PR run near 15 minutes end to end. +// A full run's wall is the pre-shard preamble plus the slowest of its shards. +// The preamble (discovery, matrix build, runner start) measures ~5.5 min and the +// slowest shard lands above the target by the imbalance margin below, so a +// 12-minute shard target puts a full PR run near 19 minutes end to end. const TARGET_WALL_SECONDS = 12 * 60 // Per-product cost within a runner: turbo dispatch, pytest collection, Django // init. First product pays ~45s, subsequent ~15s; use 60s as a conservative // average that also absorbs the amortized portion of runner startup. const PRODUCT_PER_PRODUCT_OVERHEAD_SECONDS = 60 -// Headroom for run-to-run variance when deciding how much fits in a bucket. Was -// 2x originally because pytest-split data was noisy under Django Core's shared -// session; the outlier-based merge produces cleaner numbers now. -const PRODUCT_SAFETY_FACTOR = 1.3 +// Headroom on a packed bucket. A bucket runs its products sequentially, so its +// wall is the sum of its parts — there is no mean-versus-max gap to cover here, +// and this only absorbs error in the recorded durations. Was one shared 1.3 with +// the split factor below, which marked up every small product by 30% against a +// budget of TARGET minus PRODUCT_JOB_BASE_OVERHEAD_SECONDS and bought extra +// buckets, each paying that base overhead again. +const PRODUCT_BUCKET_SAFETY_FACTOR = 1.1 +// Headroom on a split product. Sizing solves for the MEAN shard, but a run's +// wall is the MAX shard, and this is the only thing bridging the two. The gap +// grows with the split count: the max is an order statistic over more shards, +// and the product's own intra-split skew rides on top. Measured max/mean at p90 +// over PR runs; interpolated between the points and clamped outside them. +// A per-product ratio read back from JUnit would beat a table keyed on n alone, +// because the skew is a property of the suite. That needs per-test wall times, +// which call-only durations cannot give (see PRODUCTS_SCALED_MARKER). +const SPLIT_IMBALANCE_BY_SHARDS = [ + [2, 1.12], + [4, 1.2], + [6, 1.23], + [9, 1.32], +] // Fitted per-shard overhead for a split product job. Two measured parts, from // run 32717208712: the job base (docker stack, deps, turbo dispatch) is // mean(job wall - JUnit suite time), 247-413s across 12 bucket jobs (median @@ -96,10 +115,13 @@ const PRODUCTS_RUNNING_TEMPORAL_IN_JOB = new Set([ 'tasks', 'warehouse-sources', ]) -// Products that always get their own matrix entry instead of being packed with -// others — isolates a flaky/hang-prone product so it can't cancel bucket-mates -// at the job timeout. Trade-off: a dedicated runner. -const DEDICATED_BUCKET_PRODUCTS = new Set(['batch-exports']) +// Products that always get their own matrix entry instead of sharing one — +// isolates a flaky/hang-prone product so it can't cancel job-mates at the job +// timeout. Trade-off: a dedicated runner. Empty today: batch-exports was listed +// for an async-fixture teardown hang, and it now runs at the median product +// failure rate with no job near the timeout. Add a product here when its wall +// runs close enough to the job timeout that a hang is a realistic outcome. +const DEDICATED_BUCKET_PRODUCTS = new Set() // --- Staleness detection for .test_durations --- // When a product's test files on disk significantly outnumber what .test_durations @@ -723,23 +745,27 @@ function resolveProductSizing(product, durations, productsScaled = false) { function productEffectiveCost(product, durations, productsScaled = false) { const { work } = resolveProductSizing(product, durations, productsScaled) - return work * PRODUCT_SAFETY_FACTOR + PRODUCT_PER_PRODUCT_OVERHEAD_SECONDS + return work * PRODUCT_BUCKET_SAFETY_FACTOR + PRODUCT_PER_PRODUCT_OVERHEAD_SECONDS } -// First-fit-decreasing bin packing into TARGET-sized shards. Sorts products by +// First-fit-decreasing bin packing into TARGET-sized jobs. Sorts products by // effective cost descending so the largest products land first and small ones -// fill the gaps. Each bucket caps at the wall target minus the base overhead the -// job pays once, so the effective costs only compete for the remaining budget. -function packProducts(products, durations, productsScaled = false) { +// fill the gaps. Each job caps at the wall target minus the base overhead it +// pays once, so the effective costs only compete for the remaining budget. +// `seedJobs` are jobs that already hold work — a split product's last shard — +// and they sit first so their leftover budget is used before a new runner is +// started. A seed carries its own base overhead, which is a large product's +// session cost rather than the packed-bucket base. +function packProducts(products, durations, productsScaled = false, seedJobs = []) { const items = products .map((product) => ({ product, cost: productEffectiveCost(product, durations, productsScaled) })) .sort((a, b) => b.cost - a.cost) - const buckets = [] + const buckets = [...seedJobs] for (const { product, cost } of items) { let placed = false for (const bucket of buckets) { - if (bucket.cost + cost <= TARGET_WALL_SECONDS - PRODUCT_JOB_BASE_OVERHEAD_SECONDS) { + if (bucket.cost + cost <= TARGET_WALL_SECONDS - bucket.baseOverhead) { bucket.products.push(product) bucket.cost += cost placed = true @@ -747,7 +773,13 @@ function packProducts(products, durations, productsScaled = false) { } } if (!placed) { - buckets.push({ products: [product], cost }) + buckets.push({ + label: null, + legs: [], + products: [product], + cost, + baseOverhead: PRODUCT_JOB_BASE_OVERHEAD_SECONDS, + }) } } return buckets @@ -833,6 +865,44 @@ function calculateShards(totalWorkSeconds, overheadSeconds, minShards = DJANGO_M return Math.max(minShards, Math.min(DJANGO_MAX_SHARDS, shards)) } +// Interpolates SPLIT_IMBALANCE_BY_SHARDS, clamped at both ends. One shard is not +// a split, so it carries no imbalance. +function splitImbalanceFactor(shards) { + if (shards <= 1) { + return 1 + } + const first = SPLIT_IMBALANCE_BY_SHARDS[0] + const last = SPLIT_IMBALANCE_BY_SHARDS[SPLIT_IMBALANCE_BY_SHARDS.length - 1] + if (shards <= first[0]) { + return first[1] + } + for (let i = 1; i < SPLIT_IMBALANCE_BY_SHARDS.length; i++) { + const [prevShards, prevFactor] = SPLIT_IMBALANCE_BY_SHARDS[i - 1] + const [nextShards, nextFactor] = SPLIT_IMBALANCE_BY_SHARDS[i] + if (shards <= nextShards) { + const span = (shards - prevShards) / (nextShards - prevShards) + return prevFactor + (nextFactor - prevFactor) * span + } + } + return last[1] +} + +// The imbalance margin depends on the shard count, which depends on the margin. +// Iterate to the fixed point, seeding at no margin so a product that fits whole +// is never split into two by its own headroom. Both sides only rise, so the +// sequence is monotone and settles in a pass or two. +function productSplitShards(workSeconds) { + let shards = calculateShards(workSeconds, PRODUCT_JOB_OVERHEAD_SECONDS, 1) + for (let pass = 0; pass < 3; pass++) { + const next = calculateShards(workSeconds * splitImbalanceFactor(shards), PRODUCT_JOB_OVERHEAD_SECONDS, 1) + if (next === shards) { + break + } + shards = next + } + return shards +} + // Selector segment key -> Django matrix segment name. const MATRIX_NAME_BY_SEGMENT = { core: 'Core', poe: 'CorePOE', temporal: 'Temporal' } @@ -989,6 +1059,7 @@ function buildDjangoShards(durations, ranNodeIds = {}) { function buildMatrix(products, durations, productsScaled = false) { const matrix = [] const packable = [] + const fillableJobs = [] // Split a product across multiple shards with the same rule Django uses: // enough shards that each lands at the shared wall target. The safety @@ -1008,7 +1079,7 @@ function buildMatrix(products, durations, productsScaled = false) { ) } - const shards = calculateShards(work * PRODUCT_SAFETY_FACTOR, PRODUCT_JOB_OVERHEAD_SECONDS, 1) + const shards = productSplitShards(work) if (shards > 1) { console.error(` ${product}: ${(work / 60).toFixed(1)} min work → split across ${shards} shards`) const filters = `--filter=@posthog/products-${product}` @@ -1017,34 +1088,49 @@ function buildMatrix(products, durations, productsScaled = false) { // optimally. The greedy rule in duration_based_chunks lets every shard // overrun the per-shard average, which on skewed suites starves trailing // shards down to zero tests (pytest exit 5, "no tests collected"). + const shardCost = (work * splitImbalanceFactor(shards)) / shards for (let i = 1; i <= shards; i++) { - matrix.push({ - group: `${product} (${i}/${shards})`, + const leg = { filters, pytest_args: `-- --splits ${shards} --group ${i} --splitting-algorithm optimal_chunks`, - }) + } + // ceil() rounds the split short and optimal_chunks cuts in order, so + // the last shard is reliably the lightest. Offer its leftover budget + // to the packer rather than starting another runner for that work. + if (i === shards && !DEDICATED_BUCKET_PRODUCTS.has(product)) { + fillableJobs.push({ + label: `${product} (${i}/${shards})`, + legs: [leg], + products: [], + cost: shardCost, + baseOverhead: PRODUCT_JOB_OVERHEAD_SECONDS, + }) + } else { + matrix.push({ group: `${product} (${i}/${shards})`, legs: [leg] }) + } } } else if (DEDICATED_BUCKET_PRODUCTS.has(product)) { - console.error(` ${product}: ${(work / 60).toFixed(1)} min work → dedicated bucket (never packed)`) + console.error(` ${product}: ${(work / 60).toFixed(1)} min work → dedicated job (never shared)`) matrix.push({ group: product, - filters: `--filter=@posthog/products-${product}`, - pytest_args: '', + legs: [{ filters: `--filter=@posthog/products-${product}`, pytest_args: '' }], }) } else { packable.push(product) } } - for (const bucket of packProducts(packable, durations, productsScaled)) { - console.error( - ` bucket (${(bucket.cost / 60).toFixed(1)} min effective): ${bucket.products.join(', ')}` - ) - matrix.push({ - group: bucket.products.join(', '), - filters: bucket.products.map((p) => `--filter=@posthog/products-${p}`).join(' '), - pytest_args: '', - }) + for (const bucket of packProducts(packable, durations, productsScaled, fillableJobs)) { + const group = [bucket.label, ...bucket.products].filter(Boolean).join(', ') + console.error(` job (${(bucket.cost / 60).toFixed(1)} min effective): ${group}`) + const legs = [...bucket.legs] + if (bucket.products.length > 0) { + legs.push({ + filters: bucket.products.map((p) => `--filter=@posthog/products-${p}`).join(' '), + pytest_args: '', + }) + } + matrix.push({ group, legs }) } return matrix @@ -1062,7 +1148,10 @@ module.exports = { resolveProductSizing, buildMatrix, PRODUCT_JOB_OVERHEAD_SECONDS, - PRODUCT_SAFETY_FACTOR, + PRODUCT_BUCKET_SAFETY_FACTOR, + SPLIT_IMBALANCE_BY_SHARDS, + splitImbalanceFactor, + productSplitShards, PRODUCTS_SCALED_MARKER, TARGET_WALL_SECONDS, DJANGO_OVERHEAD_SECONDS_BY_SEGMENT, diff --git a/.github/workflows/ci-backend.yml b/.github/workflows/ci-backend.yml index debe189fa398..f87e7cd29623 100644 --- a/.github/workflows/ci-backend.yml +++ b/.github/workflows/ci-backend.yml @@ -965,8 +965,12 @@ jobs: continue-on-error: true # --force: discover already decided this product needs testing, skip turbo cache # --log-order=stream: stream pytest output live instead of buffering until completion - # pytest_args: optional pytest-split flags for sharded products (e.g. "-- --splits 3 --group 1") + # legs: one turbo invocation per entry, each with its own pytest-split flags + # (e.g. "-- --splits 3 --group 1"). A job holds several legs when a split + # product's last shard had budget left over for whole small products, and + # pytest-split flags must not leak from that shard onto its job-mates. env: + PRODUCT_LEGS: ${{ toJSON(matrix.legs) }} CLICKHOUSE_HOGQL_USE_NEW_EVENTS_SCHEMA: ${{ matrix.new-events-schema && 'true' || 'false' }} # products/tasks/backend/temporal moved here from the Django Temporal # segment, and its conftest and workflows talk to Modal. Injected only @@ -993,17 +997,28 @@ jobs: # bin-packed buckets (each product writes its own file) and split products (unioned later). # sysmon: Python 3.12+'s low-overhead coverage backend, ~a few % vs ~20% for the C tracer. COVERAGE_CORE: sysmon + shell: bash run: | set +e - pnpm turbo run backend:test ${{ matrix.filters }} --concurrency=1 --output-logs=full --force --log-order=stream ${{ matrix.pytest_args }} - exit_code=$? + overall=0 + leg_count=$(jq 'length' <<< "$PRODUCT_LEGS") + for ((leg_index = 0; leg_index < leg_count; leg_index++)); do + leg_filters=$(jq -r ".[$leg_index].filters" <<< "$PRODUCT_LEGS") + leg_args=$(jq -r ".[$leg_index].pytest_args" <<< "$PRODUCT_LEGS") + echo "::group::turbo backend:test $leg_filters $leg_args" + # Word splitting is intended here: both variables carry several flags. + # shellcheck disable=SC2086 + pnpm turbo run backend:test $leg_filters --concurrency=1 --output-logs=full --force --log-order=stream $leg_args + exit_code=$? + echo "::endgroup::" + if [ $exit_code -eq 5 ]; then + echo "No tests collected for this leg, this is expected when splitting tests" + elif [ $exit_code -ne 0 ]; then + overall=$exit_code + fi + done set -e - if [ $exit_code -eq 5 ]; then - echo "No tests collected for this shard, this is expected when splitting tests" - exit 0 - else - exit $exit_code - fi + exit $overall # Best-effort Trunk upload (continue-on-error); the "Fail on test failure" step below is # the verdict, so a Trunk outage can't red a passing shard. Internal PRs only (needs the From 821a81c5bcabd2a70a99e551618ed9974cb0d8a9 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 27 Aug 2026 11:04:29 +0200 Subject: [PATCH 09/24] chore(devex): add the closed-unmerged PRs the first pass skipped The first pass filtered closed PRs to those with a closing comment over 60 characters, which covered 34 of 125. That threw away the stale-bot closures, where the body still records exactly what was tried and why it stopped. Adds the ideas that get re-proposed most: Tilt over mprocs, sharing the dev environment across worktrees (three separate attempts), sparse-checkout on the big CI workflows, moving ee/ into products/, session-scoped django_db_setup, and six unmerged attempts at validating responses against the OpenAPI schema. Two existing entries were wrong by omission and are corrected. BuildKit cache mounts were rejected in bulk, then adopted narrowly for uv, which then cached wheels built against a different libxmlsec1 and broke the image; the fix that stuck puts the library version in the cache ID. Migration squashing had a second attempt on a different design, and that one also stalled. Both corrections came from checking present-day state rather than trusting the verdict in the PR, which is the habit the doc asks readers to have. --- docs/internal/ci-things-already-tried.md | 111 ++++++++++++++++++++++- 1 file changed, 108 insertions(+), 3 deletions(-) diff --git a/docs/internal/ci-things-already-tried.md b/docs/internal/ci-things-already-tried.md index 29e10defeab5..21fe6365b839 100644 --- a/docs/internal/ci-things-already-tried.md +++ b/docs/internal/ci-things-already-tried.md @@ -56,6 +56,19 @@ A factor of 2.5 in compute is too much for 3 minutes of wall time. _Also asked as:_ parallelize tests within a shard, `-n auto`, use the idle cores on the runner, why is each shard single-process +### Change the `django_db_setup` fixture from package scope to session scope + +**Verdict: rejected** · Apr 2026 · [#57030](https://github.com/PostHog/posthog/pull/57030) + +Package scope builds the test database one time for each package directory. Session scope builds it one time for the whole run, which looks strictly faster. + +The PR did not merge. `posthog/conftest.py` still declares `@pytest.fixture(scope="package")`. +Read [#57227](https://github.com/PostHog/posthog/pull/57227) with this one. It makes the cost of `django_db_setup` visible in the pytest output. Today that cost hides in the setup phase of the first test that pytest collects, and makes that test look slow for no reason. + +Measure the setup cost first. Then you know what a scope change can win. + +_Also asked as:_ session-scoped database fixture, build the test database once, why is the first test so slow + ### Shard the Playwright E2E suite **Verdict: reverted** · Feb 2026 · [#46774](https://github.com/PostHog/posthog/pull/46774), reverted by [#46853](https://github.com/PostHog/posthog/pull/46853) @@ -181,7 +194,14 @@ A backend change increased from 52.7 to 57.5 seconds. This is approximately 9% s The workload is the reason. A cache mount adds 5 to 7 seconds of overhead, even when the cache has the data. A cache mount gives a benefit only when the dependencies change. Approximately 95% of PRs change code and do not change dependencies. Thus the change made the frequent case slower to make the rare case faster. -_Also asked as:_ `--mount=type=cache`, speed up Docker builds, follow Depot cache best practices +A narrow version came later and stayed. [#42124](https://github.com/PostHog/posthog/pull/42124) added a uv cache mount, and the Dockerfile also has pnpm and npm mounts today. +That narrow version then caused its own failure. The uv cache kept wheels that were compiled against a different `libxmlsec1` version, and the build failed with a version mismatch. +[#43066](https://github.com/PostHog/posthog/pull/43066) proposed to remove the mount again. [#43091](https://github.com/PostHog/posthog/pull/43091) gave the better fix: it puts the `libxmlsec1` version in the cache ID, so a change of the system library invalidates the cache. +Read the `id=uv-libxmlsec1...` mount in the Dockerfile. + +The rule: add a cache mount for one expensive step that you measured. Do not add cache mounts everywhere. Put the version of any system library that the cached artifacts compile against in the cache ID. + +_Also asked as:_ `--mount=type=cache`, speed up Docker builds, follow Depot cache best practices, xmlsec version mismatch in the image build ### Move the source COPY to the end of the Dockerfile @@ -226,6 +246,32 @@ If you propose this again, equalize the caches first. Otherwise the next trial m _Also asked as:_ change CI provider, Blacksmith, cheaper runners, are the Depot runners slow +### Use sparse-checkout on the large CI workflows + +**Verdict: rejected for those workflows** · Oct 2025 · [#39239](https://github.com/PostHog/posthog/pull/39239) + +The proposal added sparse-checkout to the backend, frontend, and Rust workflows. Each job would exclude the directories that it does not use. + +The PR did not merge. Sparse-checkout is still correct for small jobs, and `ci-storybook.yml`, `ci-security.yaml`, and `pr-resolve-outdated-bot-comments.yml` use it today. +The large test workflows do not. A test job reads more of the tree than the exclusion list expects, and the migration jobs change refs. + +If you propose this again, name the jobs and prove that each one reads only the included paths. + +_Also asked as:_ sparse-checkout, partial clone, do not check out the whole repo, speed up the checkout step + +### Jest reports the Rust snapshots as obsolete + +**Verdict: superseded** · Jan 2026 · [#46008](https://github.com/PostHog/posthog/pull/46008) + +Jest found the `.snap` files under `rust/cymbal/tests/snapshots/` during the Storybook visual regression job. It marked them as obsolete, and all 19 jobs failed. + +The PR records the attempts that did not work. `modulePathIgnorePatterns` changes only the module resolution. It does not change which snapshot files Jest finds. +The PR proposed `haste.blockList`. The repository does not use that option today, so a different change solved this. + +Keep the record: the snapshot scan and the module resolution use different configuration. + +_Also asked as:_ obsolete snapshots in CI, Jest finds rust snapshots, modulePathIgnorePatterns + ### Skip Storybook and E2E for snapshot-only commits from the bot **Verdict: reverted** · Mar 2026 · [#49997](https://github.com/PostHog/posthog/pull/49997), reverted by [#51212](https://github.com/PostHog/posthog/pull/51212) @@ -257,6 +303,33 @@ _Also asked as:_ auto-assign bot PRs, find the human behind an agent PR, nudge f ## Dev environment +### Replace mprocs with Tilt for the local dev orchestration + +**Verdict: rejected** · Oct 2025 · [#40698](https://github.com/PostHog/posthog/pull/40698) + +The proposal is correct about the problem. mprocs starts every process in parallel and knows nothing about the dependencies, so services fail when their dependencies are not ready. + +The PR did not merge. `bin/mprocs.yaml` is still the process list today, and the repository has no Tilt configuration. + +The dependency problem got other answers. `bin/wait-for-docker` and the compose health checks do the waiting, and the intent system in hogli decides which processes start. + +_Also asked as:_ Tilt, replace mprocs, dev orchestrator, services start in the wrong order + +### Share the dev environment and the Docker containers across worktrees + +**Verdict: three attempts, none merged** · Oct 2025 to Apr 2026 · [#40634](https://github.com/PostHog/posthog/pull/40634), [#45984](https://github.com/PostHog/posthog/pull/45984), [#51100](https://github.com/PostHog/posthog/pull/51100) + +Each attempt used a different mechanism. +[#40634](https://github.com/PostHog/posthog/pull/40634) changed the compose setup so a worktree uses the containers of the main checkout. +[#45984](https://github.com/PostHog/posthog/pull/45984) set `COMPOSE_PROJECT_NAME` in the flox variables. Docker Compose uses the directory name when this variable is absent, so each worktree makes its own containers. +[#51100](https://github.com/PostHog/posthog/pull/51100) shared the flox environment, the Python virtual environment, and `node_modules`. It reports approximately 5 GB of disk for each worktree. + +None of the three merged. `bin/wait-for-docker` gives the compose project the default name `posthog` today, which gives the shared containers that #45984 wanted. + +Read [#40634](https://github.com/PostHog/posthog/pull/40634) first if you propose this again. It asks the question that stopped all three: does any person need separate databases for each worktree? + +_Also asked as:_ worktrees start their own containers, share node_modules between worktrees, worktree disk usage, COMPOSE_PROJECT_NAME + ### Run a dmypy daemon for fast local type checks **Verdict: rejected** · Oct 2025 · [#39319](https://github.com/PostHog/posthog/pull/39319) @@ -323,9 +396,11 @@ The PR added a squash planner, a policy for opaque operations, and 65 squashed m The problem is the value. The PR reports that the effect on the timing was small and noisy. The work to resolve each blocker is large, and the reviews are difficult. -The migration replay in CI is a real cost. A different change must decrease it. +[#60518](https://github.com/PostHog/posthog/pull/60518) tried a second angle three months later. It took the final project state at a cutoff date and rebuilt it as one set of `CreateModel` operations. The PR says that per-app squashing "only nibbles at it because the dep graph is cross-app". That PR also did not merge. + +The migration replay in CI is a real cost. Two different squash designs did not decrease it enough. A different change must decrease it. -_Also asked as:_ squash the migrations, compress the migration history, why are there so many migrations, speed up the migration replay +_Also asked as:_ squash the migrations, compress the migration history, why are there so many migrations, speed up the migration replay, nextgensquash ### Build the generated pydantic schema lazily with `defer_build` @@ -352,8 +427,38 @@ Import a model from `posthog.schema` inside the method that uses it. Take the en _Also asked as:_ remove pydantic, use dataclasses for the schema, the schema import is slow, why is `posthog.schema` so big +## API contracts + +### Validate the API responses against the generated OpenAPI schema + +**Verdict: six attempts, none merged** · Mar 2026 to Jun 2026 + +The idea returns in two shapes. + +End-to-end traffic validation, through a Django middleware and the Playwright run: [#49895](https://github.com/PostHog/posthog/pull/49895), [#49898](https://github.com/PostHog/posthog/pull/49898), [#49932](https://github.com/PostHog/posthog/pull/49932), [#49940](https://github.com/PostHog/posthog/pull/49940). +Response validation inside the pytest run, with a report as a CI artifact: [#56804](https://github.com/PostHog/posthog/pull/56804), [#56810](https://github.com/PostHog/posthog/pull/56810). + +Each PR made the validation optional and non-blocking, to avoid noise. None of them merged. +Read this history before you start a seventh attempt. Six PRs that all stop before the merge is a signal about the design, not about the effort. + +The generated types have a different guard today. The serializers produce the OpenAPI schema, and `hogli build:openapi` generates the TypeScript from it. CI fails when the committed output does not match. + +_Also asked as:_ contract testing, validate responses against the schema, spectral, prism, schema drift in CI + ## Product isolation +### Move `ee/` into `products/enterprise/backend/` + +**Verdict: rejected** · Nov 2025 · [#41025](https://github.com/PostHog/posthog/pull/41025) + +The PR moved 613 files and kept the git history. It kept the app label `ee`, so the database did not change. Django validated, and no migration was necessary. + +The PR did not merge, and `ee/` is still a top-level directory. + +A mechanically correct move is not sufficient for a directory of this size. If you propose this again, say who reviews 613 moved files, and what breaks for each open PR that touches `ee/`. + +_Also asked as:_ move ee to products, get rid of the ee folder, enterprise product folder + ### Use `logs` as the first product behind a facade **Verdict: superseded** · Jun 2026 · [#63184](https://github.com/PostHog/posthog/pull/63184) From 6753556edf61e991535cfedccf2904a48ec48362 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 27 Aug 2026 11:16:03 +0200 Subject: [PATCH 10/24] chore(devex): fact-check every entry, drop three, add the person-table saga Ran a per-entry check of all 32 entries against PR metadata, PR bodies and comments, and present-day repo state. 29 held up. Three did not, and all three failed the same way: the PR was trusted over the repo. - The Tilt entry claimed bin/mprocs.yaml is still the process list. It is not. bin/start defaults to phrocs, which landed in #50341 back in March and reads a config generated per run by hogli dev:generate. - The SeaweedFS entry dated the first attempt to Mar 2026. The real first attempts were Sep and Oct 2025, the replay store migration merged in Nov 2025, and the PR that actually shipped the objectstorage swap (#60432) was never cited. The one PR it did cite never merged. - The xdist entry quoted 42.8%. No source says that. The closing comment gives rounded wall times, so the entry now gives those. Two claims were mine rather than the record's, and are marked or removed. The Blacksmith trial's duration and its cache-asymmetry conclusion are not in either PR, so the entry now says the detail lives in a team-channel report it cannot show. The sparse-checkout entry stated a rejection reason that nobody ever wrote: no human reviewed that PR, the stale bot closed it. It also described a diff that touched three workflows when it touched two, and missed that ci-rust.yml already used sparse-checkout beforehand. Removes Tilt, SeaweedFS, and Granian. None for being old. Tilt's subsystem was rebuilt around it, and the other two shipped, so neither failure is a live trap. Adds a "Remove an entry" section so the next removal has a rule to cite. The oldest entry in the file, xdist from October, stays: it is still the most re-proposed idea here. Adds the person-table cutover, eight unmerged PRs from November trying five mechanisms to repoint the Person model at a partitioned table. Person access now goes through the personhog client, so a Django-level cutover is no longer where that problem lives. --- docs/internal/ci-things-already-tried.md | 92 +++++++++++++----------- 1 file changed, 49 insertions(+), 43 deletions(-) diff --git a/docs/internal/ci-things-already-tried.md b/docs/internal/ci-things-already-tried.md index 21fe6365b839..2bcf6e1e13da 100644 --- a/docs/internal/ci-things-already-tried.md +++ b/docs/internal/ci-things-already-tried.md @@ -37,6 +37,20 @@ Add an entry when you close a PR and do not merge it. Add an entry when you reve Give the entry the title of the idea. Then give the verdict, the date, and the measurement. An entry of five lines has more value than a design document that nobody opens. +## Remove an entry + +Age alone is not a reason to remove an entry. The pytest-xdist entry is the oldest here, and people still propose that idea. + +Remove an entry when one of these is true: + +The system that it describes is gone. A person cannot propose the idea any more, so the verdict guides nobody. + +The idea shipped later. The entry is history, not prior art. Keep it only when the first failure is still a trap. + +The entry gives a general lesson and no specific trap. "Measure before you optimize" does not need an entry. + +Give the reason when you remove an entry. Do not remove an entry because it looks old. + --- ## Test parallelism and sharding @@ -46,7 +60,7 @@ An entry of five lines has more value than a design document that nobody opens. **Verdict: rejected** · Oct 2025 · [#38927](https://github.com/PostHog/posthog/pull/38927) The test used 53 shards and `-n 4`. -Wall time decreased from approximately 15 minutes to approximately 9 minutes. This is a speed increase of 42.8%, and the measurement is statistically strong. +Wall time decreased from approximately 15 minutes to approximately 9 minutes. The PR reports the difference as statistically strong. CPU cost increased from 1,572 to 3,908 core-minutes. This is a factor of approximately 2.5. The speed increase is real. The cost is the problem. @@ -237,12 +251,12 @@ _Also asked as:_ Docker Hub rate limit in CI, unauthenticated pull limit, DOCKER The trial did not do a direct exchange. It ran a Blacksmith shadow of most compute jobs on the same commit, behind the `BLACKSMITH_SHADOW_ENABLED` variable. Each shadow used `continue-on-error`, and no shadow was a required check. -The comparison was not conclusive. Some jobs looked much slower on the shadow runner. The step-level data showed a difference in the cache state between the two providers, not a difference in compute speed. The trial also ran for approximately 16 hours, which is too short for the jobs whose medians were close. - -The team kept Depot. [#57991](https://github.com/PostHog/posthog/pull/57991) removed the shadow workflow and the matrix branches. +The team kept Depot. The comparison did not give a clear answer. +The detail of that comparison is not in the PRs. It went to a report in the team channel, so this entry cannot show you the numbers. [#57991](https://github.com/PostHog/posthog/pull/57991) removed the shadow workflow and the matrix branches. It kept `.github/scripts/compare-ci-runners.py` and marked the file as legacy. That script produced the numbers of the trial. -If you propose this again, equalize the caches first. Otherwise the next trial measures the cache plumbing again, not the runners. +If you propose this again, equalize the caches of the two providers first. A runner that keeps a warm cache between jobs measures the cache, not the compute. +Run the trial for several days. A short window cannot separate the jobs whose times are close. _Also asked as:_ change CI provider, Blacksmith, cheaper runners, are the Depot runners slow @@ -250,12 +264,15 @@ _Also asked as:_ change CI provider, Blacksmith, cheaper runners, are the Depot **Verdict: rejected for those workflows** · Oct 2025 · [#39239](https://github.com/PostHog/posthog/pull/39239) -The proposal added sparse-checkout to the backend, frontend, and Rust workflows. Each job would exclude the directories that it does not use. +The description covers the backend, frontend, and Rust workflows. The diff changes only `ci-backend.yml` and `ci-rust.yml`. + +The PR did not merge, and no person reviewed it. The stale bot closed it. Thus there is no recorded reason for the result. -The PR did not merge. Sparse-checkout is still correct for small jobs, and `ci-storybook.yml`, `ci-security.yaml`, and `pr-resolve-outdated-bot-comments.yml` use it today. -The large test workflows do not. A test job reads more of the tree than the exclusion list expects, and the migration jobs change refs. +Sparse-checkout is correct for small jobs, and `ci-storybook.yml`, `ci-security.yaml`, and `pr-resolve-outdated-bot-comments.yml` use it today. +`ci-rust.yml` also uses it on the build and test jobs, but that came before this PR. +The large Python and frontend test jobs do not use it. Their checkout is complete. -If you propose this again, name the jobs and prove that each one reads only the included paths. +If you propose this again, name the jobs and prove that each one reads only the included paths. A test job can read more of the tree than an exclusion list expects. _Also asked as:_ sparse-checkout, partial clone, do not check out the whole repo, speed up the checkout step @@ -303,18 +320,6 @@ _Also asked as:_ auto-assign bot PRs, find the human behind an agent PR, nudge f ## Dev environment -### Replace mprocs with Tilt for the local dev orchestration - -**Verdict: rejected** · Oct 2025 · [#40698](https://github.com/PostHog/posthog/pull/40698) - -The proposal is correct about the problem. mprocs starts every process in parallel and knows nothing about the dependencies, so services fail when their dependencies are not ready. - -The PR did not merge. `bin/mprocs.yaml` is still the process list today, and the repository has no Tilt configuration. - -The dependency problem got other answers. `bin/wait-for-docker` and the compose health checks do the waiting, and the intent system in hogli decides which processes start. - -_Also asked as:_ Tilt, replace mprocs, dev orchestrator, services start in the wrong order - ### Share the dev environment and the Docker containers across worktrees **Verdict: three attempts, none merged** · Oct 2025 to Apr 2026 · [#40634](https://github.com/PostHog/posthog/pull/40634), [#45984](https://github.com/PostHog/posthog/pull/45984), [#51100](https://github.com/PostHog/posthog/pull/51100) @@ -363,26 +368,6 @@ Before you increase the Python version, examine which versions the pinned flox u _Also asked as:_ bump Python, upgrade the interpreter, why is Python pinned to an exact version -### Replace Unit and Uvicorn with Granian in one step - -**Verdict: superseded** · Oct 2025 · [#40450](https://github.com/PostHog/posthog/pull/40450), replaced by [#40847](https://github.com/PostHog/posthog/pull/40847) - -A dual-mode PR replaced the direct exchange. The dual mode keeps Unit as the default and starts Granian when `USE_GRANIAN=true`. -The work is the same, and the rollout is safer. Granian is a dependency today. - -_Also asked as:_ migrate to Granian, replace Nginx Unit, unify the ASGI server - -### Change the object storage service from MinIO to SeaweedFS - -**Verdict: shipped, after several unsuccessful attempts** · first attempt Mar 2026 · [#49827](https://github.com/PostHog/posthog/pull/49827) - -Other people made earlier attempts, and those attempts also failed. -The change is complete now. The dev stack and CI use SeaweedFS for both S3-compatible stores. `AGENTS.md` prohibits new dependencies on MinIO. - -This entry is evidence that a migration can be correct after several failures. - -_Also asked as:_ remove MinIO, SeaweedFS, replace the object storage container - ## Django performance [docs/internal/django-startup-time.md](django-startup-time.md) is the deep source for this area. It has a Traps section that records the failure modes of each mechanism. @@ -420,13 +405,34 @@ _Also asked as:_ `defer_build`, make the schema import lazy, pydantic model buil **Verdict: not viable today** `posthog/schema.py` has more than 1,000 generated classes. `hogli build:schema` generates the file from the TypeScript types with pydantic tooling. -More than 250 files call `model_validate`, and the API layer depends on this validation. Thus a change of the model library is not a local change. +Approximately 220 files call `model_validate`, and more call the `model_validate_json` and `model_validate_python` variants. The API layer depends on this validation. Thus a change of the model library is not a local change. The import cost is solved. `posthog.schema` costs approximately 2 seconds to import, but `django.setup()` no longer loads it. The enums also moved to `posthog.schema_enums`, which imports in approximately 20 ms. Import a model from `posthog.schema` inside the method that uses it. Take the enums from `posthog.schema_enums`. _Also asked as:_ remove pydantic, use dataclasses for the schema, the schema import is slow, why is `posthog.schema` so big +## Database migrations + +### Switch the Person model to the partitioned table with a Django setting + +**Verdict: eight attempts, none merged** · Nov 2025 · [#41436](https://github.com/PostHog/posthog/pull/41436), [#41513](https://github.com/PostHog/posthog/pull/41513), [#41522](https://github.com/PostHog/posthog/pull/41522), [#41600](https://github.com/PostHog/posthog/pull/41600), [#41604](https://github.com/PostHog/posthog/pull/41604), [#41669](https://github.com/PostHog/posthog/pull/41669), [#41698](https://github.com/PostHog/posthog/pull/41698), [#41813](https://github.com/PostHog/posthog/pull/41813) + +The goal is to move the Person model from `posthog_person` to a table that is partitioned by `team_id`. Eight PRs tried five mechanisms. + +A `PERSON_TABLE_NAME` setting that gives `db_table` to the model. A dual manager that reads both tables and prefers the new one. A swap of the two table names in the database. A wrapper that rejects any query to a partitioned table without `team_id` in the `WHERE` clause. A separate test database for the person tables. + +None of them merged. The author wrote this on [#41513](https://github.com/PostHog/posthog/pull/41513): + +> I'm still not sure if I got on the wrong track here by wanting to bend all test setup to use the person_new table and other sqlx migrated stuff. It seems I overlooked something fundamental since things are failing so much. + +The work continues, but the mechanism is different. `PERSON_TABLE_NAME` is not in the code. `posthog_person_new` survives in one Dagster job, with a comment about a future name swap. +Person and group data now goes through the gRPC client in `posthog/personhog_client/`. `AGENTS.md` makes that client the required interface and prohibits new ORM queries against the person tables. + +Read this entry before you propose a Django-level cutover. The Django setting is not where this problem lives any more. + +_Also asked as:_ partition the person table, `PERSON_TABLE_NAME`, `posthog_person_new`, dual-table reads, cut over the Person model + ## API contracts ### Validate the API responses against the generated OpenAPI schema @@ -435,7 +441,7 @@ _Also asked as:_ remove pydantic, use dataclasses for the schema, the schema imp The idea returns in two shapes. -End-to-end traffic validation, through a Django middleware and the Playwright run: [#49895](https://github.com/PostHog/posthog/pull/49895), [#49898](https://github.com/PostHog/posthog/pull/49898), [#49932](https://github.com/PostHog/posthog/pull/49932), [#49940](https://github.com/PostHog/posthog/pull/49940). +End-to-end traffic validation, in the Playwright run. Most of these use a Django middleware, and [#49895](https://github.com/PostHog/posthog/pull/49895) uses Spectral and a Prism proxy instead: [#49898](https://github.com/PostHog/posthog/pull/49898), [#49932](https://github.com/PostHog/posthog/pull/49932), [#49940](https://github.com/PostHog/posthog/pull/49940). Response validation inside the pytest run, with a report as a CI artifact: [#56804](https://github.com/PostHog/posthog/pull/56804), [#56810](https://github.com/PostHog/posthog/pull/56810). Each PR made the validation optional and non-blocking, to avoid noise. None of them merged. From 78e01b7f3db6dddfd2719df7efeba770f76675fd Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 27 Aug 2026 11:40:14 +0200 Subject: [PATCH 11/24] fix(devex): unblock the timing refresh and derive the split margin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The timing workflow has failed on every run since .test_durations left the repo. That PR stripped the commit machinery from this workflow but left a `git checkout -- .test_durations` behind, and the file is now untracked and gitignored, so the step errors: error: pathspec '.test_durations' did not match any file(s) known to git Consequence: no map has been published since. Every consumer has been planning against a cache entry from before the duration floor was removed, so that fix has never actually reached CI. Drop the checkout, and guard the copy — the cache restore is best-effort and there is no tracked file behind it any more. With an unfloored map in hand the fitted split margin is wrong. It was calibrated against shards planned on a floored map, where tens of thousands of tiny parametrized tests carried ten times their real weight and the partition only looked balanced. Replaying the real map through the splitter puts max/mean at 1.02, not the 1.12-1.32 the table assumes, so the table would now buy shards nothing needs. Derive it instead. pytest-split cuts between tests and never inside one, so a contiguous split's worst chunk runs at most one whole test past the mean. Sizing the worst chunk rather than the mean gives work / n + maxTest <= budget -> n = ceil(work / (budget - maxTest)) which needs no fitted constant, no fixed point, and tracks the map rather than a snapshot of it: a product carrying one heavy test gets the shards that test forces, an evenly grained one gets none it does not need. A test at or above the budget is called out on its own — no split can hold the target then, so size by work alone instead of buying shards that cannot help. Against the current map this sizes the split products at 22 shards where they run 27 today, and stops splitting signals, whose whole suite fits one shard. Claude-Session: https://claude.ai/code/session_01EJ1y3UzXR1BKtukE886ks9 --- .github/scripts/turbo-discover-sizing.test.js | 28 ++-- .github/scripts/turbo-discover.js | 123 +++++++++--------- .../ci-backend-update-test-timing.yml | 14 +- 3 files changed, 92 insertions(+), 73 deletions(-) diff --git a/.github/scripts/turbo-discover-sizing.test.js b/.github/scripts/turbo-discover-sizing.test.js index efb995549ce8..d81d92c58cbb 100644 --- a/.github/scripts/turbo-discover-sizing.test.js +++ b/.github/scripts/turbo-discover-sizing.test.js @@ -7,7 +7,7 @@ const test = require('node:test') const assert = require('node:assert/strict') -const { pruneDeadDurations, getSegmentDuration, calculateShards, resolveProductSizing, buildMatrix, productSplitShards, splitImbalanceFactor, PRODUCT_JOB_OVERHEAD_SECONDS, TARGET_WALL_SECONDS } = require('./turbo-discover.js') +const { pruneDeadDurations, getSegmentDuration, calculateShards, resolveProductSizing, buildMatrix, productSplitShards, PRODUCT_JOB_OVERHEAD_SECONDS, TARGET_WALL_SECONDS } = require('./turbo-discover.js') // A path that exists in every checkout, so the existence check is deterministic. const LIVE_FILE = '.github/scripts/turbo-discover.js' @@ -96,7 +96,7 @@ test('buildMatrix splits a product to the shared wall target', () => { const matrix = buildMatrix(['big-one'], union, true) // 2000s of work, with the imbalance margin, over a (target - overhead) budget. - assert.equal(matrix.length, productSplitShards(2000)) + assert.equal(matrix.length, productSplitShards(2000, 50)) assert.match(matrix[0].group, /^big-one \(1\/\d+\)$/) }) @@ -110,12 +110,18 @@ test('buildMatrix leaves a small product packed', () => { assert.deepEqual(matrix[0].legs, [{ filters: '--filter=@posthog/products-small-one', pytest_args: '' }]) }) -test('splitImbalanceFactor rises with the split count and never marks up a single shard', () => { - assert.equal(splitImbalanceFactor(1), 1) - assert.ok(splitImbalanceFactor(2) < splitImbalanceFactor(6)) - assert.ok(splitImbalanceFactor(6) < splitImbalanceFactor(9)) - // Clamped, not extrapolated, past the measured range. - assert.equal(splitImbalanceFactor(40), splitImbalanceFactor(9)) +test('productSplitShards sizes the worst chunk, so a heavy test buys shards', () => { + const budget = TARGET_WALL_SECONDS - PRODUCT_JOB_OVERHEAD_SECONDS + // Same total work; the coarser grain cannot be cut as finely, so it needs more + // shards to keep its worst chunk inside the budget. + const fine = productSplitShards(1000, 10) + const coarse = productSplitShards(1000, 200) + assert.ok(coarse > fine, `expected ${coarse} > ${fine}`) + assert.ok(1000 / fine + 10 <= budget) + assert.ok(1000 / coarse + 200 <= budget) + // A test at or above the budget cannot be split out of, so no shard count + // meets the target; size by work alone rather than buying useless shards. + assert.equal(productSplitShards(1000, budget * 2), Math.ceil(1000 / budget)) }) test('a product that fits one shard is packed, not split by its own margin', () => { @@ -127,7 +133,7 @@ test('a product that fits one shard is packed, not split by its own margin', () } assert.ok(300 <= TARGET_WALL_SECONDS - PRODUCT_JOB_OVERHEAD_SECONDS) - assert.equal(productSplitShards(300), 1) + assert.equal(productSplitShards(300, 30), 1) const matrix = buildMatrix(['mid-one'], union, true) @@ -140,9 +146,9 @@ test("a split product's last shard absorbs a small product without leaking split for (let i = 0; i < 11; i++) { union[`products/big_one/backend/test_${i}.py::test_${i}`] = 30 } - union['products/small_one/backend/test_s.py::test_s'] = 60 + union['products/small_one/backend/test_s.py::test_s'] = 40 - assert.equal(productSplitShards(330), 2) + assert.equal(productSplitShards(330, 30), 2) const matrix = buildMatrix(['big-one', 'small-one'], union, true) diff --git a/.github/scripts/turbo-discover.js b/.github/scripts/turbo-discover.js index 301026b645d7..ac4cd1183c85 100644 --- a/.github/scripts/turbo-discover.js +++ b/.github/scripts/turbo-discover.js @@ -50,9 +50,9 @@ const { loadContractSurfaces } = require('./trunk-impacted-targets') // for n, so the target is a promise about the PR lane (where the overheads below // are fitted); master pays extra overhead (full migration replay) on top. // A full run's wall is the pre-shard preamble plus the slowest of its shards. -// The preamble (discovery, matrix build, runner start) measures ~5.5 min and the -// slowest shard lands above the target by the imbalance margin below, so a -// 12-minute shard target puts a full PR run near 19 minutes end to end. +// The preamble (discovery, matrix build, runner start) measures ~5.5 min, and +// sizing bounds the slowest shard at the target rather than the average, so a +// 12-minute shard target puts a full PR run near 18 minutes end to end. const TARGET_WALL_SECONDS = 12 * 60 // Per-product cost within a runner: turbo dispatch, pytest collection, Django // init. First product pays ~45s, subsequent ~15s; use 60s as a conservative @@ -65,20 +65,9 @@ const PRODUCT_PER_PRODUCT_OVERHEAD_SECONDS = 60 // budget of TARGET minus PRODUCT_JOB_BASE_OVERHEAD_SECONDS and bought extra // buckets, each paying that base overhead again. const PRODUCT_BUCKET_SAFETY_FACTOR = 1.1 -// Headroom on a split product. Sizing solves for the MEAN shard, but a run's -// wall is the MAX shard, and this is the only thing bridging the two. The gap -// grows with the split count: the max is an order statistic over more shards, -// and the product's own intra-split skew rides on top. Measured max/mean at p90 -// over PR runs; interpolated between the points and clamped outside them. -// A per-product ratio read back from JUnit would beat a table keyed on n alone, -// because the skew is a property of the suite. That needs per-test wall times, -// which call-only durations cannot give (see PRODUCTS_SCALED_MARKER). -const SPLIT_IMBALANCE_BY_SHARDS = [ - [2, 1.12], - [4, 1.2], - [6, 1.23], - [9, 1.32], -] +// No headroom constant for a split product: the gap between the mean shard that +// sizing solves for and the max shard that sets the wall is derived per product +// in productSplitShards below. // Fitted per-shard overhead for a split product job. Two measured parts, from // run 32717208712: the job base (docker stack, deps, turbo dispatch) is // mean(job wall - JUnit suite time), 247-413s across 12 bucket jobs (median @@ -718,6 +707,24 @@ function getProductDuration(product, durations) { return total } +// The longest single test in a product. pytest-split cuts between tests, never +// inside one, so this is the irreducible grain of any split and it bounds how +// far the worst chunk can run past the mean. +function getProductMaxTest(product, durations) { + if (!durations) { + return 0 + } + const prefix = productPrefix(product) + const excluded = PRODUCTS_RUNNING_TEMPORAL_IN_JOB.has(product) ? [] : EXCLUDED_PATH_SEGMENTS + let longest = 0 + for (const [test, dur] of Object.entries(durations)) { + if (test.startsWith(prefix) && !excluded.some((seg) => test.includes(seg)) && dur > longest) { + longest = dur + } + } + return longest +} + // One definition of a product's work estimate, shared by the split decision // (buildMatrix) and the bucket cost (packProducts), so they cannot disagree. // @@ -730,17 +737,24 @@ function getProductDuration(product, durations) { // the recorded sum, so the caller can log it once. function resolveProductSizing(product, durations, productsScaled = false) { const unionWork = getProductDuration(product, durations) + const maxTest = getProductMaxTest(product, durations) if (productsScaled && unionWork > 0) { - return { work: unionWork, staleUnionWork: null, staleness: null } + return { work: unionWork, maxTest, staleUnionWork: null, staleness: null } } const staleness = checkProductStaleness(product, durations) if (staleness.stale && staleness.fileCount > 0) { const fallbackWork = staleness.fileCount * STALENESS_FALLBACK_SECONDS_PER_FILE if (fallbackWork > unionWork) { - return { work: fallbackWork, staleUnionWork: unionWork, staleness } + // The guess has no per-test shape, so assume one file's worth is one test. + return { + work: fallbackWork, + maxTest: Math.max(maxTest, STALENESS_FALLBACK_SECONDS_PER_FILE), + staleUnionWork: unionWork, + staleness, + } } } - return { work: unionWork, staleUnionWork: null, staleness: null } + return { work: unionWork, maxTest, staleUnionWork: null, staleness: null } } function productEffectiveCost(product, durations, productsScaled = false) { @@ -865,42 +879,36 @@ function calculateShards(totalWorkSeconds, overheadSeconds, minShards = DJANGO_M return Math.max(minShards, Math.min(DJANGO_MAX_SHARDS, shards)) } -// Interpolates SPLIT_IMBALANCE_BY_SHARDS, clamped at both ends. One shard is not -// a split, so it carries no imbalance. -function splitImbalanceFactor(shards) { - if (shards <= 1) { - return 1 - } - const first = SPLIT_IMBALANCE_BY_SHARDS[0] - const last = SPLIT_IMBALANCE_BY_SHARDS[SPLIT_IMBALANCE_BY_SHARDS.length - 1] - if (shards <= first[0]) { - return first[1] - } - for (let i = 1; i < SPLIT_IMBALANCE_BY_SHARDS.length; i++) { - const [prevShards, prevFactor] = SPLIT_IMBALANCE_BY_SHARDS[i - 1] - const [nextShards, nextFactor] = SPLIT_IMBALANCE_BY_SHARDS[i] - if (shards <= nextShards) { - const span = (shards - prevShards) / (nextShards - prevShards) - return prevFactor + (nextFactor - prevFactor) * span - } - } - return last[1] +// Budget of test work one product shard can hold, mirroring calculateShards. +function productShardBudget() { + return Math.max(TARGET_WALL_SECONDS - PRODUCT_JOB_OVERHEAD_SECONDS, PRODUCT_JOB_OVERHEAD_SECONDS / 2, 1) } -// The imbalance margin depends on the shard count, which depends on the margin. -// Iterate to the fixed point, seeding at no margin so a product that fits whole -// is never split into two by its own headroom. Both sides only rise, so the -// sequence is monotone and settles in a pass or two. -function productSplitShards(workSeconds) { - let shards = calculateShards(workSeconds, PRODUCT_JOB_OVERHEAD_SECONDS, 1) - for (let pass = 0; pass < 3; pass++) { - const next = calculateShards(workSeconds * splitImbalanceFactor(shards), PRODUCT_JOB_OVERHEAD_SECONDS, 1) - if (next === shards) { - break - } - shards = next +// Shards for one product. Sizing a split by work/n sizes the MEAN shard, but the +// run's wall is the MAX shard, and pytest-split cuts between tests: a contiguous +// split's worst chunk runs at most one whole test past the mean. So size the +// worst chunk directly -- work/n + maxTest <= budget -- which rearranges to +// n = ceil(work / (budget - maxTest)). +// +// This replaces a fitted margin. A ratio measured off CI describes one map, and +// a map that misreports test weights (a floor on tiny tests, say) bakes its own +// error into the constant. Deriving from maxTest tracks the map instead: a +// product carrying one heavy test gets the shards that test forces, an evenly +// grained one gets none it does not need, and a fixed point is unnecessary. +// +// n = 1 is checked first because the bound does not apply to it -- an unsplit +// product's chunk is its whole work, with no extra test on top. +function productSplitShards(workSeconds, maxTestSeconds = 0) { + const budget = productShardBudget() + if (workSeconds <= budget) { + return 1 } - return shards + if (maxTestSeconds >= budget) { + // One test already overruns a shard's budget, so no split can hold the + // target. Size by work alone rather than buying shards that cannot help. + return Math.max(2, Math.min(DJANGO_MAX_SHARDS, Math.ceil(workSeconds / budget))) + } + return Math.max(2, Math.min(DJANGO_MAX_SHARDS, Math.ceil(workSeconds / (budget - maxTestSeconds)))) } // Selector segment key -> Django matrix segment name. @@ -1067,7 +1075,7 @@ function buildMatrix(products, durations, productsScaled = false) { // the fixture-heavy suites whose recorded durations undercount the most, // and a split sized on the bare sum lands its shards well past the target. for (const product of products) { - const { work, staleUnionWork, staleness } = resolveProductSizing(product, durations, productsScaled) + const { work, maxTest, staleUnionWork, staleness } = resolveProductSizing(product, durations, productsScaled) if (staleUnionWork !== null) { console.error( ` ${product}: .test_durations stale, ${staleness.coveredCount}/${staleness.fileCount} test files covered ` + @@ -1079,7 +1087,7 @@ function buildMatrix(products, durations, productsScaled = false) { ) } - const shards = productSplitShards(work) + const shards = productSplitShards(work, maxTest) if (shards > 1) { console.error(` ${product}: ${(work / 60).toFixed(1)} min work → split across ${shards} shards`) const filters = `--filter=@posthog/products-${product}` @@ -1088,7 +1096,7 @@ function buildMatrix(products, durations, productsScaled = false) { // optimally. The greedy rule in duration_based_chunks lets every shard // overrun the per-shard average, which on skewed suites starves trailing // shards down to zero tests (pytest exit 5, "no tests collected"). - const shardCost = (work * splitImbalanceFactor(shards)) / shards + const shardCost = work / shards + maxTest for (let i = 1; i <= shards; i++) { const leg = { filters, @@ -1149,9 +1157,8 @@ module.exports = { buildMatrix, PRODUCT_JOB_OVERHEAD_SECONDS, PRODUCT_BUCKET_SAFETY_FACTOR, - SPLIT_IMBALANCE_BY_SHARDS, - splitImbalanceFactor, productSplitShards, + getProductMaxTest, PRODUCTS_SCALED_MARKER, TARGET_WALL_SECONDS, DJANGO_OVERHEAD_SECONDS_BY_SEGMENT, diff --git a/.github/workflows/ci-backend-update-test-timing.yml b/.github/workflows/ci-backend-update-test-timing.yml index fbde480bc553..8ef168c20cb5 100644 --- a/.github/workflows/ci-backend-update-test-timing.yml +++ b/.github/workflows/ci-backend-update-test-timing.yml @@ -166,11 +166,17 @@ jobs: posthog-test-durations- - name: Set the previous map aside - # The restore lands on the tracked file; keep the copy elsewhere and put - # the committed file back so the change check below diffs a clean tree. + # Keep the restored map where the drift fallback below expects it, and + # take it out of the tree so the fresh map is written rather than merged + # onto the old one. .test_durations is not tracked (it left the repo when + # the timing workflow started publishing it as an artifact), so there is + # nothing to restore it from and a cache miss leaves no file at all. run: | - cp .test_durations /tmp/previous_durations - git checkout -- .test_durations + if [ -f .test_durations ]; then + mv .test_durations /tmp/previous_durations + else + echo "::warning::No cached map restored; a drifting Products slice has no stand-in" + fi - name: Download timing artifacts env: From 6185f007c825c7026b635e36cde379a8992b1d13 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 27 Aug 2026 11:44:25 +0200 Subject: [PATCH 12/24] chore(devex): mirror the shard-job changes into the depot shadows The drift check compares the depot shadows against canonical, so the legs loop and the untracked-map guard have to land in both. Claude-Session: https://claude.ai/code/session_01EJ1y3UzXR1BKtukE886ks9 --- .../ci-backend-update-test-timing.yml | 9 +++-- .depot/workflows/ci-backend.yml | 33 ++++++++++++++----- 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/.depot/workflows/ci-backend-update-test-timing.yml b/.depot/workflows/ci-backend-update-test-timing.yml index 7544672342e6..aca8ca40ae8b 100644 --- a/.depot/workflows/ci-backend-update-test-timing.yml +++ b/.depot/workflows/ci-backend-update-test-timing.yml @@ -67,9 +67,14 @@ jobs: posthog-test-durations- - name: Set the previous map aside + # .test_durations is not tracked, so there is nothing to restore it from + # and a cache miss leaves no file at all. run: | - cp .test_durations /tmp/previous_durations - git checkout -- .test_durations + if [ -f .test_durations ]; then + mv .test_durations /tmp/previous_durations + else + echo "::warning::No cached map restored; a drifting Products slice has no stand-in" + fi - name: Download timing artifacts env: diff --git a/.depot/workflows/ci-backend.yml b/.depot/workflows/ci-backend.yml index cc182e96a2f9..4850f7da9333 100644 --- a/.depot/workflows/ci-backend.yml +++ b/.depot/workflows/ci-backend.yml @@ -941,8 +941,12 @@ jobs: - name: Run product tests # --force: discover already decided this product needs testing, skip turbo cache # --log-order=stream: stream pytest output live instead of buffering until completion - # pytest_args: optional pytest-split flags for sharded products (e.g. "-- --splits 3 --group 1") + # legs: one turbo invocation per entry, each with its own pytest-split flags + # (e.g. "-- --splits 3 --group 1"). A job holds several legs when a split + # product's last shard had budget left over for whole small products, and + # pytest-split flags must not leak from that shard onto its job-mates. env: + PRODUCT_LEGS: ${{ toJSON(matrix.legs) }} CLICKHOUSE_HOGQL_USE_NEW_EVENTS_SCHEMA: ${{ matrix.new-events-schema && 'true' || 'false' }} # products/tasks/backend/temporal moved here from the Django Temporal # segment, and its conftest and workflows talk to Modal. Injected only @@ -964,17 +968,28 @@ jobs: # bin-packed buckets (each product writes its own file) and split products (unioned later). # sysmon: Python 3.12+'s low-overhead coverage backend, ~a few % vs ~20% for the C tracer. COVERAGE_CORE: sysmon + shell: bash run: | set +e - pnpm turbo run backend:test ${{ matrix.filters }} --concurrency=1 --output-logs=full --force --log-order=stream ${{ matrix.pytest_args }} - exit_code=$? + overall=0 + leg_count=$(jq 'length' <<< "$PRODUCT_LEGS") + for ((leg_index = 0; leg_index < leg_count; leg_index++)); do + leg_filters=$(jq -r ".[$leg_index].filters" <<< "$PRODUCT_LEGS") + leg_args=$(jq -r ".[$leg_index].pytest_args" <<< "$PRODUCT_LEGS") + echo "::group::turbo backend:test $leg_filters $leg_args" + # Word splitting is intended here: both variables carry several flags. + # shellcheck disable=SC2086 + pnpm turbo run backend:test $leg_filters --concurrency=1 --output-logs=full --force --log-order=stream $leg_args + exit_code=$? + echo "::endgroup::" + if [ $exit_code -eq 5 ]; then + echo "No tests collected for this leg, this is expected when splitting tests" + elif [ $exit_code -ne 0 ]; then + overall=$exit_code + fi + done set -e - if [ $exit_code -eq 5 ]; then - echo "No tests collected for this shard, this is expected when splitting tests" - exit 0 - else - exit $exit_code - fi + exit $overall # Lightweight repo-wide checks that only need Python + uv (no Docker/DB). # Consolidates checks that previously each spun up their own runner. repo-checks: From 0802d174300a7fce9eac182784660c29c8a0ed30 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 27 Aug 2026 11:50:44 +0200 Subject: [PATCH 13/24] fix(devex): keep the timing refresh alive when the cache misses Review of the previous commit found the drift fallback still assumed the previous map is always on disk. Tolerating a cache miss in "Set the previous map aside" opened a path where /tmp/previous_durations does not exist, and the fallback's jq then aborts the whole processing step under bash -e, so Core, Temporal and Dagster never publish either. Losing the product entries for one refresh is the smaller failure, so fall back to an empty slice and keep the rest. A product test job now refuses to pass when its matrix entry carries no legs. An unset or malformed entry left the loop unentered and the job green having run no tests, which the single-command form could not do, and the rollup gate only reads this step. Three comments were left describing behaviour the code no longer has: the split path claiming a safety factor it no longer applies, a restore step claiming a cache miss leaves a checked-out file, and a packing note claiming optimal_chunks leaves the last shard lightest. The algorithm splits its heaviest group to reach the requested count, so that last claim was never true; the packing is sound because work/shards + maxTest bounds every shard, which the comment now says instead. Claude-Session: https://claude.ai/code/session_01EJ1y3UzXR1BKtukE886ks9 --- .../ci-backend-update-test-timing.yml | 20 +++++++++++----- .depot/workflows/ci-backend.yml | 7 ++++++ .github/scripts/turbo-discover.js | 17 ++++++++------ .../ci-backend-update-test-timing.yml | 23 +++++++++++++------ .github/workflows/ci-backend.yml | 7 ++++++ 5 files changed, 54 insertions(+), 20 deletions(-) diff --git a/.depot/workflows/ci-backend-update-test-timing.yml b/.depot/workflows/ci-backend-update-test-timing.yml index aca8ca40ae8b..bca2381b61d6 100644 --- a/.depot/workflows/ci-backend-update-test-timing.yml +++ b/.depot/workflows/ci-backend-update-test-timing.yml @@ -108,12 +108,20 @@ jobs: set -e if [ "$products_status" -ne 0 ]; then echo "::warning::Products durations refresh exited $products_status; keeping the previous products slice. Drift or an incomplete JUnit set are the expected causes, see the log above." - # Only entries the fresh slices do not carry: the outlier merge would - # otherwise let a stale value here beat a fresh one for the same test. - jq -s 'add' $(ls /tmp/core_durations /tmp/temporal_durations 2>/dev/null) > /tmp/fresh_durations - jq --slurpfile fresh /tmp/fresh_durations \ - 'with_entries(select((.key | startswith("products/")) and ($fresh[0][.key] == null)))' \ - /tmp/previous_durations > /tmp/products_durations + if [ -f /tmp/previous_durations ]; then + # Only entries the fresh slices do not carry: the outlier merge would + # otherwise let a stale value here beat a fresh one for the same test. + jq -s 'add' $(ls /tmp/core_durations /tmp/temporal_durations 2>/dev/null) > /tmp/fresh_durations + jq --slurpfile fresh /tmp/fresh_durations \ + 'with_entries(select((.key | startswith("products/")) and ($fresh[0][.key] == null)))' \ + /tmp/previous_durations > /tmp/products_durations + else + # The cache missed, so there is no slice to stand in. Publishing the + # other segments without product entries beats failing this step and + # publishing nothing at all. + echo "::warning::No previous map to stand in; this refresh carries no product entries." + echo '{}' > /tmp/products_durations + fi fi uv run .github/scripts/optimize_test_durations.py .test_durations \ --merge-files /tmp/core_durations /tmp/temporal_durations /tmp/products_durations \ diff --git a/.depot/workflows/ci-backend.yml b/.depot/workflows/ci-backend.yml index 4850f7da9333..15c613d4b54c 100644 --- a/.depot/workflows/ci-backend.yml +++ b/.depot/workflows/ci-backend.yml @@ -973,6 +973,13 @@ jobs: set +e overall=0 leg_count=$(jq 'length' <<< "$PRODUCT_LEGS") + # An unset or malformed matrix entry would leave the loop unentered and + # the job green having run no tests, which the single-command form could + # not do. The rollup gate only reads this step, so fail loudly instead. + if [ -z "$leg_count" ] || [ "$leg_count" -lt 1 ]; then + echo "::error::No test legs in this matrix entry; refusing to pass without running tests" + exit 1 + fi for ((leg_index = 0; leg_index < leg_count; leg_index++)); do leg_filters=$(jq -r ".[$leg_index].filters" <<< "$PRODUCT_LEGS") leg_args=$(jq -r ".[$leg_index].pytest_args" <<< "$PRODUCT_LEGS") diff --git a/.github/scripts/turbo-discover.js b/.github/scripts/turbo-discover.js index ac4cd1183c85..ec9b8d3305bd 100644 --- a/.github/scripts/turbo-discover.js +++ b/.github/scripts/turbo-discover.js @@ -1070,10 +1070,12 @@ function buildMatrix(products, durations, productsScaled = false) { const fillableJobs = [] // Split a product across multiple shards with the same rule Django uses: - // enough shards that each lands at the shared wall target. The safety - // factor applies here as it does to packing: the products that split are - // the fixture-heavy suites whose recorded durations undercount the most, - // and a split sized on the bare sum lands its shards well past the target. + // enough shards that each lands at the shared wall target. Unlike packing, + // the split carries no safety factor -- productSplitShards derives its own + // headroom from the product's longest test instead. That leaves it trusting + // the recorded sum, which holds only while the map carries + // PRODUCTS_SCALED_MARKER: call-only durations undercount a fixture-heavy + // suite several-fold, and sizing an unscaled sum under-shards it. for (const product of products) { const { work, maxTest, staleUnionWork, staleness } = resolveProductSizing(product, durations, productsScaled) if (staleUnionWork !== null) { @@ -1102,9 +1104,10 @@ function buildMatrix(products, durations, productsScaled = false) { filters, pytest_args: `-- --splits ${shards} --group ${i} --splitting-algorithm optimal_chunks`, } - // ceil() rounds the split short and optimal_chunks cuts in order, so - // the last shard is reliably the lightest. Offer its leftover budget - // to the packer rather than starting another runner for that work. + // work/shards + maxTest bounds every shard, whichever one + // optimal_chunks leaves lightest, so one shard can be offered to the + // packer without knowing which. Do not tighten this to work/shards: + // the bound is what keeps a filled shard inside the job budget. if (i === shards && !DEDICATED_BUCKET_PRODUCTS.has(product)) { fillableJobs.push({ label: `${product} (${i}/${shards})`, diff --git a/.github/workflows/ci-backend-update-test-timing.yml b/.github/workflows/ci-backend-update-test-timing.yml index 8ef168c20cb5..5948445bbf6b 100644 --- a/.github/workflows/ci-backend-update-test-timing.yml +++ b/.github/workflows/ci-backend-update-test-timing.yml @@ -155,7 +155,8 @@ jobs: # The merge below takes product entries only from the products file # (--replace-prefix), so a Products slice that fails the drift check # needs a stand-in or every product loses its durations. The last - # cached map is that stand-in. A miss leaves the checked-out file. + # cached map is that stand-in. .test_durations is untracked, so a miss + # leaves no file and the fallback below publishes no product entries. uses: actions/cache/restore@cdf6c1fa76f9f475f3d7449005a359c84ca0f306 # v5.0.3 continue-on-error: true with: @@ -271,12 +272,20 @@ jobs: set -e if [ "$products_status" -ne 0 ]; then echo "::warning::Products durations refresh exited $products_status; keeping the previous products slice. Drift or an incomplete JUnit set are the expected causes, see the log above." - # Only entries the fresh slices do not carry: the outlier merge would - # otherwise let a stale value here beat a fresh one for the same test. - jq -s 'add' $(ls /tmp/core_durations /tmp/temporal_durations /tmp/dagster_durations 2>/dev/null) > /tmp/fresh_durations - jq --slurpfile fresh /tmp/fresh_durations \ - 'with_entries(select((.key | startswith("products/")) and ($fresh[0][.key] == null)))' \ - /tmp/previous_durations > /tmp/products_durations + if [ -f /tmp/previous_durations ]; then + # Only entries the fresh slices do not carry: the outlier merge would + # otherwise let a stale value here beat a fresh one for the same test. + jq -s 'add' $(ls /tmp/core_durations /tmp/temporal_durations /tmp/dagster_durations 2>/dev/null) > /tmp/fresh_durations + jq --slurpfile fresh /tmp/fresh_durations \ + 'with_entries(select((.key | startswith("products/")) and ($fresh[0][.key] == null)))' \ + /tmp/previous_durations > /tmp/products_durations + else + # The cache missed, so there is no slice to stand in. Publishing the + # other segments without product entries beats failing this step and + # publishing nothing at all. + echo "::warning::No previous map to stand in; this refresh carries no product entries." + echo '{}' > /tmp/products_durations + fi fi # Process Dagster segment if available diff --git a/.github/workflows/ci-backend.yml b/.github/workflows/ci-backend.yml index f87e7cd29623..796e9eb262e8 100644 --- a/.github/workflows/ci-backend.yml +++ b/.github/workflows/ci-backend.yml @@ -1002,6 +1002,13 @@ jobs: set +e overall=0 leg_count=$(jq 'length' <<< "$PRODUCT_LEGS") + # An unset or malformed matrix entry would leave the loop unentered and + # the job green having run no tests, which the single-command form could + # not do. The rollup gate only reads this step, so fail loudly instead. + if [ -z "$leg_count" ] || [ "$leg_count" -lt 1 ]; then + echo "::error::No test legs in this matrix entry; refusing to pass without running tests" + exit 1 + fi for ((leg_index = 0; leg_index < leg_count; leg_index++)); do leg_filters=$(jq -r ".[$leg_index].filters" <<< "$PRODUCT_LEGS") leg_args=$(jq -r ".[$leg_index].pytest_args" <<< "$PRODUCT_LEGS") From f3ffedc4f11c4fee4c76b73841f905eed1746eab Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 27 Aug 2026 11:54:25 +0200 Subject: [PATCH 14/24] fix(devex): stop a near-budget test from exploding the shard count The mean-plus-one-test bound stops being informative once a single test eats most of a shard's budget: the denominator collapses and the count runs to the cap. A product with 321 seconds of work and one 319-second test asked for 50 shards where 3 hold the target. Below a quarter of the budget, size by work and add the one shard that carries the heavy test. The bucket factor's comment described the shared factor the sizing used before, which is history rather than a reason and goes stale as the model moves. It now states the invariant alone: the bucket factor covers error in the recorded durations, and the split path derives its own headroom. Claude-Session: https://claude.ai/code/session_01EJ1y3UzXR1BKtukE886ks9 --- .github/scripts/turbo-discover-sizing.test.js | 3 +++ .github/scripts/turbo-discover.js | 22 ++++++++++++------- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/.github/scripts/turbo-discover-sizing.test.js b/.github/scripts/turbo-discover-sizing.test.js index d81d92c58cbb..c5999531827a 100644 --- a/.github/scripts/turbo-discover-sizing.test.js +++ b/.github/scripts/turbo-discover-sizing.test.js @@ -122,6 +122,9 @@ test('productSplitShards sizes the worst chunk, so a heavy test buys shards', () // A test at or above the budget cannot be split out of, so no shard count // meets the target; size by work alone rather than buying useless shards. assert.equal(productSplitShards(1000, budget * 2), Math.ceil(1000 / budget)) + // Just under the budget the bound goes uninformative and asks for a shard per + // few seconds of remainder. Stay near what the split actually needs. + assert.ok(productSplitShards(budget + 1, budget - 1) <= 3) }) test('a product that fits one shard is packed, not split by its own margin', () => { diff --git a/.github/scripts/turbo-discover.js b/.github/scripts/turbo-discover.js index ec9b8d3305bd..9c83cf497e72 100644 --- a/.github/scripts/turbo-discover.js +++ b/.github/scripts/turbo-discover.js @@ -58,12 +58,10 @@ const TARGET_WALL_SECONDS = 12 * 60 // init. First product pays ~45s, subsequent ~15s; use 60s as a conservative // average that also absorbs the amortized portion of runner startup. const PRODUCT_PER_PRODUCT_OVERHEAD_SECONDS = 60 -// Headroom on a packed bucket. A bucket runs its products sequentially, so its -// wall is the sum of its parts — there is no mean-versus-max gap to cover here, -// and this only absorbs error in the recorded durations. Was one shared 1.3 with -// the split factor below, which marked up every small product by 30% against a -// budget of TARGET minus PRODUCT_JOB_BASE_OVERHEAD_SECONDS and bought extra -// buckets, each paying that base overhead again. +// Headroom on a packed bucket, covering error in the recorded durations alone. +// A bucket runs its products sequentially, so its wall is the sum of its parts +// and it needs no allowance for an uneven split. That allowance belongs to the +// split path, which derives its own in productSplitShards. const PRODUCT_BUCKET_SAFETY_FACTOR = 1.1 // No headroom constant for a split product: the gap between the mean shard that // sizing solves for and the max shard that sets the wall is derived per product @@ -892,7 +890,7 @@ function productShardBudget() { // // This replaces a fitted margin. A ratio measured off CI describes one map, and // a map that misreports test weights (a floor on tiny tests, say) bakes its own -// error into the constant. Deriving from maxTest tracks the map instead: a +// error into the constant. Deriving from maxTest ties the sizing to the map: a // product carrying one heavy test gets the shards that test forces, an evenly // grained one gets none it does not need, and a fixed point is unnecessary. // @@ -908,7 +906,15 @@ function productSplitShards(workSeconds, maxTestSeconds = 0) { // target. Size by work alone rather than buying shards that cannot help. return Math.max(2, Math.min(DJANGO_MAX_SHARDS, Math.ceil(workSeconds / budget))) } - return Math.max(2, Math.min(DJANGO_MAX_SHARDS, Math.ceil(workSeconds / (budget - maxTestSeconds)))) + const headroom = budget - maxTestSeconds + if (headroom < budget / 4) { + // The mean-plus-one-test bound stops being informative once a single test + // eats most of the budget: it asks for a shard per few seconds of the + // remainder. Size by work and add the one shard that carries the heavy + // test, which is what the split actually needs. + return Math.max(2, Math.min(DJANGO_MAX_SHARDS, Math.ceil(workSeconds / budget) + 1)) + } + return Math.max(2, Math.min(DJANGO_MAX_SHARDS, Math.ceil(workSeconds / headroom))) } // Selector segment key -> Django matrix segment name. From d5a0dcd3f2027e5ad2ff454e5f28ac6a6d36466c Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 27 Aug 2026 11:58:56 +0200 Subject: [PATCH 15/24] fix(devex): keep product jobs running on branches that have not rebased A workflow edit reaches every open PR the moment it merges, while a change to the script beside it only arrives when the branch rebases. Reading matrix.legs and nothing else made the new workflow require a key those branches cannot have, and the guard added with it turned that into a hard failure before any test ran. The workflow now normalizes a missing legs value into a single leg built from the flat filters and pytest_args pair, so an unrebased branch runs the invocation its own turbo-discover planned. The script keeps that pair beside the leg on every entry one invocation can express; an entry with several legs has no such form, and by the time one exists the workflow reading it is the new one. The guard stays for the case where neither shape is present, which would pass the job having run no tests. Claude-Session: https://claude.ai/code/session_01EJ1y3UzXR1BKtukE886ks9 --- .depot/workflows/ci-backend.yml | 16 +++++++++++--- .github/scripts/turbo-discover-sizing.test.js | 11 ++++++++++ .github/scripts/turbo-discover.js | 22 ++++++++++++++----- .github/workflows/ci-backend.yml | 16 +++++++++++--- 4 files changed, 53 insertions(+), 12 deletions(-) diff --git a/.depot/workflows/ci-backend.yml b/.depot/workflows/ci-backend.yml index 15c613d4b54c..02fc646783aa 100644 --- a/.depot/workflows/ci-backend.yml +++ b/.depot/workflows/ci-backend.yml @@ -947,6 +947,9 @@ jobs: # pytest-split flags must not leak from that shard onto its job-mates. env: PRODUCT_LEGS: ${{ toJSON(matrix.legs) }} + # Read only when an unrebased branch planned the pre-legs shape. + LEGACY_FILTERS: ${{ matrix.filters }} + LEGACY_PYTEST_ARGS: ${{ matrix.pytest_args }} CLICKHOUSE_HOGQL_USE_NEW_EVENTS_SCHEMA: ${{ matrix.new-events-schema && 'true' || 'false' }} # products/tasks/backend/temporal moved here from the Django Temporal # segment, and its conftest and workflows talk to Modal. Injected only @@ -972,10 +975,17 @@ jobs: run: | set +e overall=0 + # A workflow edit reaches an open PR before the script edit does, so a + # branch that has not rebased still plans the flat {filters, pytest_args} + # shape. Normalize that into one leg rather than requiring the new key. + if [ -z "$PRODUCT_LEGS" ] || [ "$PRODUCT_LEGS" = "null" ]; then + PRODUCT_LEGS=$(jq -cn --arg f "$LEGACY_FILTERS" --arg a "$LEGACY_PYTEST_ARGS" \ + '[{filters: $f, pytest_args: $a}]') + echo "Matrix entry carries no legs; running the legacy single invocation" + fi leg_count=$(jq 'length' <<< "$PRODUCT_LEGS") - # An unset or malformed matrix entry would leave the loop unentered and - # the job green having run no tests, which the single-command form could - # not do. The rollup gate only reads this step, so fail loudly instead. + # Neither shape present means the job would pass having run no tests, and + # the rollup gate only reads this step's outcome. Fail loudly instead. if [ -z "$leg_count" ] || [ "$leg_count" -lt 1 ]; then echo "::error::No test legs in this matrix entry; refusing to pass without running tests" exit 1 diff --git a/.github/scripts/turbo-discover-sizing.test.js b/.github/scripts/turbo-discover-sizing.test.js index c5999531827a..dabe83253e9e 100644 --- a/.github/scripts/turbo-discover-sizing.test.js +++ b/.github/scripts/turbo-discover-sizing.test.js @@ -110,6 +110,17 @@ test('buildMatrix leaves a small product packed', () => { assert.deepEqual(matrix[0].legs, [{ filters: '--filter=@posthog/products-small-one', pytest_args: '' }]) }) +test('a single-invocation entry keeps the pre-legs keys for unrebased branches', () => { + // A workflow edit lands on an open PR before this script does, so an old + // workflow reading matrix.filters must still find something to run. + const union = { 'products/small_one/backend/test_c.py::test_c': 100 } + + const matrix = buildMatrix(['small-one'], union, true) + + assert.equal(matrix[0].filters, '--filter=@posthog/products-small-one') + assert.equal(matrix[0].pytest_args, '') +}) + test('productSplitShards sizes the worst chunk, so a heavy test buys shards', () => { const budget = TARGET_WALL_SECONDS - PRODUCT_JOB_OVERHEAD_SECONDS // Same total work; the coarser grain cannot be cut as finely, so it needs more diff --git a/.github/scripts/turbo-discover.js b/.github/scripts/turbo-discover.js index 9c83cf497e72..3c8b6289c0d5 100644 --- a/.github/scripts/turbo-discover.js +++ b/.github/scripts/turbo-discover.js @@ -1070,6 +1070,19 @@ function buildDjangoShards(durations, ranNodeIds = {}) { return result } +// A workflow edit reaches an open PR before this script does, so an entry a +// single turbo invocation can express keeps the pre-legs {filters, pytest_args} +// keys beside its leg. An entry with several legs has no such expression and +// carries legs alone, by which point the workflow reading it is the new one. +function matrixEntry(group, legs) { + const entry = { group, legs } + if (legs.length === 1) { + entry.filters = legs[0].filters + entry.pytest_args = legs[0].pytest_args + } + return entry +} + function buildMatrix(products, durations, productsScaled = false) { const matrix = [] const packable = [] @@ -1123,15 +1136,12 @@ function buildMatrix(products, durations, productsScaled = false) { baseOverhead: PRODUCT_JOB_OVERHEAD_SECONDS, }) } else { - matrix.push({ group: `${product} (${i}/${shards})`, legs: [leg] }) + matrix.push(matrixEntry(`${product} (${i}/${shards})`, [leg])) } } } else if (DEDICATED_BUCKET_PRODUCTS.has(product)) { console.error(` ${product}: ${(work / 60).toFixed(1)} min work → dedicated job (never shared)`) - matrix.push({ - group: product, - legs: [{ filters: `--filter=@posthog/products-${product}`, pytest_args: '' }], - }) + matrix.push(matrixEntry(product, [{ filters: `--filter=@posthog/products-${product}`, pytest_args: '' }])) } else { packable.push(product) } @@ -1147,7 +1157,7 @@ function buildMatrix(products, durations, productsScaled = false) { pytest_args: '', }) } - matrix.push({ group, legs }) + matrix.push(matrixEntry(group, legs)) } return matrix diff --git a/.github/workflows/ci-backend.yml b/.github/workflows/ci-backend.yml index 796e9eb262e8..547392ce8462 100644 --- a/.github/workflows/ci-backend.yml +++ b/.github/workflows/ci-backend.yml @@ -971,6 +971,9 @@ jobs: # pytest-split flags must not leak from that shard onto its job-mates. env: PRODUCT_LEGS: ${{ toJSON(matrix.legs) }} + # Read only when an unrebased branch planned the pre-legs shape. + LEGACY_FILTERS: ${{ matrix.filters }} + LEGACY_PYTEST_ARGS: ${{ matrix.pytest_args }} CLICKHOUSE_HOGQL_USE_NEW_EVENTS_SCHEMA: ${{ matrix.new-events-schema && 'true' || 'false' }} # products/tasks/backend/temporal moved here from the Django Temporal # segment, and its conftest and workflows talk to Modal. Injected only @@ -1001,10 +1004,17 @@ jobs: run: | set +e overall=0 + # A workflow edit reaches an open PR before the script edit does, so a + # branch that has not rebased still plans the flat {filters, pytest_args} + # shape. Normalize that into one leg rather than requiring the new key. + if [ -z "$PRODUCT_LEGS" ] || [ "$PRODUCT_LEGS" = "null" ]; then + PRODUCT_LEGS=$(jq -cn --arg f "$LEGACY_FILTERS" --arg a "$LEGACY_PYTEST_ARGS" \ + '[{filters: $f, pytest_args: $a}]') + echo "Matrix entry carries no legs; running the legacy single invocation" + fi leg_count=$(jq 'length' <<< "$PRODUCT_LEGS") - # An unset or malformed matrix entry would leave the loop unentered and - # the job green having run no tests, which the single-command form could - # not do. The rollup gate only reads this step, so fail loudly instead. + # Neither shape present means the job would pass having run no tests, and + # the rollup gate only reads this step's outcome. Fail loudly instead. if [ -z "$leg_count" ] || [ "$leg_count" -lt 1 ]; then echo "::error::No test legs in this matrix entry; refusing to pass without running tests" exit 1 From db31d9cefe8aa6a59fbf1594c0d16262ace07da6 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 27 Aug 2026 12:19:01 +0200 Subject: [PATCH 16/24] fix(devex): size a split from the duration distribution, not one max Sizing read a product's longest test and assumed the rest could pack around it. That holds for one heavy test and breaks for several: ten tests just under the budget were sized at nine shards, so two of them shared a chunk and ran to twice the target. Read the distribution instead. Tests above half a shard's budget cannot pair with each other, so they are counted rather than summed and set a floor no packing goes below. The remainder is at most half a budget per test, so a contiguous chunk of it runs at most one of them past its mean, which leaves n = ceil(lightWork / (budget - maxLight)). That denominator cannot fall below half the budget, so the count no longer runs to the cap when one test sits just under the budget either. Against the current map no product carries a test that large, so the real shard counts do not move. Claude-Session: https://claude.ai/code/session_01EJ1y3UzXR1BKtukE886ks9 --- .github/scripts/turbo-discover-sizing.test.js | 36 +++--- .github/scripts/turbo-discover.js | 108 ++++++++++-------- 2 files changed, 79 insertions(+), 65 deletions(-) diff --git a/.github/scripts/turbo-discover-sizing.test.js b/.github/scripts/turbo-discover-sizing.test.js index dabe83253e9e..c6404e6b97f6 100644 --- a/.github/scripts/turbo-discover-sizing.test.js +++ b/.github/scripts/turbo-discover-sizing.test.js @@ -96,7 +96,7 @@ test('buildMatrix splits a product to the shared wall target', () => { const matrix = buildMatrix(['big-one'], union, true) // 2000s of work, with the imbalance margin, over a (target - overhead) budget. - assert.equal(matrix.length, productSplitShards(2000, 50)) + assert.equal(matrix.length, productSplitShards({ work: 2000, heavyCount: 0, lightWork: 2000, maxLight: 50 })) assert.match(matrix[0].group, /^big-one \(1\/\d+\)$/) }) @@ -121,21 +121,27 @@ test('a single-invocation entry keeps the pre-legs keys for unrebased branches', assert.equal(matrix[0].pytest_args, '') }) -test('productSplitShards sizes the worst chunk, so a heavy test buys shards', () => { +test('productSplitShards sizes the worst chunk, not the mean', () => { const budget = TARGET_WALL_SECONDS - PRODUCT_JOB_OVERHEAD_SECONDS + const evenly = { work: 1000, heavyCount: 0, lightWork: 1000, maxLight: 10 } + const coarse = { work: 1000, heavyCount: 0, lightWork: 1000, maxLight: 150 } + // Same total work; the coarser grain cannot be cut as finely, so it needs more // shards to keep its worst chunk inside the budget. - const fine = productSplitShards(1000, 10) - const coarse = productSplitShards(1000, 200) - assert.ok(coarse > fine, `expected ${coarse} > ${fine}`) - assert.ok(1000 / fine + 10 <= budget) - assert.ok(1000 / coarse + 200 <= budget) - // A test at or above the budget cannot be split out of, so no shard count - // meets the target; size by work alone rather than buying useless shards. - assert.equal(productSplitShards(1000, budget * 2), Math.ceil(1000 / budget)) - // Just under the budget the bound goes uninformative and asks for a shard per - // few seconds of remainder. Stay near what the split actually needs. - assert.ok(productSplitShards(budget + 1, budget - 1) <= 3) + assert.ok(productSplitShards(coarse) > productSplitShards(evenly)) + assert.ok(evenly.lightWork / productSplitShards(evenly) + evenly.maxLight <= budget) + assert.ok(coarse.lightWork / productSplitShards(coarse) + coarse.maxLight <= budget) +}) + +test('tests above half the budget each hold a shard of their own', () => { + const budget = TARGET_WALL_SECONDS - PRODUCT_JOB_OVERHEAD_SECONDS + const heavy = Math.ceil(budget * 0.8) + + // Ten tests this size cannot pair, so no count below ten holds the budget, + // however the total work divides. + assert.equal(productSplitShards({ work: heavy * 10, heavyCount: 10, lightWork: 0, maxLight: 0 }), 10) + // One heavy test and a sliver takes two, not a shard per second of remainder. + assert.equal(productSplitShards({ work: budget + 1, heavyCount: 1, lightWork: 1, maxLight: 1 }), 2) }) test('a product that fits one shard is packed, not split by its own margin', () => { @@ -147,7 +153,7 @@ test('a product that fits one shard is packed, not split by its own margin', () } assert.ok(300 <= TARGET_WALL_SECONDS - PRODUCT_JOB_OVERHEAD_SECONDS) - assert.equal(productSplitShards(300, 30), 1) + assert.equal(productSplitShards({ work: 300, heavyCount: 0, lightWork: 300, maxLight: 30 }), 1) const matrix = buildMatrix(['mid-one'], union, true) @@ -162,7 +168,7 @@ test("a split product's last shard absorbs a small product without leaking split } union['products/small_one/backend/test_s.py::test_s'] = 40 - assert.equal(productSplitShards(330, 30), 2) + assert.equal(productSplitShards({ work: 330, heavyCount: 0, lightWork: 330, maxLight: 30 }), 2) const matrix = buildMatrix(['big-one', 'small-one'], union, true) diff --git a/.github/scripts/turbo-discover.js b/.github/scripts/turbo-discover.js index 3c8b6289c0d5..b60fd87d0ab6 100644 --- a/.github/scripts/turbo-discover.js +++ b/.github/scripts/turbo-discover.js @@ -708,19 +708,37 @@ function getProductDuration(product, durations) { // The longest single test in a product. pytest-split cuts between tests, never // inside one, so this is the irreducible grain of any split and it bounds how // far the worst chunk can run past the mean. -function getProductMaxTest(product, durations) { +// Budget of test work one product shard can hold, mirroring calculateShards. +function productShardBudget() { + return Math.max(TARGET_WALL_SECONDS - PRODUCT_JOB_OVERHEAD_SECONDS, PRODUCT_JOB_OVERHEAD_SECONDS / 2, 1) +} + +// The parts of a product's duration distribution that sizing needs. Two tests +// longer than half a shard's budget can never share a shard, so those are counted +// rather than summed; the rest are summed, with their own longest, because a +// contiguous chunk of them runs at most one of them past the mean. +function getProductShape(product, durations) { + const shape = { work: 0, maxTest: 0, heavyCount: 0, lightWork: 0, maxLight: 0 } if (!durations) { - return 0 + return shape } const prefix = productPrefix(product) const excluded = PRODUCTS_RUNNING_TEMPORAL_IN_JOB.has(product) ? [] : EXCLUDED_PATH_SEGMENTS - let longest = 0 + const heavyThreshold = productShardBudget() / 2 for (const [test, dur] of Object.entries(durations)) { - if (test.startsWith(prefix) && !excluded.some((seg) => test.includes(seg)) && dur > longest) { - longest = dur + if (!test.startsWith(prefix) || excluded.some((seg) => test.includes(seg))) { + continue + } + shape.work += dur + shape.maxTest = Math.max(shape.maxTest, dur) + if (dur > heavyThreshold) { + shape.heavyCount += 1 + } else { + shape.lightWork += dur + shape.maxLight = Math.max(shape.maxLight, dur) } } - return longest + return shape } // One definition of a product's work estimate, shared by the split decision @@ -734,25 +752,27 @@ function getProductMaxTest(product, durations) { // under-sharding. `staleUnionWork` is non-null exactly when the guess replaced // the recorded sum, so the caller can log it once. function resolveProductSizing(product, durations, productsScaled = false) { - const unionWork = getProductDuration(product, durations) - const maxTest = getProductMaxTest(product, durations) - if (productsScaled && unionWork > 0) { - return { work: unionWork, maxTest, staleUnionWork: null, staleness: null } + const shape = getProductShape(product, durations) + if (productsScaled && shape.work > 0) { + return { ...shape, staleUnionWork: null, staleness: null } } const staleness = checkProductStaleness(product, durations) if (staleness.stale && staleness.fileCount > 0) { const fallbackWork = staleness.fileCount * STALENESS_FALLBACK_SECONDS_PER_FILE - if (fallbackWork > unionWork) { - // The guess has no per-test shape, so assume one file's worth is one test. + if (fallbackWork > shape.work) { + // The guess carries no distribution, so treat it as one file's worth per test. return { work: fallbackWork, - maxTest: Math.max(maxTest, STALENESS_FALLBACK_SECONDS_PER_FILE), - staleUnionWork: unionWork, + maxTest: Math.max(shape.maxTest, STALENESS_FALLBACK_SECONDS_PER_FILE), + heavyCount: 0, + lightWork: fallbackWork, + maxLight: STALENESS_FALLBACK_SECONDS_PER_FILE, + staleUnionWork: shape.work, staleness, } } } - return { work: unionWork, maxTest, staleUnionWork: null, staleness: null } + return { ...shape, staleUnionWork: null, staleness: null } } function productEffectiveCost(product, durations, productsScaled = false) { @@ -877,44 +897,31 @@ function calculateShards(totalWorkSeconds, overheadSeconds, minShards = DJANGO_M return Math.max(minShards, Math.min(DJANGO_MAX_SHARDS, shards)) } -// Budget of test work one product shard can hold, mirroring calculateShards. -function productShardBudget() { - return Math.max(TARGET_WALL_SECONDS - PRODUCT_JOB_OVERHEAD_SECONDS, PRODUCT_JOB_OVERHEAD_SECONDS / 2, 1) -} - // Shards for one product. Sizing a split by work/n sizes the MEAN shard, but the -// run's wall is the MAX shard, and pytest-split cuts between tests: a contiguous -// split's worst chunk runs at most one whole test past the mean. So size the -// worst chunk directly -- work/n + maxTest <= budget -- which rearranges to -// n = ceil(work / (budget - maxTest)). +// run's wall is the MAX shard, and pytest-split cuts between tests rather than +// inside one, so size the worst chunk instead. +// +// Split the suite at half the budget. Two tests above that cannot share a shard +// at all, so each takes one and they set a floor no packing goes below. What is +// left is at most half a budget per test, so a contiguous chunk of it runs at +// most one such test past its mean, giving lightWork/n + maxLight <= budget and +// so n = ceil(lightWork / (budget - maxLight)). That denominator is at least +// half the budget, so it cannot collapse. // -// This replaces a fitted margin. A ratio measured off CI describes one map, and -// a map that misreports test weights (a floor on tiny tests, say) bakes its own -// error into the constant. Deriving from maxTest ties the sizing to the map: a -// product carrying one heavy test gets the shards that test forces, an evenly -// grained one gets none it does not need, and a fixed point is unnecessary. +// Reading the distribution rather than a fitted ratio ties the sizing to the +// map: a suite of heavy tests gets the shards they force, an evenly grained one +// gets none it does not need, and no constant carries a past map's error. // -// n = 1 is checked first because the bound does not apply to it -- an unsplit -// product's chunk is its whole work, with no extra test on top. -function productSplitShards(workSeconds, maxTestSeconds = 0) { +// A product whose whole suite fits one shard is not split, and the bound does +// not apply to it -- an unsplit chunk is the work itself, with nothing on top. +function productSplitShards(shape) { const budget = productShardBudget() - if (workSeconds <= budget) { + const { work = 0, heavyCount = 0, lightWork = 0, maxLight = 0 } = shape ?? {} + if (work <= budget) { return 1 } - if (maxTestSeconds >= budget) { - // One test already overruns a shard's budget, so no split can hold the - // target. Size by work alone rather than buying shards that cannot help. - return Math.max(2, Math.min(DJANGO_MAX_SHARDS, Math.ceil(workSeconds / budget))) - } - const headroom = budget - maxTestSeconds - if (headroom < budget / 4) { - // The mean-plus-one-test bound stops being informative once a single test - // eats most of the budget: it asks for a shard per few seconds of the - // remainder. Size by work and add the one shard that carries the heavy - // test, which is what the split actually needs. - return Math.max(2, Math.min(DJANGO_MAX_SHARDS, Math.ceil(workSeconds / budget) + 1)) - } - return Math.max(2, Math.min(DJANGO_MAX_SHARDS, Math.ceil(workSeconds / headroom))) + const lightShards = lightWork > 0 ? Math.ceil(lightWork / (budget - maxLight)) : 0 + return Math.max(2, Math.min(DJANGO_MAX_SHARDS, heavyCount + lightShards)) } // Selector segment key -> Django matrix segment name. @@ -1096,7 +1103,8 @@ function buildMatrix(products, durations, productsScaled = false) { // PRODUCTS_SCALED_MARKER: call-only durations undercount a fixture-heavy // suite several-fold, and sizing an unscaled sum under-shards it. for (const product of products) { - const { work, maxTest, staleUnionWork, staleness } = resolveProductSizing(product, durations, productsScaled) + const sizing = resolveProductSizing(product, durations, productsScaled) + const { work, maxTest, staleUnionWork, staleness } = sizing if (staleUnionWork !== null) { console.error( ` ${product}: .test_durations stale, ${staleness.coveredCount}/${staleness.fileCount} test files covered ` + @@ -1108,7 +1116,7 @@ function buildMatrix(products, durations, productsScaled = false) { ) } - const shards = productSplitShards(work, maxTest) + const shards = productSplitShards(sizing) if (shards > 1) { console.error(` ${product}: ${(work / 60).toFixed(1)} min work → split across ${shards} shards`) const filters = `--filter=@posthog/products-${product}` @@ -1177,7 +1185,7 @@ module.exports = { PRODUCT_JOB_OVERHEAD_SECONDS, PRODUCT_BUCKET_SAFETY_FACTOR, productSplitShards, - getProductMaxTest, + getProductShape, PRODUCTS_SCALED_MARKER, TARGET_WALL_SECONDS, DJANGO_OVERHEAD_SECONDS_BY_SEGMENT, From 5776c3b6d7abb204d88ff2e61c94e117f8b8052b Mon Sep 17 00:00:00 2001 From: hpouillot Date: Thu, 27 Aug 2026 12:35:00 +0200 Subject: [PATCH 17/24] feat(error-tracking): simplify rule settings --- frontend/src/scenes/settings/SettingsMap.tsx | 28 +++++++++---------- .../ErrorTrackingConfigurationMovedBanner.tsx | 2 +- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/frontend/src/scenes/settings/SettingsMap.tsx b/frontend/src/scenes/settings/SettingsMap.tsx index 58251fc46f89..a7965bd3d509 100644 --- a/frontend/src/scenes/settings/SettingsMap.tsx +++ b/frontend/src/scenes/settings/SettingsMap.tsx @@ -563,13 +563,6 @@ export const SETTINGS_MAP: SettingSection[] = [ component: , keywords: ['notification', 'alert', 'threshold', 'spike'], }, - { - id: 'error-tracking-suppression-rules', - title: 'Suppression rules', - description: 'Filter out exceptions that match the given filters.', - component: , - keywords: ['filter', 'ignore', 'suppress', 'exception', 'type', 'message'], - }, { id: 'error-tracking-spike-detection', title: 'Spike detection', @@ -584,10 +577,17 @@ export const SETTINGS_MAP: SettingSection[] = [ }, { id: 'error-tracking-auto-assignment', - title: 'Auto assignment rules', + title: 'Assignment rules', description: 'Automatically assign errors to team members based on rules you define.', component: , - keywords: ['assign', 'owner', 'team', 'rule', 'routing'], + keywords: ['assign', 'auto', 'owner', 'team', 'rule', 'routing'], + }, + { + id: 'error-tracking-custom-grouping', + title: 'Grouping rules', + description: 'Define rules for how errors are grouped together into issues.', + component: , + keywords: ['group', 'custom', 'merge', 'fingerprint', 'dedup'], }, { id: 'error-tracking-severity-rules', @@ -598,11 +598,11 @@ export const SETTINGS_MAP: SettingSection[] = [ keywords: ['severity', 'priority', 'triage', 'critical', 'rule'], }, { - id: 'error-tracking-custom-grouping', - title: 'Custom grouping rules', - description: 'Define rules for how errors are grouped together into issues.', - component: , - keywords: ['group', 'merge', 'fingerprint', 'dedup'], + id: 'error-tracking-suppression-rules', + title: 'Suppression rules', + description: 'Filter out exceptions that match the given filters.', + component: , + keywords: ['filter', 'ignore', 'suppress', 'exception', 'type', 'message'], }, { id: 'error-tracking-symbol-sets', diff --git a/frontend/src/scenes/settings/environment/ErrorTrackingConfigurationMovedBanner.tsx b/frontend/src/scenes/settings/environment/ErrorTrackingConfigurationMovedBanner.tsx index fb5b0aa20720..054f0aac4d68 100644 --- a/frontend/src/scenes/settings/environment/ErrorTrackingConfigurationMovedBanner.tsx +++ b/frontend/src/scenes/settings/environment/ErrorTrackingConfigurationMovedBanner.tsx @@ -7,7 +7,7 @@ export function ErrorTrackingConfigurationMovedBanner(): JSX.Element {

Error tracking configuration has moved. Configurations for alerting, suppression rules, - spike detection, auto assignment, custom grouping, symbol sets, and releases are now on the{' '} + spike detection, assignment rules, grouping rules, symbol sets, and releases are now on the{' '} Error tracking configuration page.

From e95bda93319910dbf0d88d74deaf38dbc20633a6 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 27 Aug 2026 12:44:57 +0200 Subject: [PATCH 18/24] fix(devex): charge a split for the fragmentation contiguous cuts force Sizing counted a heavy test as taking a shard and left the rest to pack around it, but the cuts are contiguous: a heavy test between light tests divides the light run instead of lifting out of it. Ordered durations of 53, 301 and 133 seconds sized at two shards, and either cut leaves a chunk of 354 or 434 seconds against a 320-second budget. H heavy tests leave at most H + 1 light runs and each run rounds up on its own, so the light side can cost H shards beyond its own bound. Charge that whenever any light work exists. This is the worst case rather than the actual one, because the sizer reads the map and not collection order, and it only costs shards on a product holding a test above half the budget. Against the current map no product holds one, so the real shard counts do not move. The dedicated-bucket comment kept a record of why batch-exports was once listed. That is operational detail with a shelf life; only the condition for belonging in the set stays. Claude-Session: https://claude.ai/code/session_01EJ1y3UzXR1BKtukE886ks9 --- .github/scripts/turbo-discover-sizing.test.js | 12 ++++++++++-- .github/scripts/turbo-discover.js | 18 +++++++++++------- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/.github/scripts/turbo-discover-sizing.test.js b/.github/scripts/turbo-discover-sizing.test.js index c6404e6b97f6..5e40ca3cf227 100644 --- a/.github/scripts/turbo-discover-sizing.test.js +++ b/.github/scripts/turbo-discover-sizing.test.js @@ -140,8 +140,16 @@ test('tests above half the budget each hold a shard of their own', () => { // Ten tests this size cannot pair, so no count below ten holds the budget, // however the total work divides. assert.equal(productSplitShards({ work: heavy * 10, heavyCount: 10, lightWork: 0, maxLight: 0 }), 10) - // One heavy test and a sliver takes two, not a shard per second of remainder. - assert.equal(productSplitShards({ work: budget + 1, heavyCount: 1, lightWork: 1, maxLight: 1 }), 2) + // One heavy test and a sliver stays bounded rather than asking for a shard per + // second of remainder. Three, not two: the cuts are contiguous, so light work + // on both sides of the heavy test cannot be gathered into one chunk. + assert.equal(productSplitShards({ work: budget + 1, heavyCount: 1, lightWork: 1, maxLight: 1 }), 3) +}) + +test('a heavy test between light ones splits the light run', () => { + // Ordered [53, 301, 133] against a 320s budget: either contiguous cut leaves a + // 354s or 434s chunk, so two shards cannot hold the budget however it is cut. + assert.equal(productSplitShards({ work: 487, heavyCount: 1, lightWork: 186, maxLight: 133 }), 3) }) test('a product that fits one shard is packed, not split by its own margin', () => { diff --git a/.github/scripts/turbo-discover.js b/.github/scripts/turbo-discover.js index b60fd87d0ab6..7df1ab03cdb8 100644 --- a/.github/scripts/turbo-discover.js +++ b/.github/scripts/turbo-discover.js @@ -102,12 +102,10 @@ const PRODUCTS_RUNNING_TEMPORAL_IN_JOB = new Set([ 'tasks', 'warehouse-sources', ]) -// Products that always get their own matrix entry instead of sharing one — -// isolates a flaky/hang-prone product so it can't cancel job-mates at the job -// timeout. Trade-off: a dedicated runner. Empty today: batch-exports was listed -// for an async-fixture teardown hang, and it now runs at the median product -// failure rate with no job near the timeout. Add a product here when its wall -// runs close enough to the job timeout that a hang is a realistic outcome. +// Products that always get their own matrix entry instead of sharing one, so a +// hang cannot cancel job-mates when the job timeout fires. The cost is a +// dedicated runner, so a product belongs here only while its wall runs close +// enough to the job timeout that a hang is a realistic outcome. const DEDICATED_BUCKET_PRODUCTS = new Set() // --- Staleness detection for .test_durations --- @@ -908,6 +906,11 @@ function calculateShards(totalWorkSeconds, overheadSeconds, minShards = DJANGO_M // so n = ceil(lightWork / (budget - maxLight)). That denominator is at least // half the budget, so it cannot collapse. // +// The cuts are contiguous, so a heavy test sitting between light ones divides +// the light run rather than lifting out of it. H heavy tests leave at most H + 1 +// light runs, and each run rounds up on its own, so the light side can cost H +// shards beyond its own bound. Charge that whenever any light work exists. +// // Reading the distribution rather than a fitted ratio ties the sizing to the // map: a suite of heavy tests gets the shards they force, an evenly grained one // gets none it does not need, and no constant carries a past map's error. @@ -921,7 +924,8 @@ function productSplitShards(shape) { return 1 } const lightShards = lightWork > 0 ? Math.ceil(lightWork / (budget - maxLight)) : 0 - return Math.max(2, Math.min(DJANGO_MAX_SHARDS, heavyCount + lightShards)) + const fragmentation = lightWork > 0 ? heavyCount : 0 + return Math.max(2, Math.min(DJANGO_MAX_SHARDS, heavyCount + lightShards + fragmentation)) } // Selector segment key -> Django matrix segment name. From 0fa13e0eff1de34233ae12dd896c6945a62b1113 Mon Sep 17 00:00:00 2001 From: Tue Haulund Date: Thu, 27 Aug 2026 12:56:28 +0200 Subject: [PATCH 19/24] fix(replay): bill every recording regardless of reported source and sdk The two billed replay meters keyed on $snapshot_source and $lib, both of which arrive from the client unvalidated. Combinations outside the expected set matched neither meter. Web is now the catch-all for the source, and the SDK no longer takes part in the mobile meter, so the two partition every session. --- .../test/__snapshots__/test_usage_report.ambr | 14 ++--- posthog/tasks/test/test_usage_report.py | 62 ++++++++++++++++--- posthog/tasks/usage_report.py | 16 +++-- 3 files changed, 69 insertions(+), 23 deletions(-) diff --git a/posthog/tasks/test/__snapshots__/test_usage_report.ambr b/posthog/tasks/test/__snapshots__/test_usage_report.ambr index da8b2f78c636..647e506742f2 100644 --- a/posthog/tasks/test/__snapshots__/test_usage_report.ambr +++ b/posthog/tasks/test/__snapshots__/test_usage_report.ambr @@ -940,7 +940,7 @@ WHERE min_first_timestamp >= '2022-01-10 00:00:00' AND min_first_timestamp < '2022-01-10 23:59:59' GROUP BY session_id - HAVING ifNull(argMinMerge(snapshot_source), 'web') == 'web' + HAVING (ifNull(argMinMerge(snapshot_source), 'web') == 'mobile') == 0 AND max(is_deleted) = 0) WHERE session_id NOT IN (SELECT DISTINCT session_id @@ -987,7 +987,7 @@ WHERE min_first_timestamp >= '2022-01-10 00:00:00' AND min_first_timestamp < '2022-01-10 23:59:59' GROUP BY session_id - HAVING ifNull(argMinMerge(snapshot_source), 'web') == 'web' + HAVING (ifNull(argMinMerge(snapshot_source), 'web') == 'mobile') == 0 AND max(is_deleted) = 0) WHERE session_id NOT IN (SELECT DISTINCT session_id @@ -1045,7 +1045,7 @@ WHERE min_first_timestamp >= '2022-01-10 00:00:00' AND min_first_timestamp < '2022-01-10 23:59:59' GROUP BY session_id - HAVING ifNull(argMinMerge(snapshot_source), 'web') == 'mobile' + HAVING (ifNull(argMinMerge(snapshot_source), 'web') == 'mobile') == 1 AND max(is_deleted) = 0) WHERE session_id NOT IN (SELECT DISTINCT session_id @@ -1069,7 +1069,7 @@ WHERE min_first_timestamp >= '2022-01-10 00:00:00' AND min_first_timestamp < '2022-01-10 23:59:59' GROUP BY session_id - HAVING ifNull(argMinMerge(snapshot_source), 'web') == 'mobile' + HAVING (ifNull(argMinMerge(snapshot_source), 'web') == 'mobile') == 1 AND max(is_deleted) = 0) WHERE session_id NOT IN (SELECT DISTINCT session_id @@ -1092,11 +1092,7 @@ WHERE min_first_timestamp >= '2022-01-10 00:00:00' AND min_first_timestamp < '2022-01-10 23:59:59' GROUP BY session_id - HAVING (ifNull(argMinMerge(snapshot_source), '') == 'mobile' - AND ifNull(argMinMerge(snapshot_library), '') IN ('posthog-ios', - 'posthog-android', - 'posthog-react-native', - 'posthog-flutter')) + HAVING ifNull(argMinMerge(snapshot_source), '') == 'mobile' AND max(is_deleted) = 0) WHERE session_id NOT IN (SELECT DISTINCT session_id diff --git a/posthog/tasks/test/test_usage_report.py b/posthog/tasks/test/test_usage_report.py index 132852a232ba..30d9efb8109c 100644 --- a/posthog/tasks/test/test_usage_report.py +++ b/posthog/tasks/test/test_usage_report.py @@ -1179,19 +1179,32 @@ def setUp(self) -> None: def test_usage_report_replay(self) -> None: _setup_replay_data(self.team.pk, include_mobile_replay=False) + # `$snapshot_source` reaches ClickHouse unvalidated, so anything that is not mobile has to + # land on the web meter. An equality on 'web' here would bill this session under neither. + timestamp = now() - relativedelta(hours=12) + produce_replay_summary( + team_id=self.team.pk, + session_id="unrecognized-snapshot-source", + distinct_id=str(uuid4()), + first_timestamp=timestamp, + last_timestamp=timestamp + timedelta(seconds=1), + snapshot_source="not-a-real-source", + size=10, + ) + period = get_previous_day() all_reports = _get_all_usage_data_as_team_rows(period.start, period.end) report = _get_team_report(all_reports, self.team) - assert report.recording_count_in_period == 5 + assert report.recording_count_in_period == 6 assert report.mobile_recording_count_in_period == 0 assert report.zero_duration_recording_count_in_period == 0 org_reports: dict[str, OrgReport] = {} _add_team_report_to_org_reports(org_reports, self.team, report, period.start) - assert org_reports[str(self.organization.id)].recording_count_in_period == 5 + assert org_reports[str(self.organization.id)].recording_count_in_period == 6 assert org_reports[str(self.organization.id)].mobile_recording_count_in_period == 0 assert org_reports[str(self.organization.id)].mobile_billable_recording_count_in_period == 0 @@ -1228,13 +1241,14 @@ def test_usage_report_replay_with_mobile(self) -> None: # but we do split them out of the daily usage since that field is used assert report.recording_count_in_period == 5 assert report.mobile_recording_count_in_period == 1 - assert report.mobile_billable_recording_count_in_period == 0 + # Reports no library at all, and still bills: the client decides what to send here. + assert report.mobile_billable_recording_count_in_period == 1 org_reports: dict[str, OrgReport] = {} _add_team_report_to_org_reports(org_reports, self.team, report, period.start) assert org_reports[str(self.organization.id)].recording_count_in_period == 5 assert org_reports[str(self.organization.id)].mobile_recording_count_in_period == 1 - assert org_reports[str(self.organization.id)].mobile_billable_recording_count_in_period == 0 + assert org_reports[str(self.organization.id)].mobile_billable_recording_count_in_period == 1 @also_test_with_materialized_columns(event_properties=["$lib", "$exception_values"], verify_no_jsonextract=False) def test_usage_report_replay_with_billable_mobile(self) -> None: @@ -1276,17 +1290,49 @@ def test_usage_report_replay_with_billable_mobile(self) -> None: all_reports = _get_all_usage_data_as_team_rows(period.start, period.end) report = _get_team_report(all_reports, self.team) - # Regular mobile recordings (non-billable) + billable ones assert report.recording_count_in_period == 5 # web recordings - assert report.mobile_recording_count_in_period == 4 # 1 non-billable + 2 billable + 1 from _setup_replay_data - assert report.mobile_billable_recording_count_in_period == 2 # iOS and Android recordings + assert report.mobile_recording_count_in_period == 4 + # All four bill, the one naming a library we do not ship included: `$lib` is whatever the + # caller sent, so letting it decide leaves a session no meter charges for. + assert report.mobile_billable_recording_count_in_period == 4 org_reports: dict[str, OrgReport] = {} _add_team_report_to_org_reports(org_reports, self.team, report, period.start) assert org_reports[str(self.organization.id)].recording_count_in_period == 5 assert org_reports[str(self.organization.id)].mobile_recording_count_in_period == 4 - assert org_reports[str(self.organization.id)].mobile_billable_recording_count_in_period == 2 + assert org_reports[str(self.organization.id)].mobile_billable_recording_count_in_period == 4 + + @also_test_with_materialized_columns(event_properties=["$lib", "$exception_values"], verify_no_jsonextract=False) + def test_usage_report_replay_bills_every_recording_exactly_once(self) -> None: + # `$snapshot_source` and `$lib` arrive from the client and are never validated, so the two + # billed meters have to partition every recording between them. A combination that matched + # neither would be a recording anyone could ask for and not be charged for. + crafted = [ + ("plain-web", "web", "web"), + ("mobile-shipped-sdk", "mobile", "posthog-ios"), + ("mobile-unshipped-sdk", "mobile", "posthog-unity"), + ("mobile-invented-sdk", "mobile", "not-a-real-sdk"), + ("invented-source", "not-a-real-source", "web"), + ] + timestamp = now() - relativedelta(hours=12) + for session_id, snapshot_source, snapshot_library in crafted: + produce_replay_summary( + team_id=self.team.pk, + session_id=session_id, + distinct_id=str(uuid4()), + first_timestamp=timestamp, + last_timestamp=timestamp + timedelta(seconds=1), + snapshot_source=snapshot_source, + snapshot_library=snapshot_library, + size=10, + ) + + period = get_previous_day() + report = _get_team_report(_get_all_usage_data_as_team_rows(period.start, period.end), self.team) + + billed = report.recording_count_in_period + report.mobile_billable_recording_count_in_period + assert billed == len(crafted) @also_test_with_materialized_columns(event_properties=["$lib", "$exception_values"], verify_no_jsonextract=False) def test_usage_report_replay_excludes_deleted_recordings(self) -> None: diff --git a/posthog/tasks/usage_report.py b/posthog/tasks/usage_report.py index 582ee446f527..c884db590256 100644 --- a/posthog/tasks/usage_report.py +++ b/posthog/tasks/usage_report.py @@ -1080,7 +1080,7 @@ def get_teams_with_recording_count_in_period( FROM session_replay_events WHERE min_first_timestamp >= %(begin)s AND min_first_timestamp < %(end)s GROUP BY session_id - HAVING ifNull(argMinMerge(snapshot_source), 'web') == %(snapshot_source)s + HAVING (ifNull(argMinMerge(snapshot_source), 'web') == 'mobile') == %(want_mobile)s AND max(is_deleted) = 0 ) WHERE session_id NOT IN ( @@ -1100,7 +1100,9 @@ def get_teams_with_recording_count_in_period( "previous_begin": previous_begin, "begin": begin, "end": end, - "snapshot_source": snapshot_source, + # Web is the catch-all, not an equality on 'web'. `$snapshot_source` is client-supplied + # and unvalidated, so the two meters have to partition every session between them. + "want_mobile": 1 if snapshot_source == "mobile" else 0, }, workload=Workload.OFFLINE, settings=CH_BILLING_SETTINGS, @@ -1183,6 +1185,7 @@ def get_teams_with_zero_duration_recording_count_in_period(begin: datetime, end: @timed_log() @retry(tries=QUERY_RETRIES, delay=QUERY_RETRY_DELAY, backoff=QUERY_RETRY_BACKOFF) def get_teams_with_mobile_billable_recording_count_in_period(begin: datetime, end: datetime) -> list[tuple[int, int]]: + """Mobile recordings in the period; the client-reported SDK does not affect billing.""" previous_begin = begin - (end - begin) with tags_context(product=Product.MOBILE_REPLAY, feature=Feature.USAGE_REPORT): @@ -1194,8 +1197,7 @@ def get_teams_with_mobile_billable_recording_count_in_period(begin: datetime, en FROM session_replay_events WHERE min_first_timestamp >= %(begin)s AND min_first_timestamp < %(end)s GROUP BY session_id - HAVING (ifNull(argMinMerge(snapshot_source), '') == 'mobile' - AND ifNull(argMinMerge(snapshot_library), '') IN ('posthog-ios', 'posthog-android', 'posthog-react-native', 'posthog-flutter')) + HAVING ifNull(argMinMerge(snapshot_source), '') == 'mobile' AND max(is_deleted) = 0 ) WHERE session_id NOT IN ( @@ -2335,7 +2337,7 @@ def get_teams_with_recording_bytes_in_period( FROM session_replay_events WHERE min_first_timestamp >= %(begin)s AND min_first_timestamp < %(end)s GROUP BY session_id - HAVING ifNull(argMinMerge(snapshot_source), 'web') == %(snapshot_source)s + HAVING (ifNull(argMinMerge(snapshot_source), 'web') == 'mobile') == %(want_mobile)s AND max(is_deleted) = 0 ) WHERE session_id NOT IN ( @@ -2355,7 +2357,9 @@ def get_teams_with_recording_bytes_in_period( "previous_begin": previous_begin, "begin": begin, "end": end, - "snapshot_source": snapshot_source, + # Web is the catch-all, not an equality on 'web'. `$snapshot_source` is client-supplied + # and unvalidated, so the two meters have to partition every session between them. + "want_mobile": 1 if snapshot_source == "mobile" else 0, }, workload=Workload.OFFLINE, settings=CH_BILLING_SETTINGS, From e411acb88a5d2691eacb89410b99cf02f1563675 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 27 Aug 2026 12:57:19 +0200 Subject: [PATCH 20/24] fix(devex): stop the refresh publishing a map with no product entries Standing in an empty products slice when the cache missed let the refresh succeed and publish. That map becomes the newest cache entry and the latest successful artifact, so it shadows the older valid one and every consumer drops to the file-count estimate, under-sharding the fixture-heavy products until a later refresh lands. Fail the refresh instead and leave the older map newest. Reaching that needs both a cache miss and a Products slice that fails its drift check, unlike the unconditional checkout this branch started from. Shard sizing also charged for a fragmentation the suite may not have, and the charge could ask for more shards than the product has tests. A shard past that point collects nothing and spends a runner without shortening the critical path, so the count is capped at the test count. Claude-Session: https://claude.ai/code/session_01EJ1y3UzXR1BKtukE886ks9 --- .../ci-backend-update-test-timing.yml | 11 ++++--- .github/scripts/turbo-discover-sizing.test.js | 31 +++++++++++++------ .github/scripts/turbo-discover.js | 13 ++++++-- .../ci-backend-update-test-timing.yml | 11 ++++--- 4 files changed, 43 insertions(+), 23 deletions(-) diff --git a/.depot/workflows/ci-backend-update-test-timing.yml b/.depot/workflows/ci-backend-update-test-timing.yml index bca2381b61d6..df868de18ea1 100644 --- a/.depot/workflows/ci-backend-update-test-timing.yml +++ b/.depot/workflows/ci-backend-update-test-timing.yml @@ -116,11 +116,12 @@ jobs: 'with_entries(select((.key | startswith("products/")) and ($fresh[0][.key] == null)))' \ /tmp/previous_durations > /tmp/products_durations else - # The cache missed, so there is no slice to stand in. Publishing the - # other segments without product entries beats failing this step and - # publishing nothing at all. - echo "::warning::No previous map to stand in; this refresh carries no product entries." - echo '{}' > /tmp/products_durations + # No cached slice to stand in and no fresh one. A map with no product + # entries would still become the newest cache and the latest successful + # artifact, so every consumer would fall back to the file-count estimate + # and under-shard the fixture-heavy products. Leave the older map newest. + echo "::error::Products slice failed with no previous map to stand in; refusing to publish a map without product entries" + exit 1 fi fi uv run .github/scripts/optimize_test_durations.py .test_durations \ diff --git a/.github/scripts/turbo-discover-sizing.test.js b/.github/scripts/turbo-discover-sizing.test.js index 5e40ca3cf227..3d69cbf375c2 100644 --- a/.github/scripts/turbo-discover-sizing.test.js +++ b/.github/scripts/turbo-discover-sizing.test.js @@ -96,7 +96,7 @@ test('buildMatrix splits a product to the shared wall target', () => { const matrix = buildMatrix(['big-one'], union, true) // 2000s of work, with the imbalance margin, over a (target - overhead) budget. - assert.equal(matrix.length, productSplitShards({ work: 2000, heavyCount: 0, lightWork: 2000, maxLight: 50 })) + assert.equal(matrix.length, productSplitShards({ work: 2000, heavyCount: 0, lightWork: 2000, maxLight: 50, testCount: 40 })) assert.match(matrix[0].group, /^big-one \(1\/\d+\)$/) }) @@ -123,8 +123,8 @@ test('a single-invocation entry keeps the pre-legs keys for unrebased branches', test('productSplitShards sizes the worst chunk, not the mean', () => { const budget = TARGET_WALL_SECONDS - PRODUCT_JOB_OVERHEAD_SECONDS - const evenly = { work: 1000, heavyCount: 0, lightWork: 1000, maxLight: 10 } - const coarse = { work: 1000, heavyCount: 0, lightWork: 1000, maxLight: 150 } + const evenly = { work: 1000, heavyCount: 0, lightWork: 1000, maxLight: 10, testCount: 100 } + const coarse = { work: 1000, heavyCount: 0, lightWork: 1000, maxLight: 150, testCount: 100 } // Same total work; the coarser grain cannot be cut as finely, so it needs more // shards to keep its worst chunk inside the budget. @@ -139,17 +139,28 @@ test('tests above half the budget each hold a shard of their own', () => { // Ten tests this size cannot pair, so no count below ten holds the budget, // however the total work divides. - assert.equal(productSplitShards({ work: heavy * 10, heavyCount: 10, lightWork: 0, maxLight: 0 }), 10) + assert.equal(productSplitShards({ work: heavy * 10, heavyCount: 10, lightWork: 0, maxLight: 0, testCount: 10 }), 10) // One heavy test and a sliver stays bounded rather than asking for a shard per - // second of remainder. Three, not two: the cuts are contiguous, so light work - // on both sides of the heavy test cannot be gathered into one chunk. - assert.equal(productSplitShards({ work: budget + 1, heavyCount: 1, lightWork: 1, maxLight: 1 }), 3) + // second of remainder. Two tests cannot fill three shards, so the count stops + // there rather than planning one that collects nothing. + assert.equal( + productSplitShards({ work: budget + 1, heavyCount: 1, lightWork: 1, maxLight: 1, testCount: 2 }), + 2 + ) +}) + +test('the count never exceeds the tests there are to place', () => { + // The fragmentation charge assumes a shape the suite may not have. Past the + // test count a shard collects nothing and spends a runner for it. + const many = { work: 10000, heavyCount: 3, lightWork: 100, maxLight: 10, testCount: 5 } + + assert.equal(productSplitShards(many), 5) }) test('a heavy test between light ones splits the light run', () => { // Ordered [53, 301, 133] against a 320s budget: either contiguous cut leaves a // 354s or 434s chunk, so two shards cannot hold the budget however it is cut. - assert.equal(productSplitShards({ work: 487, heavyCount: 1, lightWork: 186, maxLight: 133 }), 3) + assert.equal(productSplitShards({ work: 487, heavyCount: 1, lightWork: 186, maxLight: 133, testCount: 3 }), 3) }) test('a product that fits one shard is packed, not split by its own margin', () => { @@ -161,7 +172,7 @@ test('a product that fits one shard is packed, not split by its own margin', () } assert.ok(300 <= TARGET_WALL_SECONDS - PRODUCT_JOB_OVERHEAD_SECONDS) - assert.equal(productSplitShards({ work: 300, heavyCount: 0, lightWork: 300, maxLight: 30 }), 1) + assert.equal(productSplitShards({ work: 300, heavyCount: 0, lightWork: 300, maxLight: 30, testCount: 10 }), 1) const matrix = buildMatrix(['mid-one'], union, true) @@ -176,7 +187,7 @@ test("a split product's last shard absorbs a small product without leaking split } union['products/small_one/backend/test_s.py::test_s'] = 40 - assert.equal(productSplitShards({ work: 330, heavyCount: 0, lightWork: 330, maxLight: 30 }), 2) + assert.equal(productSplitShards({ work: 330, heavyCount: 0, lightWork: 330, maxLight: 30, testCount: 11 }), 2) const matrix = buildMatrix(['big-one', 'small-one'], union, true) diff --git a/.github/scripts/turbo-discover.js b/.github/scripts/turbo-discover.js index 7df1ab03cdb8..0c63db9c0252 100644 --- a/.github/scripts/turbo-discover.js +++ b/.github/scripts/turbo-discover.js @@ -716,7 +716,7 @@ function productShardBudget() { // rather than summed; the rest are summed, with their own longest, because a // contiguous chunk of them runs at most one of them past the mean. function getProductShape(product, durations) { - const shape = { work: 0, maxTest: 0, heavyCount: 0, lightWork: 0, maxLight: 0 } + const shape = { work: 0, maxTest: 0, heavyCount: 0, lightWork: 0, maxLight: 0, testCount: 0 } if (!durations) { return shape } @@ -728,6 +728,7 @@ function getProductShape(product, durations) { continue } shape.work += dur + shape.testCount += 1 shape.maxTest = Math.max(shape.maxTest, dur) if (dur > heavyThreshold) { shape.heavyCount += 1 @@ -765,6 +766,7 @@ function resolveProductSizing(product, durations, productsScaled = false) { heavyCount: 0, lightWork: fallbackWork, maxLight: STALENESS_FALLBACK_SECONDS_PER_FILE, + testCount: Math.max(shape.testCount, staleness.fileCount), staleUnionWork: shape.work, staleness, } @@ -911,6 +913,10 @@ function calculateShards(totalWorkSeconds, overheadSeconds, minShards = DJANGO_M // light runs, and each run rounds up on its own, so the light side can cost H // shards beyond its own bound. Charge that whenever any light work exists. // +// That charge assumes a fragmentation the suite may not have, so cap the count +// at the number of tests. Past it a shard is guaranteed to collect nothing +// (pytest exit 5) and spends a runner without shortening the critical path. +// // Reading the distribution rather than a fitted ratio ties the sizing to the // map: a suite of heavy tests gets the shards they force, an evenly grained one // gets none it does not need, and no constant carries a past map's error. @@ -919,13 +925,14 @@ function calculateShards(totalWorkSeconds, overheadSeconds, minShards = DJANGO_M // not apply to it -- an unsplit chunk is the work itself, with nothing on top. function productSplitShards(shape) { const budget = productShardBudget() - const { work = 0, heavyCount = 0, lightWork = 0, maxLight = 0 } = shape ?? {} + const { work = 0, heavyCount = 0, lightWork = 0, maxLight = 0, testCount = Infinity } = shape ?? {} if (work <= budget) { return 1 } const lightShards = lightWork > 0 ? Math.ceil(lightWork / (budget - maxLight)) : 0 const fragmentation = lightWork > 0 ? heavyCount : 0 - return Math.max(2, Math.min(DJANGO_MAX_SHARDS, heavyCount + lightShards + fragmentation)) + const wanted = Math.min(heavyCount + lightShards + fragmentation, testCount) + return Math.max(2, Math.min(DJANGO_MAX_SHARDS, wanted)) } // Selector segment key -> Django matrix segment name. diff --git a/.github/workflows/ci-backend-update-test-timing.yml b/.github/workflows/ci-backend-update-test-timing.yml index 5948445bbf6b..ca341a4899aa 100644 --- a/.github/workflows/ci-backend-update-test-timing.yml +++ b/.github/workflows/ci-backend-update-test-timing.yml @@ -280,11 +280,12 @@ jobs: 'with_entries(select((.key | startswith("products/")) and ($fresh[0][.key] == null)))' \ /tmp/previous_durations > /tmp/products_durations else - # The cache missed, so there is no slice to stand in. Publishing the - # other segments without product entries beats failing this step and - # publishing nothing at all. - echo "::warning::No previous map to stand in; this refresh carries no product entries." - echo '{}' > /tmp/products_durations + # No cached slice to stand in and no fresh one. A map with no product + # entries would still become the newest cache and the latest successful + # artifact, so every consumer would fall back to the file-count estimate + # and under-shard the fixture-heavy products. Leave the older map newest. + echo "::error::Products slice failed with no previous map to stand in; refusing to publish a map without product entries" + exit 1 fi fi From 0946abcb1415c0cc317f9c7c8961d87b1c16d6b5 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 27 Aug 2026 13:09:03 +0200 Subject: [PATCH 21/24] fix(devex): let the test count outrank the two-shard floor The floor ran after the cap and undid it, so a product holding one test that overruns the budget planned two jobs. The second is guaranteed to collect nothing, and no split shortens the first, so cap the floor itself. The stale fallback also dropped the recorded heavy tests and re-estimated everything as five-second light work. Those recordings are measurements even where coverage is poor, and a test above half the budget holds a shard whatever the file count says. Keep them, and treat only the guessed remainder as light. Claude-Session: https://claude.ai/code/session_01EJ1y3UzXR1BKtukE886ks9 --- .github/scripts/turbo-discover-sizing.test.js | 11 +++++++++++ .github/scripts/turbo-discover.js | 16 +++++++++++----- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/.github/scripts/turbo-discover-sizing.test.js b/.github/scripts/turbo-discover-sizing.test.js index 3d69cbf375c2..6ba450e302be 100644 --- a/.github/scripts/turbo-discover-sizing.test.js +++ b/.github/scripts/turbo-discover-sizing.test.js @@ -157,6 +157,17 @@ test('the count never exceeds the tests there are to place', () => { assert.equal(productSplitShards(many), 5) }) +test('a product holding one test is never split', () => { + const budget = TARGET_WALL_SECONDS - PRODUCT_JOB_OVERHEAD_SECONDS + + // One test over the budget still gets one job: a second would collect nothing, + // and no split shortens the first. + assert.equal( + productSplitShards({ work: budget + 1, heavyCount: 1, lightWork: 0, maxLight: 0, testCount: 1 }), + 1 + ) +}) + test('a heavy test between light ones splits the light run', () => { // Ordered [53, 301, 133] against a 320s budget: either contiguous cut leaves a // 354s or 434s chunk, so two shards cannot hold the budget however it is cut. diff --git a/.github/scripts/turbo-discover.js b/.github/scripts/turbo-discover.js index 0c63db9c0252..b548f33a5019 100644 --- a/.github/scripts/turbo-discover.js +++ b/.github/scripts/turbo-discover.js @@ -759,13 +759,16 @@ function resolveProductSizing(product, durations, productsScaled = false) { if (staleness.stale && staleness.fileCount > 0) { const fallbackWork = staleness.fileCount * STALENESS_FALLBACK_SECONDS_PER_FILE if (fallbackWork > shape.work) { - // The guess carries no distribution, so treat it as one file's worth per test. + // The tests the map does record are still measurements, and a heavy one + // holds a shard whatever the coverage. Keep those and treat only the + // guessed remainder as light, at one file's worth per test. + const recordedHeavyWork = shape.work - shape.lightWork return { work: fallbackWork, maxTest: Math.max(shape.maxTest, STALENESS_FALLBACK_SECONDS_PER_FILE), - heavyCount: 0, - lightWork: fallbackWork, - maxLight: STALENESS_FALLBACK_SECONDS_PER_FILE, + heavyCount: shape.heavyCount, + lightWork: Math.max(fallbackWork - recordedHeavyWork, 0), + maxLight: Math.max(shape.maxLight, STALENESS_FALLBACK_SECONDS_PER_FILE), testCount: Math.max(shape.testCount, staleness.fileCount), staleUnionWork: shape.work, staleness, @@ -932,7 +935,10 @@ function productSplitShards(shape) { const lightShards = lightWork > 0 ? Math.ceil(lightWork / (budget - maxLight)) : 0 const fragmentation = lightWork > 0 ? heavyCount : 0 const wanted = Math.min(heavyCount + lightShards + fragmentation, testCount) - return Math.max(2, Math.min(DJANGO_MAX_SHARDS, wanted)) + // The two-shard floor cannot outrank the test count: a product holding one + // test that overruns the budget still gets one job, because the second would + // collect nothing and splitting cannot shorten the first. + return Math.max(Math.min(2, testCount), Math.min(DJANGO_MAX_SHARDS, wanted)) } // Selector segment key -> Django matrix segment name. From 1e267281f2c6ee881196d45f4bb4ab5128056662 Mon Sep 17 00:00:00 2001 From: Tue Haulund Date: Thu, 27 Aug 2026 13:09:27 +0200 Subject: [PATCH 22/24] chore(replay): state the mobile meter as the exact complement of the web one --- posthog/tasks/test/__snapshots__/test_usage_report.ambr | 2 +- posthog/tasks/usage_report.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/posthog/tasks/test/__snapshots__/test_usage_report.ambr b/posthog/tasks/test/__snapshots__/test_usage_report.ambr index 647e506742f2..f9b8c596b151 100644 --- a/posthog/tasks/test/__snapshots__/test_usage_report.ambr +++ b/posthog/tasks/test/__snapshots__/test_usage_report.ambr @@ -1092,7 +1092,7 @@ WHERE min_first_timestamp >= '2022-01-10 00:00:00' AND min_first_timestamp < '2022-01-10 23:59:59' GROUP BY session_id - HAVING ifNull(argMinMerge(snapshot_source), '') == 'mobile' + HAVING (ifNull(argMinMerge(snapshot_source), 'web') == 'mobile') == 1 AND max(is_deleted) = 0) WHERE session_id NOT IN (SELECT DISTINCT session_id diff --git a/posthog/tasks/usage_report.py b/posthog/tasks/usage_report.py index c884db590256..3793091ce5f5 100644 --- a/posthog/tasks/usage_report.py +++ b/posthog/tasks/usage_report.py @@ -1197,7 +1197,7 @@ def get_teams_with_mobile_billable_recording_count_in_period(begin: datetime, en FROM session_replay_events WHERE min_first_timestamp >= %(begin)s AND min_first_timestamp < %(end)s GROUP BY session_id - HAVING ifNull(argMinMerge(snapshot_source), '') == 'mobile' + HAVING (ifNull(argMinMerge(snapshot_source), 'web') == 'mobile') == 1 AND max(is_deleted) = 0 ) WHERE session_id NOT IN ( From a2b1544579b04243753ea1e852237f5e7251f33c Mon Sep 17 00:00:00 2001 From: "posthog[bot]" <206114724+posthog[bot]@users.noreply.github.com> Date: Thu, 27 Aug 2026 11:10:58 +0000 Subject: [PATCH 23/24] chore(visual): update storybook baselines 2 updated Run: 4591d94f-63cd-4f14-b77b-0701f885d6ba Co-authored-by: hpouillot <3455883+hpouillot@users.noreply.github.com> --- frontend/snapshots.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/snapshots.yml b/frontend/snapshots.yml index 9738d1f6a6ef..0fef3d31f141 100644 --- a/frontend/snapshots.yml +++ b/frontend/snapshots.yml @@ -7913,9 +7913,9 @@ snapshots: scenes-app-settings-environment--settings-environment-error-tracking--light: hash: v1.k794b7964.5ac167271f00184ce6159d0072ce163542658b08419f6e70b5cf6a6ca201a718.X6dKpM6uGw_g_FAwdQ5PukTVcKWGAa77bMOzgw-kiXY scenes-app-settings-environment--settings-environment-error-tracking-configuration--dark: - hash: v1.k794b7964.033e5499811363c36dee07950ff09f33413e64b81c8522859c85748bc0443e3f.Hz66ItOfiG51KAAT2yidV4lINJmLINi-9_k81Ygdk80 + hash: v1.k794b7964.3d7a3006130bdc66e1456cb81566e21bb2179f19d17caf91ead7a41ba4be9234.tbIKl_Tlb1T7RBNR3Ic9n51t6kNpTkGsw28sXZHW-Gs scenes-app-settings-environment--settings-environment-error-tracking-configuration--light: - hash: v1.k794b7964.56f8265ff5de3733a4d00b0dbde7c0fff36ccdc886f713881134159802a13d39.9ArXuAkoxa6dyVQ_uspbjHWKM8YIgCPgCbDUnLHsxso + hash: v1.k794b7964.7d62fa47841096cceaa13c9a7cb15c1152b2f929ee030674bc0d97c5bc66c239.GptZ3dbdhoABnDP-dWhaqUCQOTeKYOsMFokkyWuNjwQ scenes-app-settings-environment--settings-environment-feature-flags--dark: hash: v1.k794b7964.56670f556ffac0360ec521741db9822f4e545e25b45bd8ed4ad012da8885c30a.qj9dLuAFy8v0TwEH-UTymXnlWPe2eu-YjAOpe2zc3Q8 scenes-app-settings-environment--settings-environment-feature-flags--light: From ffd95df8fd2b3a614e985aea862e83b9421d581e Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 27 Aug 2026 13:27:03 +0200 Subject: [PATCH 24/24] chore(devex): correct four entries and widen the skill pointers The Person-table entry said `PERSON_TABLE_NAME` is not in the code. It is: #41620 merged from the same window and carried the setting to master, where `Person.Meta.db_table` reads it. The entry steered readers away from a switch that already exists. What did not land is the test setup and dual reads around it, so the entry stays and now says that. The xdist entry drew a 3-minute conclusion from a 6-minute measurement. The closing comment on #38927 reports ~15 min to ~9 min. The Blacksmith entry paraphrased a conclusion that lives only in an internal report. This repo is public, so the entry now carries the outcome the PRs show and stops there. Sparse checkout was labeled `rejected`, which this file defines as built and measured. Nobody reviewed that PR and the stale bot closed it, so `abandoned` is the verdict the taxonomy already has for it. Pointers went only to five CI skills, so a migration, endpoint, isolation, or stacking proposal never saw the sections written for it. Four more skills get the same one-line pointer. Claude-Session: https://claude.ai/code/session_011D2GDLMQGPxQLEkHxevuRs --- .agents/skills/django-migrations/SKILL.md | 2 ++ .../skills/improving-drf-endpoints/SKILL.md | 2 ++ .../SKILL.md | 2 ++ .agents/skills/stacking-prs/SKILL.md | 2 ++ docs/internal/ci-things-already-tried.md | 18 +++++++++++------- 5 files changed, 19 insertions(+), 7 deletions(-) diff --git a/.agents/skills/django-migrations/SKILL.md b/.agents/skills/django-migrations/SKILL.md index 0ae53452ad12..59a844de10ff 100644 --- a/.agents/skills/django-migrations/SKILL.md +++ b/.agents/skills/django-migrations/SKILL.md @@ -5,6 +5,8 @@ description: Django migration patterns and safety workflow for PostHog. Use when # Django migrations +Before you propose a change to the migration history or to how migrations run, check [things already tried](../../../docs/internal/ci-things-already-tried.md). It records the closed attempts at squashing the history and at the Person table cutover. + Read these files first, before writing or editing a migration: - `docs/published/handbook/engineering/developing-locally.md` (`## Django migrations`, `### Non-blocking migrations`, `### Resolving merge conflicts`) diff --git a/.agents/skills/improving-drf-endpoints/SKILL.md b/.agents/skills/improving-drf-endpoints/SKILL.md index 66a73d556e3e..b3e79013c04d 100644 --- a/.agents/skills/improving-drf-endpoints/SKILL.md +++ b/.agents/skills/improving-drf-endpoints/SKILL.md @@ -5,6 +5,8 @@ description: Use when editing, reviewing, or auditing DRF viewsets and serialize # Improving DRF Endpoints +Before you propose a contract test against the generated OpenAPI schema, check [things already tried](../../../docs/internal/ci-things-already-tried.md). Six PRs took that idea, and none merged. + ## Overview Serializer fields are the source of truth for PostHog's entire type pipeline: diff --git a/.agents/skills/isolating-product-facade-contracts/SKILL.md b/.agents/skills/isolating-product-facade-contracts/SKILL.md index 1f967eea5e99..8f0e73c45a78 100644 --- a/.agents/skills/isolating-product-facade-contracts/SKILL.md +++ b/.agents/skills/isolating-product-facade-contracts/SKILL.md @@ -5,6 +5,8 @@ description: Plan and execute product isolation migrations to a facade plus cont # Isolating a product with facade and contracts +Before you choose a product to isolate, check [things already tried](../../../docs/internal/ci-things-already-tried.md). It records which product was the first candidate, and why the field test moved to another one. + Use this skill to migrate an existing product to the isolated architecture used by Visual review. Optimize for short calendar exposure, not small diffs: authoring is cheap and the verification chain catches mechanical breakage, while human review latency and a fast-moving master are the diff --git a/.agents/skills/stacking-prs/SKILL.md b/.agents/skills/stacking-prs/SKILL.md index 60d893b92a4f..c121ade23e5e 100644 --- a/.agents/skills/stacking-prs/SKILL.md +++ b/.agents/skills/stacking-prs/SKILL.md @@ -12,6 +12,8 @@ description: > # Stacked PRs with `gh stack` +Before you divide a change into layers, check [things already tried](../../../docs/internal/ci-things-already-tried.md). It records when a stack cost more review effort than one PR. + GitHub's native stacked PRs are enabled on this repo. A stack is an ordered chain of PRs where each one targets the branch of the PR below it; the bottom PR targets `master`. GitHub tracks the chain as a first-class object: the PR UI shows a stack map, branch protections (code owner approval, required checks) apply to **every** layer including mid-stack ones, and CI that runs on `master` PRs runs on every layer. diff --git a/docs/internal/ci-things-already-tried.md b/docs/internal/ci-things-already-tried.md index 2bcf6e1e13da..2af104cc4372 100644 --- a/docs/internal/ci-things-already-tried.md +++ b/docs/internal/ci-things-already-tried.md @@ -64,7 +64,7 @@ Wall time decreased from approximately 15 minutes to approximately 9 minutes. Th CPU cost increased from 1,572 to 3,908 core-minutes. This is a factor of approximately 2.5. The speed increase is real. The cost is the problem. -A factor of 2.5 in compute is too much for 3 minutes of wall time. +A factor of 2.5 in compute is too much for 6 minutes of wall time. `pytest-xdist` is still a development dependency, and it operates correctly on a local machine. CI does not use it in the shards. @@ -251,8 +251,8 @@ _Also asked as:_ Docker Hub rate limit in CI, unauthenticated pull limit, DOCKER The trial did not do a direct exchange. It ran a Blacksmith shadow of most compute jobs on the same commit, behind the `BLACKSMITH_SHADOW_ENABLED` variable. Each shadow used `continue-on-error`, and no shadow was a required check. -The team kept Depot. The comparison did not give a clear answer. -The detail of that comparison is not in the PRs. It went to a report in the team channel, so this entry cannot show you the numbers. [#57991](https://github.com/PostHog/posthog/pull/57991) removed the shadow workflow and the matrix branches. +The team kept Depot. [#57991](https://github.com/PostHog/posthog/pull/57991) removed the shadow workflow and the matrix branches. +The measurements are not in the PRs, so this entry cannot show them. It kept `.github/scripts/compare-ci-runners.py` and marked the file as legacy. That script produced the numbers of the trial. If you propose this again, equalize the caches of the two providers first. A runner that keeps a warm cache between jobs measures the cache, not the compute. @@ -262,7 +262,7 @@ _Also asked as:_ change CI provider, Blacksmith, cheaper runners, are the Depot ### Use sparse-checkout on the large CI workflows -**Verdict: rejected for those workflows** · Oct 2025 · [#39239](https://github.com/PostHog/posthog/pull/39239) +**Verdict: abandoned** · Oct 2025 · [#39239](https://github.com/PostHog/posthog/pull/39239) The description covers the backend, frontend, and Rust workflows. The diff changes only `ci-backend.yml` and `ci-rust.yml`. @@ -416,7 +416,7 @@ _Also asked as:_ remove pydantic, use dataclasses for the schema, the schema imp ### Switch the Person model to the partitioned table with a Django setting -**Verdict: eight attempts, none merged** · Nov 2025 · [#41436](https://github.com/PostHog/posthog/pull/41436), [#41513](https://github.com/PostHog/posthog/pull/41513), [#41522](https://github.com/PostHog/posthog/pull/41522), [#41600](https://github.com/PostHog/posthog/pull/41600), [#41604](https://github.com/PostHog/posthog/pull/41604), [#41669](https://github.com/PostHog/posthog/pull/41669), [#41698](https://github.com/PostHog/posthog/pull/41698), [#41813](https://github.com/PostHog/posthog/pull/41813) +**Verdict: eight attempts closed unmerged** · Nov 2025 · [#41436](https://github.com/PostHog/posthog/pull/41436), [#41513](https://github.com/PostHog/posthog/pull/41513), [#41522](https://github.com/PostHog/posthog/pull/41522), [#41600](https://github.com/PostHog/posthog/pull/41600), [#41604](https://github.com/PostHog/posthog/pull/41604), [#41669](https://github.com/PostHog/posthog/pull/41669), [#41698](https://github.com/PostHog/posthog/pull/41698), [#41813](https://github.com/PostHog/posthog/pull/41813) The goal is to move the Person model from `posthog_person` to a table that is partitioned by `team_id`. Eight PRs tried five mechanisms. @@ -426,10 +426,14 @@ None of them merged. The author wrote this on [#41513](https://github.com/PostHo > I'm still not sure if I got on the wrong track here by wanting to bend all test setup to use the person_new table and other sqlx migrated stuff. It seems I overlooked something fundamental since things are failing so much. -The work continues, but the mechanism is different. `PERSON_TABLE_NAME` is not in the code. `posthog_person_new` survives in one Dagster job, with a comment about a future name swap. +One narrow PR from the same window did merge. [#41620](https://github.com/PostHog/posthog/pull/41620) put `team_id` into the Person queries that lacked it, and it carried the `PERSON_TABLE_NAME` setting to master. +The setting is in `posthog/settings/data_stores.py`, and `Person.Meta.db_table` reads it. It defaults to `posthog_person`, so the cutover is off. +The switch exists. The eight PRs above failed at what surrounds it: the test setup, the dual reads, and the partition guard. +`posthog_person_new` comes from the sqlx migrations in `rust/persons_migrations/`. Three Dagster jobs read it, and one carries a comment about a future name swap. Person and group data now goes through the gRPC client in `posthog/personhog_client/`. `AGENTS.md` makes that client the required interface and prohibits new ORM queries against the person tables. -Read this entry before you propose a Django-level cutover. The Django setting is not where this problem lives any more. +Read this entry before you propose a Django-level cutover. The setting is already there. +What is missing is everything that must be true before a person changes its value. _Also asked as:_ partition the person table, `PERSON_TABLE_NAME`, `posthog_person_new`, dual-table reads, cut over the Person model