diff --git a/.github/workflows/container-images-cd.yml b/.github/workflows/container-images-cd.yml index 55a02c41e87d..967afb5f24b3 100644 --- a/.github/workflows/container-images-cd.yml +++ b/.github/workflows/container-images-cd.yml @@ -515,6 +515,7 @@ jobs: selfDriving: - 'products/signals/backend/**' - 'products/signals/skills/**' + - 'products/autoresearch/backend/**' - 'posthog/settings/temporal.py' - 'posthog/temporal/common/**' - 'posthog/management/commands/start_temporal_worker.py' diff --git a/docs/internal/ai-observability-judge-inputs.md b/docs/internal/ai-observability-judge-inputs.md index c4812f7808b1..89adc2cb6752 100644 --- a/docs/internal/ai-observability-judge-inputs.md +++ b/docs/internal/ai-observability-judge-inputs.md @@ -39,6 +39,24 @@ They sample the combined input, tool definitions, and output only when that text Implementation: [trace judge](../../posthog/temporal/ai_observability/run_trace_evaluation.py), [session judge](../../posthog/temporal/ai_observability/run_session_evaluation.py), and [generation judge](../../posthog/temporal/ai_observability/evaluation_llm_judge.py). +## OpenAI-compatible judges + +Custom OpenAI-compatible connections use the shared DNS-pinned HTTPX transport with response bounds enabled. +Every completion request has a 60-second total HTTP deadline, including connection setup, response headers, and the body. +Key validation and model listing use a 10-second total deadline. +Responses, including errors and streamed completions, are limited to 1 MiB. +The endpoint must return uncompressed responses; compressed responses are rejected before decoding. +Expired requests, rejected responses, and streams closed by the caller close their underlying connection. + +The OpenAI SDK does not retry custom-provider requests. Online evaluations and taggers use their existing Temporal retry policies for transient failures, and worker cancellation propagates to Temporal. +Rate-limit responses retry without disabling the evaluation or marking its connection invalid, honoring `Retry-After` up to one minute. Quota and authentication errors keep their existing terminal behavior. +Models without native structured-output support retain the JSON fallback, which can make one additional bounded request. +Oversized or compressed completion responses skip the evaluation as a rejected request without disabling the connection. +The evaluation records the response limit and how to configure the endpoint. +These connection and response limits also apply when using the same provider in the playground. +Disconnecting from the playground releases the server's stream slot without waiting for an in-flight provider read. +The worker closes the connection when that read finishes or reaches the provider's deadline. + ## System One judges System One-compatible models are available under the existing LLM judge option. diff --git a/ee/hogai/utils/asgi.py b/ee/hogai/utils/asgi.py index a613ac8bb102..988d0d6c3a05 100644 --- a/ee/hogai/utils/asgi.py +++ b/ee/hogai/utils/asgi.py @@ -1,4 +1,5 @@ -from collections.abc import AsyncIterator, Callable, Iterable, Iterator +import threading +from collections.abc import AsyncIterator, Iterable, Iterator from typing import TypeVar from asgiref.sync import sync_to_async @@ -9,18 +10,45 @@ class SyncIterableToAsync(AsyncIterator[T]): def __init__(self, iterable: Iterable[T]) -> None: self._iterable: Iterable[T] = iterable - # async versions of the `next` and `iter` functions - self.next_async: Callable = sync_to_async(self.next, thread_sensitive=False) - self.iter_async: Callable = sync_to_async(iter, thread_sensitive=False) self.sync_iterator: Iterator[T] | None = None + self._lock = threading.Lock() + self._close_requested = threading.Event() + self._closed = False def __aiter__(self) -> AsyncIterator[T]: return self async def __anext__(self) -> T: - if self.sync_iterator is None: - self.sync_iterator = await self.iter_async(self._iterable) - return await self.next_async(self.sync_iterator) + return await sync_to_async(self._next, thread_sensitive=False)() + + def _next(self) -> T: + try: + with self._lock: + if self._close_requested.is_set(): + raise StopAsyncIteration + if self.sync_iterator is None: + self.sync_iterator = iter(self._iterable) + return self.next(self.sync_iterator) + finally: + if self._close_requested.is_set(): + self._close() + + def _close(self) -> None: + with self._lock: + if self._closed: + return + self._closed = True + iterator = self.sync_iterator if self.sync_iterator is not None else self._iterable + close = getattr(iterator, "close", None) + if close is not None: + close() + + async def aclose(self) -> None: + self._close_requested.set() + # Cancellation cannot interrupt a sync read, so its worker closes the iterator when the read finishes. + if self._lock.acquire(blocking=False): + self._lock.release() + await sync_to_async(self._close, thread_sensitive=False)() @staticmethod def next(it: Iterator[T]) -> T: diff --git a/ee/hogai/utils/test/test_asgi.py b/ee/hogai/utils/test/test_asgi.py new file mode 100644 index 000000000000..dccb4ba01b7a --- /dev/null +++ b/ee/hogai/utils/test/test_asgi.py @@ -0,0 +1,61 @@ +import asyncio +import threading +from collections.abc import Iterator + +import pytest + +from parameterized import parameterized + +from ee.hogai.utils.asgi import SyncIterableToAsync + + +class TestSyncIterableToAsync: + @parameterized.expand([("idle", False), ("reading", True)]) + async def test_closes_cancelled_stream_in_worker(self, _name: str, reading: bool) -> None: + loop = asyncio.get_running_loop() + read_started = asyncio.Event() + close_started = asyncio.Event() + closed = asyncio.Event() + release_read = threading.Event() + release_close = threading.Event() + closed_on: list[int] = [] + + def generate() -> Iterator[int]: + try: + yield 1 + loop.call_soon_threadsafe(read_started.set) + assert release_read.wait(5) + yield 2 + finally: + loop.call_soon_threadsafe(close_started.set) + assert release_close.wait(5) + closed_on.append(threading.get_ident()) + loop.call_soon_threadsafe(closed.set) + + stream = SyncIterableToAsync(generate()) + assert await anext(stream) == 1 + if reading: + pending = asyncio.create_task(anext(stream)) + await asyncio.wait_for(read_started.wait(), 5) + pending.cancel() + with pytest.raises(asyncio.CancelledError): + await pending + + cleanup = asyncio.create_task(stream.aclose()) + try: + if reading: + await asyncio.wait_for(cleanup, 5) + assert not close_started.is_set() + release_read.set() + await asyncio.wait_for(close_started.wait(), 5) + finally: + release_read.set() + release_close.set() + await asyncio.wait_for(cleanup, 5) + await asyncio.wait_for(closed.wait(), 5) + + assert len(closed_on) == 1 + assert closed_on[0] != threading.get_ident() + await stream.aclose() + with pytest.raises(StopAsyncIteration): + await anext(stream) diff --git a/frontend/src/layout/navigation-3000/Navigation.tsx b/frontend/src/layout/navigation-3000/Navigation.tsx index f4b27bd7d5e5..d0754c051b8a 100644 --- a/frontend/src/layout/navigation-3000/Navigation.tsx +++ b/frontend/src/layout/navigation-3000/Navigation.tsx @@ -134,7 +134,17 @@ export function Navigation({ } > {showMinimalNavigation && } -
{children}
+
+ {children} +
) } diff --git a/frontend/src/layout/navigation-3000/navigationLogic.tsx b/frontend/src/layout/navigation-3000/navigationLogic.tsx index 43ae74565e08..d1a5a3399409 100644 --- a/frontend/src/layout/navigation-3000/navigationLogic.tsx +++ b/frontend/src/layout/navigation-3000/navigationLogic.tsx @@ -5,6 +5,7 @@ import posthog from 'posthog-js' import { FEATURE_FLAGS } from 'lib/constants' import { featureFlagLogic } from 'lib/logic/featureFlagLogic' +import { isEmbeddedPageFrame } from 'lib/utils/embeddedPageFrame' import { onboardingVariantChrome, resolveOnboardingFlowVariant } from 'scenes/onboarding/onboardingVariants' import { organizationLogic } from 'scenes/organizationLogic' import { sceneLogic } from 'scenes/sceneLogic' @@ -13,7 +14,7 @@ import { Scene } from 'scenes/sceneTypes' import type { SceneConfig } from '../../scenes/sceneTypes' import { navigationLogic } from '../navigation/navigationLogic' -export type Navigation3000Mode = 'none' | 'minimal' | 'zen' | 'full' +export type Navigation3000Mode = 'none' | 'minimal' | 'zen' | 'embedded' | 'full' export type ZenModeTrigger = 'shortcut' | 'account_menu' | 'help_menu' | 'exit_button' | 'url' @@ -120,6 +121,10 @@ export const navigation3000Logic = kea([ activeSceneId: string | null, featureFlags: import('lib/logic/featureFlagLogic').FeatureFlagsSet ): Navigation3000Mode => { + // An embedded page frame sits inside another app page, so it shows the scene alone. + if (isEmbeddedPageFrame()) { + return 'embedded' + } if (zenMode) { return 'zen' } diff --git a/frontend/src/layout/today/TodaySpaceActions.tsx b/frontend/src/layout/today/TodaySpaceActions.tsx index eb25cb5cd974..be98e43cbe9f 100644 --- a/frontend/src/layout/today/TodaySpaceActions.tsx +++ b/frontend/src/layout/today/TodaySpaceActions.tsx @@ -16,7 +16,7 @@ import { urls } from 'scenes/urls' import { ChannelDTOApi } from 'products/tasks/frontend/generated/api.schemas' import { TodayMenuParts } from './todayMenuParts' -import { spaceNewSessionUrl, todaySpacesLogic } from './todaySpacesLogic' +import { todaySpacesLogic } from './todaySpacesLogic' interface TodaySpaceActionsProps { parts: TodayMenuParts @@ -42,7 +42,7 @@ export function TodaySpaceActions({ return ( <> - + New session diff --git a/frontend/src/layout/today/TodaySpacesSidebar.tsx b/frontend/src/layout/today/TodaySpacesSidebar.tsx index 2f4df95edd75..eb268a24d277 100644 --- a/frontend/src/layout/today/TodaySpacesSidebar.tsx +++ b/frontend/src/layout/today/TodaySpacesSidebar.tsx @@ -163,11 +163,11 @@ export function TodaySpacesSidebar(): JSX.Element { variant="primary" size="lg" className="w-full" - render={} + render={} data-attr="today-spaces-new-chat" > - New chat + New session
{hasPinned && ( @@ -220,7 +220,7 @@ export function TodaySpacesSidebar(): JSX.Element { loadError('Recent sessions didn’t load.', loadRecentTasks, 'today-recent-retry') ) : recentState === 'empty' ? ( - Sessions and chats you open show up here. Start one with New chat. + Sessions and chats you open show up here. Start one with New session. ) : recentState === 'no-matches' ? ( notice( diff --git a/frontend/src/layout/today/todaySpacesLogic.ts b/frontend/src/layout/today/todaySpacesLogic.ts index 55ae7cb80772..a96a5df72523 100644 --- a/frontend/src/layout/today/todaySpacesLogic.ts +++ b/frontend/src/layout/today/todaySpacesLogic.ts @@ -1,10 +1,11 @@ import { MakeLogicType, actions, afterMount, connect, kea, listeners, path, reducers, selectors } from 'kea' import { loaders } from 'kea-loaders' -import { combineUrl, router } from 'kea-router' +import { router } from 'kea-router' import type { LocationChangedPayload } from 'kea-router/lib/types' import { toast } from '@posthog/quill' +import { removeProjectIdIfPresent } from 'lib/utils/kea-router' import { writeToClipboard } from 'lib/utils/writeToClipboard' import { maxGlobalLogic } from 'scenes/max/maxGlobalLogic' import { teamLogic } from 'scenes/teamLogic' @@ -79,11 +80,10 @@ const SPACE_PRESENCE_POLL_INTERVAL_MS = 90_000 export type TodayWorkSectionId = 'pinned' | 'recent' | 'spaces' -/** The space page reads this search param once and focuses its new-session composer. */ -export const SPACE_COMPOSE_PARAM = 'compose' - -export function spaceNewSessionUrl(spaceId: string): string { - return combineUrl(urls.taskSpace(spaceId), { [SPACE_COMPOSE_PARAM]: 1 }).url +/** The space a path is in, like PostHog Desktop's scoped space. `/spaces/new` is in no space. */ +export function spaceIdForPath(pathname: string): string | null { + const match = removeProjectIdIfPresent(pathname).match(/^\/spaces\/([^/]+)/) + return match && match[1] !== 'new' ? match[1] : null } /** The personal space first, then the team's general space, then starred spaces, then the rest by name. */ @@ -133,6 +133,7 @@ export interface todaySpacesLogicValues { user: UserType | null // userLogic allRecentItems: TodayWorkItem[] collapsedSections: TodayWorkSectionId[] + lastSpaceId: string | null pendingSpaceIds: string[] pinnedItems: TodayWorkItem[] pinnedTasks: TaskListItemApi[] @@ -325,6 +326,9 @@ export interface todaySpacesLogicActions { setSectionHeights: (heights: Partial>) => { heights: Partial> } + spaceVisited: (spaceId: string) => { + spaceId: string + } starFailed: (spaceId: string) => { spaceId: string } @@ -408,6 +412,7 @@ export const todaySpacesLogic = kea([ toggleSection: (sectionId: TodayWorkSectionId) => ({ sectionId }), setSectionHeights: (heights: Partial>) => ({ heights }), resetSectionPair: (upper: TodayWorkSectionId, lower: TodayWorkSectionId) => ({ upper, lower }), + spaceVisited: (spaceId: string) => ({ spaceId }), setRecentQuery: (query: string) => ({ query }), setRecentSearchOpen: (open: boolean) => ({ open }), setRecentFilters: (filters: TodayRecentFilters) => ({ filters }), @@ -529,6 +534,8 @@ export const todaySpacesLogic = kea([ }, }, ], + // A generic New session files here, like PostHog Desktop's scoped space. A stale id falls back to personal. + lastSpaceId: [null as string | null, { persist: true }, { spaceVisited: (_, { spaceId }) => spaceId }], recentQuery: ['', { setRecentQuery: (_, { query }) => query, clearRecentSearchAndFilters: () => '' }], recentSearchOpen: [ false, @@ -709,7 +716,13 @@ export const todaySpacesLogic = kea([ } } return { - locationChanged: markOpenSessionRead, + locationChanged: ({ pathname }) => { + const spaceId = spaceIdForPath(pathname) + if (spaceId) { + actions.spaceVisited(spaceId) + } + markOpenSessionRead() + }, loadTaskActivitySuccess: markOpenSessionRead, loadRecentTasks: () => actions.loadTaskActivity(), markSessionRead: async ({ marker, activityIds }) => { @@ -758,6 +771,10 @@ export const todaySpacesLogic = kea([ }, })), afterMount(({ actions, cache }) => { + const spaceId = spaceIdForPath(router.values.location.pathname) + if (spaceId) { + actions.spaceVisited(spaceId) + } actions.loadSpaces() actions.loadPinnedTasks() actions.loadRecentTasks() diff --git a/frontend/src/lib/constants.tsx b/frontend/src/lib/constants.tsx index ffbc2db468f2..7bc5e17d881c 100644 --- a/frontend/src/lib/constants.tsx +++ b/frontend/src/lib/constants.tsx @@ -423,7 +423,6 @@ export const FEATURE_FLAGS = { METRICS: 'metrics', // owner: #team-apm (@jonmcwest, @frankh) METRICS_DASHBOARD_PANELS: 'metrics-dashboard-panels', // owner: #team-apm — gates the stat/gauge/bargauge/table panel picker entries METRICS_ERROR_OVERLAYS: 'metrics-error-overlays', // owner: #team-apm — gates the error-spike overlay PoC on metrics charts - METRICS_FUNDAMENTALS: 'metrics-fundamentals', // owner: #team-apm (@jonmcwest, @frankh), gates the Fundamentals tab and the explain API behind it, which check the metrics viewer's own reductions ML_INFERENCE_DECISIONS: 'ml-inference-decisions', // owner: #team-ai-research, gates the decisions playground; the API checks the same flag server side NEW_TAB_PROJECT_EXPLORER: 'new-tab-project-explorer', // owner: #team-platform-ux NEW_TEAM_CORE_EVENTS: 'new-team-core-events', // owner: @jabahamondes #team-web-analytics diff --git a/frontend/src/lib/utils/embeddedPageFrame.ts b/frontend/src/lib/utils/embeddedPageFrame.ts new file mode 100644 index 000000000000..0a1998e47526 --- /dev/null +++ b/frontend/src/lib/utils/embeddedPageFrame.ts @@ -0,0 +1,19 @@ +/** The name of a frame that shows an app page inside another app page, such as a cited object in a task. */ +export const EMBEDDED_PAGE_FRAME_NAME = 'posthog-embedded-page' + +/** + * True when this document is an app page inside an embedded page frame, so it renders without the app's + * navigation. The frame name stays when the page navigates inside the frame. The origin check makes sure + * only the app itself can turn the mode on. + */ +export function isEmbeddedPageFrame(): boolean { + if (typeof window === 'undefined' || window.name !== EMBEDDED_PAGE_FRAME_NAME || window.parent === window) { + return false + } + try { + return window.parent.location.origin === window.location.origin + } catch { + // A cross-origin parent throws on any read of its location. + return false + } +} diff --git a/frontend/src/loadPostHogJS.tsx b/frontend/src/loadPostHogJS.tsx index 07e5659e0ea9..c044dc3a2700 100644 --- a/frontend/src/loadPostHogJS.tsx +++ b/frontend/src/loadPostHogJS.tsx @@ -3,6 +3,7 @@ import posthog, { BeforeSendFn, BrowserMetricsConfig, PostHogConfig, SessionReco import { FEATURE_FLAGS } from 'lib/constants' import { isOAuthMode } from 'lib/oauth/oauthClient' import { inStorybook, inStorybookTestRunner } from 'lib/utils/dom' +import { isEmbeddedPageFrame } from 'lib/utils/embeddedPageFrame' import { getAppContext } from 'lib/utils/getAppContext' import { startDetachedElementTracking } from './detachedElementTracker' @@ -54,6 +55,10 @@ export function withLastSeenFeatureFlags( return { ...bootstrap, featureFlags: { ...lastSeen.featureFlags, ...bootstrap.featureFlags } } } +// pinned: analytics property name. Insights filter the framed pages by it. +const stampEmbeddedPageFrame: BeforeSendFn = (event) => + event && { ...event, properties: { ...event.properties, embedded_page_frame: true } } + function readLastSeenFeatureFlags(): LastSeenFeatureFlags | null { try { const stored = window.localStorage.getItem(LAST_SEEN_FEATURE_FLAGS_KEY) @@ -119,7 +124,11 @@ export function loadPostHogJS(options: LoadPostHogJSOptions = {}): void { __capturePostHogExceptions: true, }, metrics: { network: true, serviceName: 'posthog-app', ...options.metrics }, - before_send: options.beforeSend, + // A page in a frame counts its own pageviews, so its events say so and analysis can filter them. + // `register` would persist the property in storage the main window shares, so it is stamped per event. + before_send: isEmbeddedPageFrame() + ? [stampEmbeddedPageFrame, ...(options.beforeSend ? [options.beforeSend].flat() : [])] + : options.beforeSend, loaded: (loadedInstance) => { if (loadedInstance.sessionRecording) { loadedInstance.sessionRecording._forceAllowLocalhostNetworkCapture = true diff --git a/frontend/src/productScenes.tsx b/frontend/src/productScenes.tsx index 598b8ff41c8b..51fc0bbca85b 100644 --- a/frontend/src/productScenes.tsx +++ b/frontend/src/productScenes.tsx @@ -174,6 +174,7 @@ export const productScenes: Record Promise> = { Subscription: () => import('../../products/subscriptions/frontend/scenes/SubscriptionScene'), SlackTaskContext: () => import('../../products/tasks/frontend/SlackTaskContextScene'), TaskSpaces: () => import('../../products/tasks/frontend/spaces/SpacesScene'), + TaskNewSession: () => import('../../products/tasks/frontend/spaces/NewSessionScene'), TaskSpace: () => import('../../products/tasks/frontend/spaces/SpaceScene'), Tracing: () => import('../../products/tracing/frontend/TracingScene'), TracingOperation: () => import('../../products/tracing/frontend/TracingOperationScene'), diff --git a/frontend/src/products.tsx b/frontend/src/products.tsx index 343b487e4c25..b4f907fb7aae 100644 --- a/frontend/src/products.tsx +++ b/frontend/src/products.tsx @@ -284,8 +284,10 @@ export const productRoutes: Record = { '/subscriptions/:subscriptionId': ['Subscription', 'subscription'], '/slack-task-context': ['SlackTaskContext', 'slackTaskContext'], '/spaces': ['TaskSpaces', 'taskSpaces'], + '/spaces/new': ['TaskNewSession', 'taskNewSession'], '/spaces/:id': ['TaskSpace', 'taskSpace'], '/spaces/:id/canvases': ['TaskSpace', 'taskSpaceCanvases'], + '/spaces/:id/new': ['TaskNewSession', 'taskSpaceNewSession'], '/spaces/:id/settings': ['TaskSpace', 'taskSpaceSettings'], '/tracing': ['Tracing', 'tracing'], '/tracing/operation': ['TracingOperation', 'tracingOperation'], @@ -1106,6 +1108,7 @@ export const productConfiguration: Record = { }, SlackTaskContext: { name: 'Slack task context', projectBased: true }, TaskSpaces: { name: 'Spaces', projectBased: true }, + TaskNewSession: { name: 'New session', projectBased: true }, TaskSpace: { name: 'Space', projectBased: true }, Toolbar: { name: 'Toolbar', @@ -1756,8 +1759,10 @@ export const productUrls = { `/surveys/guided/${id}${template ? `?template=${encodeURIComponent(template)}` : ''}`, slackTaskContext: (): string => '/slack-task-context', taskSpaces: (): string => '/spaces', + taskNewSession: (): string => '/spaces/new', taskSpace: (id: string): string => `/spaces/${id}`, taskSpaceCanvases: (id: string): string => `/spaces/${id}/canvases`, + taskSpaceNewSession: (id: string): string => `/spaces/${id}/new`, taskSpaceSettings: (id: string): string => `/spaces/${id}/settings`, toolbarLaunch: (): string => '/toolbar', tracing: (): string => '/tracing', diff --git a/frontend/src/scenes/AuthenticatedShell.tsx b/frontend/src/scenes/AuthenticatedShell.tsx index bd96c75f29c5..e1fe39cea206 100644 --- a/frontend/src/scenes/AuthenticatedShell.tsx +++ b/frontend/src/scenes/AuthenticatedShell.tsx @@ -9,6 +9,7 @@ import { ToastCloseButton } from 'lib/lemon-ui/LemonToast/LemonToast' import { apiStatusLogic } from 'lib/logic/apiStatusLogic' import { eventIngestionRestrictionLogic } from 'lib/logic/eventIngestionRestrictionLogic' import { featureFlagLogic } from 'lib/logic/featureFlagLogic' +import { isEmbeddedPageFrame } from 'lib/utils/embeddedPageFrame' import { lazyWithRetry } from 'lib/utils/retryImport' import { WizardHandoffDialog } from 'scenes/onboarding/shared/wizard-sync/WizardHandoffDialog' import { WizardSyncDebugPanel } from 'scenes/onboarding/shared/wizard-sync/WizardSyncDebugPanel' @@ -43,6 +44,28 @@ export default function AuthenticatedShell({ children }: { children: React.React const { featureFlags } = useValues(featureFlagLogic) const { isDarkModeOn } = useValues(themeLogic) const runSyncEnabled = featureFlags[FEATURE_FLAGS.WIZARD_RUN_SYNC] === 'wizard-run' + const toasts = ( + } + position="bottom-right" + theme={isDarkModeOn ? 'dark' : 'light'} + /> + ) + + // The page around the frame already has the command palette, shortcuts and floating buttons. + if (isEmbeddedPageFrame()) { + return ( + <> +
+ {children} + +
+ {toasts} + + ) + } return ( <> @@ -71,13 +94,7 @@ export default function AuthenticatedShell({ children }: { children: React.React
)}
- } - position="bottom-right" - theme={isDarkModeOn ? 'dark' : 'light'} - /> + {toasts} ) } diff --git a/frontend/src/scenes/project-homepage/today/Today.stories.tsx b/frontend/src/scenes/project-homepage/today/Today.stories.tsx index e7d45a75d1de..97e99946b4e6 100644 --- a/frontend/src/scenes/project-homepage/today/Today.stories.tsx +++ b/frontend/src/scenes/project-homepage/today/Today.stories.tsx @@ -778,6 +778,11 @@ export const SpacePage: Story = { parameters: { pageUrl: urls.taskSpace('space-checkout') }, } +// New session opens this page. It files into the personal space until the user picks another one. +export const NewSessionPage: Story = { + parameters: { pageUrl: urls.taskNewSession() }, +} + // A Cmd-click pick can't be held in a static story, so the play step selects a pinned and a recent row through the logic. export const SpacesPaneWithSelectedSessions: Story = { parameters: { pageUrl: urls.taskSpace('space-checkout') }, diff --git a/frontend/src/scenes/project-homepage/today/TodayHomeSidebar.tsx b/frontend/src/scenes/project-homepage/today/TodayHomeSidebar.tsx index e5517a3d5d2b..edc2947b00c6 100644 --- a/frontend/src/scenes/project-homepage/today/TodayHomeSidebar.tsx +++ b/frontend/src/scenes/project-homepage/today/TodayHomeSidebar.tsx @@ -86,11 +86,11 @@ export function TodayHomeSidebar(): JSX.Element { variant="primary" size="lg" className="w-full" - render={} + render={} data-attr="today-new-chat" > - New chat + New session
Today
diff --git a/frontend/src/scenes/sceneTypes.ts b/frontend/src/scenes/sceneTypes.ts index 1c27205c11d0..5f1fcfb2f34e 100644 --- a/frontend/src/scenes/sceneTypes.ts +++ b/frontend/src/scenes/sceneTypes.ts @@ -260,6 +260,7 @@ export enum Scene { NewAction = 'NewAction', TaskTracker = 'TaskTracker', SlackTaskContext = 'SlackTaskContext', + TaskNewSession = 'TaskNewSession', TaskSpace = 'TaskSpace', TaskSpaces = 'TaskSpaces', OrganizationDeactivated = 'OrganizationDeactivated', diff --git a/playwright/e2e/data-quality-overview.spec.ts b/playwright/e2e/data-quality-overview.spec.ts index ca0fe7b274dc..335bb35fb68e 100644 --- a/playwright/e2e/data-quality-overview.spec.ts +++ b/playwright/e2e/data-quality-overview.spec.ts @@ -1,5 +1,5 @@ /** - * Editing and deleting a data quality check from the Data Ops overview. + * Editing and deleting a data quality check from the Models overview. */ import { expect } from '@playwright/test' @@ -10,7 +10,7 @@ import { test } from '../utils/workspace-test-base' const CHECK_NAME = 'orders_has_rows' const SUBJECT_NAME = 'orders_e2e' -test('edits and deletes a check from Data Ops', async ({ page, playwrightSetup }) => { +test('edits and deletes a check from Models', async ({ page, playwrightSetup }) => { const workspace = await playwrightSetup.createWorkspace({ skip_onboarding: true, no_demo_data: true }) const auth = { headers: { @@ -41,11 +41,10 @@ test('edits and deletes a check from Data Ops', async ({ page, playwrightSetup } const check = await created.json() await mockFeatureFlags(page, { - [FEATURE_FLAGS.DATA_WAREHOUSE_SCENE]: true, [FEATURE_FLAGS.DATA_QUALITY_CHECKS]: true, }) await playwrightSetup.loginAndNavigateToTeam(page, workspace) - await page.goto('/data-ops?tab=data-quality') + await page.goto('/models?tab=data-quality') await page.getByLabel(`Expand checks for ${SUBJECT_NAME}`).click({ timeout: 30000 }) await expect(page.getByText(CHECK_NAME)).toBeVisible() diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 31935246a363..a28514972bc2 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -5051,6 +5051,9 @@ importers: kea-subscriptions: specifier: 'catalog:' version: 3.0.1(kea@4.0.0-pre.6(patch_hash=139b8d1f1304f9d9da452a9a1244c94ea679dbcb85687d8999563146879fb6f5)(react@18.3.1)) + posthog-js: + specifier: 'catalog:' + version: 1.435.5(@types/react@18.3.27)(react@18.3.1) react: specifier: 18.3.1 version: 18.3.1 diff --git a/posthog/api/streaming.py b/posthog/api/streaming.py index c95411be64aa..43302d09d3d1 100644 --- a/posthog/api/streaming.py +++ b/posthog/api/streaming.py @@ -171,8 +171,10 @@ async def _instrumented_aiter( _record_stream_open(endpoint) started_at = time.monotonic() outcome = "completed" + iterator: AsyncIterator[bytes | str] | None = None try: - async for chunk in stream: + iterator = aiter(stream) + async for chunk in iterator: yield chunk except (GeneratorExit, asyncio.CancelledError): outcome = "client_disconnect" @@ -181,8 +183,13 @@ async def _instrumented_aiter( outcome = "error" raise finally: - _record_stream_close(endpoint, outcome, started_at) - reservation.release() + try: + close = getattr(iterator, "aclose", None) + if close is not None: + await close() + finally: + _record_stream_close(endpoint, outcome, started_at) + reservation.release() def _instrumented_iter( diff --git a/posthog/api/test/test_streaming.py b/posthog/api/test/test_streaming.py index a2e5a7edaa27..66ab438108d2 100644 --- a/posthog/api/test/test_streaming.py +++ b/posthog/api/test/test_streaming.py @@ -1,5 +1,6 @@ import gc import asyncio +import threading from collections.abc import AsyncGenerator, AsyncIterable, AsyncIterator, Iterator from http import HTTPStatus from typing import cast @@ -22,6 +23,8 @@ streaming_response, ) +from ee.hogai.utils.asgi import SyncIterableToAsync + def _gen() -> Iterator[bytes]: yield b"data: hello\n\n" @@ -144,9 +147,15 @@ async def agen(): assert _closed_total("test_async_complete", "completed") == 1.0 async def test_async_stream_early_close_counts_client_disconnect(self): + closed = False + async def endless(): - while True: - yield b": ping\n\n" + nonlocal closed + try: + while True: + yield b": ping\n\n" + finally: + closed = True # An abandoned async stream is aclosed by the event loop's async # generator finalizer, not by response.close() (Django's resource @@ -161,6 +170,7 @@ async def endless(): await inner.__anext__() assert _open_connections("test_async_disconnect") == 1.0 await inner.aclose() + assert closed assert _open_connections("test_async_disconnect") == 0.0 assert _closed_total("test_async_disconnect", "client_disconnect") == 1.0 assert streaming._active_stream_count == baseline @@ -177,36 +187,55 @@ def test_honors_content_type_and_does_not_inject_sse_headers(self): class TestSSEAsyncCancellation: - async def test_task_cancellation_counts_client_disconnect_not_error(self): - first_chunk_pulled = asyncio.Event() - - async def blocking(): + @pytest.mark.parametrize("synchronous", [False, True]) + async def test_task_cancellation_counts_client_disconnect_not_error(self, synchronous: bool) -> None: + loop = asyncio.get_running_loop() + read_started = asyncio.Event() + read_finished = asyncio.Event() + release_read = threading.Event() + + async def blocking() -> AsyncIterator[bytes]: yield b": ping\n\n" + read_started.set() await asyncio.Event().wait() # park forever; cancellation lands here + def blocking_sync() -> Iterator[bytes]: + try: + yield b": ping\n\n" + loop.call_soon_threadsafe(read_started.set) + assert release_read.wait(10) + yield b": ping\n\n" + finally: + loop.call_soon_threadsafe(read_finished.set) + # ASGI cancellation is a path where response.close() never runs, so the # generator's finally is the only thing releasing the cap slot; pin it # (baseline-relative: this test runs outside the slot-isolation fixture). baseline = streaming._active_stream_count - stream = _instrument_stream(blocking(), "test_async_cancel", _reserve_slot()) + endpoint = f"test_async_cancel_{synchronous}" + source = SyncIterableToAsync(blocking_sync()) if synchronous else blocking() + stream = _instrument_stream(source, endpoint, _reserve_slot()) assert isinstance(stream, AsyncIterable) - async def consume(): + async def consume() -> None: async for _ in stream: - first_chunk_pulled.set() + pass task = asyncio.ensure_future(consume()) - await first_chunk_pulled.wait() - assert _open_connections("test_async_cancel") == 1.0 - task.cancel() try: - await task - except asyncio.CancelledError: - pass - assert _open_connections("test_async_cancel") == 0.0 - assert _closed_total("test_async_cancel", "client_disconnect") == 1.0 - assert _closed_total("test_async_cancel", "error") == 0.0 - assert streaming._active_stream_count == baseline + await asyncio.wait_for(read_started.wait(), 5) + assert _open_connections(endpoint) == 1.0 + task.cancel() + with pytest.raises(asyncio.CancelledError): + await asyncio.wait_for(task, 5) + assert _open_connections(endpoint) == 0.0 + assert _closed_total(endpoint, "client_disconnect") == 1.0 + assert _closed_total(endpoint, "error") == 0.0 + assert streaming._active_stream_count == baseline + finally: + release_read.set() + if synchronous: + await asyncio.wait_for(read_finished.wait(), 5) class TestSSEConcurrencyCap: diff --git a/posthog/csp_middleware.py b/posthog/csp_middleware.py index ff5953c690a1..1e1f81a7f535 100644 --- a/posthog/csp_middleware.py +++ b/posthog/csp_middleware.py @@ -307,7 +307,10 @@ def __call__(self, request): # Enforced for every viewer, flag or not, because this directive is what admits these # origins: a frame-ancestors directive makes browsers ignore X-Frame-Options, which # names only our own origin. - frame_ancestors = f"frame-ancestors {' '.join(app_frame_ancestor_sources())}" + # `'self'` lets the app show one of its own pages in a frame, such as a PostHog object + # cited in a task's Artifacts tab. It stays out of app_frame_ancestor_sources(), because + # the canvas sandbox document on the user-content origin shares that list. + frame_ancestors = f"frame-ancestors 'self' {' '.join(app_frame_ancestor_sources())}" js_url = urlsplit(settings.JS_URL) bundle_origin = f"{js_url.scheme}://{js_url.netloc}" if js_url.scheme and js_url.netloc else "" if settings.DEBUG or settings.TEST: diff --git a/posthog/hogql/constants.py b/posthog/hogql/constants.py index 1baa49239c68..c5ce49c77d89 100644 --- a/posthog/hogql/constants.py +++ b/posthog/hogql/constants.py @@ -182,6 +182,7 @@ class HogQLQuerySettings(BaseModel): join_algorithm: Optional[str] = None grace_hash_join_initial_buckets: Optional[int] = None force_data_skipping_indices: Optional[list[str]] = None + force_optimize_projection: Optional[bool] = None load_balancing: Optional[str] = None format_csv_allow_double_quotes: Optional[bool] = None optimize_skip_unused_shards: Optional[bool] = None diff --git a/posthog/hogql/database/schema/metrics.py b/posthog/hogql/database/schema/metrics.py index 2ec8d870e7db..8062d9772a78 100644 --- a/posthog/hogql/database/schema/metrics.py +++ b/posthog/hogql/database/schema/metrics.py @@ -274,6 +274,9 @@ class MetricSeriesTable(Table): "last_seen": DateTimeDatabaseField( name="timestamp", nullable=False, description="Most recent sample timestamp seen for this series." ), + "time_bucket": DateTimeDatabaseField( + name="time_bucket", nullable=False, description="Start of the UTC hour that contains `last_seen`." + ), "original_expiry_timestamp": DateTimeDatabaseField( name="original_expiry_timestamp", nullable=False, description="When the series leaves retention." ), diff --git a/posthog/models/oauth.py b/posthog/models/oauth.py index d698c01af7cf..696aad334c61 100644 --- a/posthog/models/oauth.py +++ b/posthog/models/oauth.py @@ -1,5 +1,6 @@ import enum import uuid +from datetime import timedelta from typing import TYPE_CHECKING, cast from urllib.parse import urlparse @@ -27,7 +28,13 @@ from posthog.models.activity_logging.model_activity import ModelActivityMixin from posthog.models.user import User -from posthog.models.utils import UUIDT, generate_random_token, hash_key_value, mask_key_value +from posthog.models.utils import ( + UUIDT, + generate_random_oauth_access_token, + generate_random_token, + hash_key_value, + mask_key_value, +) if TYPE_CHECKING: from posthog.models import Organization, User @@ -647,6 +654,31 @@ class Meta(AbstractGrant.Meta): ) +def mint_oauth_access_token( + *, + application: OAuthApplication, + user: "User | None", + scope: str, + lifetime: timedelta, + scoped_teams: list[int], + sandbox_task_id: uuid.UUID | None = None, +) -> OAuthAccessToken: + """Mint a fresh access token directly, outside the OAuth grant flow. + + The caller owns the scope and lifetime decision. Callers that also issue a refresh token + or rotate an existing one create their rows by hand, inside their own transaction. + """ + return OAuthAccessToken.objects.create( + application=application, + user=user, + token=generate_random_oauth_access_token(None), + expires=timezone.now() + lifetime, + scope=scope, + scoped_teams=scoped_teams, + sandbox_task_id=sandbox_task_id, + ) + + def find_oauth_access_token(token: str) -> OAuthAccessToken | None: """Find an OAuth access token by its value using the token_checksum index.""" from hashlib import sha256 diff --git a/posthog/temporal/ai_observability/evaluation_llm_judge.py b/posthog/temporal/ai_observability/evaluation_llm_judge.py index 0d14df029fc8..1b7dad28fb87 100644 --- a/posthog/temporal/ai_observability/evaluation_llm_judge.py +++ b/posthog/temporal/ai_observability/evaluation_llm_judge.py @@ -55,8 +55,10 @@ ModelPermissionError, OutputTokenLimitError, ProviderConnectionError, + ProviderRequestRejectedError, QuotaExceededError, RateLimitError, + RetryableRateLimitError, StructuredOutputParseError, UnsupportedModelError, provider_error_detail, @@ -64,8 +66,6 @@ from products.ai_observability.backend.llm.system_one import ( SystemOneClient, SystemOneEndpointBlockedError, - SystemOneRateLimitError, - SystemOneRequestRejectedError, system_one_evaluations_enabled, ) from products.ai_observability.backend.llm.types import CompletionResponse @@ -735,7 +735,7 @@ def call_llm_judge( key_id=key_id, is_byok=is_byok, ) - except SystemOneRequestRejectedError as e: + except ProviderRequestRejectedError as e: increment_user_errors("request_rejected", provider=provider) return build_skipped_evaluation_result( output_type=output_type, @@ -743,7 +743,7 @@ def call_llm_judge( reasoning=str(e), skip_reason="request_rejected", ) - except SystemOneRateLimitError as e: + except RetryableRateLimitError as e: increment_errors("rate_limit", provider=provider) raise ApplicationError( str(e), diff --git a/posthog/temporal/ai_observability/run_tagger.py b/posthog/temporal/ai_observability/run_tagger.py index b5ffdeb2980b..b4300e5807cb 100644 --- a/posthog/temporal/ai_observability/run_tagger.py +++ b/posthog/temporal/ai_observability/run_tagger.py @@ -27,8 +27,10 @@ ModelNotFoundError, ModelPermissionError, OutputTokenLimitError, + ProviderRequestRejectedError, QuotaExceededError, RateLimitError, + RetryableRateLimitError, StructuredOutputParseError, ) from products.ai_observability.backend.models.provider_keys import LLMProviderKey @@ -49,6 +51,7 @@ TAGGER_DISABLED_ERROR_TYPE = "tagger_disabled" TAGGER_PARSE_ERROR_TYPE = "tagger_parse_error" +TAGGER_REQUEST_REJECTED_ERROR_TYPE = "tagger_request_rejected" # model_resolution is shared with evaluations, so the tagger types its skip reasons on the way out. MODEL_RESOLUTION_SKIP_ERROR_TYPES = { "provider_key_required": "tagger_provider_key_required", @@ -57,7 +60,12 @@ } # RunTaggerWorkflow turns these into a skipped result, so they must stay out of error tracking. SKIPPED_RESULT_ERROR_TYPES = frozenset( - {TAGGER_DISABLED_ERROR_TYPE, TAGGER_PARSE_ERROR_TYPE, *MODEL_RESOLUTION_SKIP_ERROR_TYPES.values()} + { + TAGGER_DISABLED_ERROR_TYPE, + TAGGER_PARSE_ERROR_TYPE, + TAGGER_REQUEST_REJECTED_ERROR_TYPE, + *MODEL_RESOLUTION_SKIP_ERROR_TYPES.values(), + } ) @@ -313,6 +321,12 @@ def execute_tagger_activity(inputs: ExecuteTaggerInputs) -> dict[str, Any]: non_retryable=True, ) raise + except RetryableRateLimitError as e: + raise ApplicationError( + str(e), + {"error_type": "provider_unavailable", "provider": provider}, + next_retry_delay=timedelta(seconds=e.retry_after) if e.retry_after is not None else None, + ) from e except RateLimitError: if is_byok: raise ApplicationError( @@ -326,6 +340,13 @@ def execute_tagger_activity(inputs: ExecuteTaggerInputs) -> dict[str, Any]: f"Model '{model}' not found.", non_retryable=True, ) + except ProviderRequestRejectedError as e: + raise ApplicationError( + str(e), + {"error_type": "request_rejected"}, + type=TAGGER_REQUEST_REJECTED_ERROR_TYPE, + non_retryable=True, + ) from e except (OutputTokenLimitError, StructuredOutputParseError) as e: # A reply cut off at the output limit reaches the tagger as unusable output, same as a # malformed one, so both take the parse path. @@ -655,6 +676,7 @@ async def run(self, inputs: RunTaggerInputs) -> dict[str, Any]: "key_invalid", "parse_error", "no_default_model", + "request_rejected", ): if error_type in ( "provider_key_required", diff --git a/posthog/temporal/ai_observability/test_run_evaluation.py b/posthog/temporal/ai_observability/test_run_evaluation.py index 919768e4c5e2..08f1083827d2 100644 --- a/posthog/temporal/ai_observability/test_run_evaluation.py +++ b/posthog/temporal/ai_observability/test_run_evaluation.py @@ -614,17 +614,25 @@ def test_system_one_restricted_connection_does_not_send_evaluation_data(base_url @pytest.mark.parametrize( - "status, expected_skip_reason", - [(301, "endpoint_blocked"), (400, "request_rejected"), (422, "request_rejected")], + "provider,status,encoding,expected_skip_reason", + [ + ("system_one", 301, "identity", "endpoint_blocked"), + ("system_one", 400, "identity", "request_rejected"), + ("system_one", 422, "identity", "request_rejected"), + ("system_one", 200, "gzip", "request_rejected"), + ("openai_compatible", 200, "gzip", "request_rejected"), + ], ) -def test_system_one_rejections_distinguish_blocked_endpoints_from_bad_inputs( - status: int, expected_skip_reason: str +def test_provider_rejections_distinguish_blocked_endpoints_from_bad_inputs( + provider: str, status: int, encoding: str, expected_skip_reason: str ) -> None: key = MagicMock( - provider="system_one", + provider=provider, encrypted_config={"api_key": "example-token", "base_url": "https://decisions.example.com/v1"}, ) - response = httpx.Response(status, stream=httpx.ByteStream(b"Invalid request")) + response = httpx.Response( + status, stream=httpx.ByteStream(b"Invalid request"), headers={"Content-Encoding": encoding} + ) with ( patch("posthog.security.url_validation.resolve_host_ips", return_value={ip_address("8.8.8.8")}), patch("posthog.temporal.ai_observability.evaluation_llm_judge.model_spec") as spec, @@ -634,7 +642,7 @@ def test_system_one_rejections_distinguish_blocked_endpoints_from_bad_inputs( patch("httpx.AsyncHTTPTransport.handle_async_request", return_value=response), ): spec.return_value.resolve.return_value = MagicMock( - provider="system_one", model="example-judge-v1", provider_key=key, is_byok=True + provider=provider, model="example-judge-v1", provider_key=key, is_byok=True ) result = call_llm_judge( evaluation={"id": "test-evaluation", "team_id": 1, "evaluation_config": {"prompt": "Polite?"}}, @@ -653,10 +661,47 @@ def test_system_one_rejections_distinguish_blocked_endpoints_from_bad_inputs( assert "model" not in result assert "provider" not in result + if encoding == "gzip": + assert "uncompressed responses no larger than 1 MiB" in result["reasoning"] -def test_system_one_rate_limit_retries_without_disabling_the_evaluation() -> None: + +@pytest.mark.parametrize( + "provider, success_payload", + [ + ( + "system_one", + { + "model": "example-judge-v1", + "answers": {"verdict": {"type": "noul", "noul": 0.9}}, + "usage": {"input_tokens": 12, "output_tokens": 0}, + }, + ), + ( + "openai_compatible", + { + "id": "fixture", + "object": "chat.completion", + "created": 0, + "model": "example-judge-v1", + "choices": [ + { + "index": 0, + "finish_reason": "stop", + "message": { + "role": "assistant", + "content": json.dumps({"verdict": True, "reasoning": "Polite greeting"}), + }, + } + ], + }, + ), + ], +) +def test_custom_provider_rate_limit_retries_without_disabling_the_evaluation( + provider: str, success_payload: dict[str, Any] +) -> None: key = MagicMock( - provider="system_one", + provider=provider, encrypted_config={"api_key": "example-token", "base_url": "https://decisions.example.com/v1"}, ) with ( @@ -667,22 +712,41 @@ def test_system_one_rate_limit_retries_without_disabling_the_evaluation() -> Non ), patch( "httpx.AsyncHTTPTransport.handle_async_request", - return_value=httpx.Response(429, headers={"Retry-After": "15"}, stream=httpx.ByteStream(b"")), - ), - pytest.raises(ApplicationError) as error, + side_effect=[ + httpx.Response(429, headers={"Retry-After": "15"}, stream=httpx.ByteStream(b"")), + httpx.Response( + 200, + headers={"Content-Type": "application/json"}, + stream=httpx.ByteStream(json.dumps(success_payload).encode()), + ), + ], + ) as transport, ): spec.return_value.resolve.return_value = MagicMock( - provider="system_one", model="example-judge-v1", provider_key=key, is_byok=True + provider=provider, model="example-judge-v1", provider_key=key, is_byok=True ) - call_llm_judge( + with pytest.raises(ApplicationError) as error: + call_llm_judge( + evaluation={"team_id": 1, "evaluation_config": {"prompt": "Polite?"}}, + system_prompt="", + user_prompt="Hello!", + allows_na=False, + ) + assert not error.value.non_retryable + assert error.value.next_retry_delay == timedelta(seconds=15) + assert terminal_user_error_result_from_application_error(error.value, allows_na=False) is None + assert transport.call_count == 1 + + result = call_llm_judge( evaluation={"team_id": 1, "evaluation_config": {"prompt": "Polite?"}}, system_prompt="", user_prompt="Hello!", allows_na=False, ) - assert not error.value.non_retryable - assert error.value.next_retry_delay == timedelta(seconds=15) - assert terminal_user_error_result_from_application_error(error.value, allows_na=False) is None + assert result["verdict"] is True + assert "terminal_user_error" not in result + assert "provider_key_state" not in result + assert transport.call_count == 2 def _openai_status_error(status: int, message: str) -> openai.APIStatusError: diff --git a/posthog/temporal/ai_observability/test_run_tagger.py b/posthog/temporal/ai_observability/test_run_tagger.py index 8141c35d81ea..05e613e86295 100644 --- a/posthog/temporal/ai_observability/test_run_tagger.py +++ b/posthog/temporal/ai_observability/test_run_tagger.py @@ -1,20 +1,25 @@ import json import uuid import asyncio -from datetime import datetime +from datetime import UTC, datetime, timedelta from typing import Any, TypedDict import pytest from unittest.mock import MagicMock, patch -from temporalio.exceptions import ApplicationError +import httpx +from temporalio.exceptions import ActivityError, ApplicationError from posthog.api.capture import CaptureInternalError from posthog.models import Organization, Team from posthog.sync import database_sync_to_async from posthog.temporal.common.posthog_client import EXPECTED_CONTROL_FLOW_ERROR_TYPES, is_expected_activity_failure -from products.ai_observability.backend.llm.errors import OutputTokenLimitError, StructuredOutputParseError +from products.ai_observability.backend.llm.errors import ( + OutputTokenLimitError, + ProviderRequestRejectedError, + StructuredOutputParseError, +) from products.ai_observability.backend.models.provider_keys import LLMProviderKey from products.ai_observability.backend.models.taggers import Tagger @@ -811,14 +816,17 @@ def test_skipped_result_types_are_expected_control_flow(self) -> None: assert SKIPPED_RESULT_ERROR_TYPES <= EXPECTED_CONTROL_FLOW_ERROR_TYPES @pytest.mark.parametrize( - "llm_error", + "llm_error,error_type", [ - OutputTokenLimitError("The model reached its output token limit."), - StructuredOutputParseError("The reply did not match the schema."), + (OutputTokenLimitError("The model reached its output token limit."), "parse_error"), + (StructuredOutputParseError("The reply did not match the schema."), "parse_error"), + (ProviderRequestRejectedError("The response exceeds the limit."), "request_rejected"), ], ) @pytest.mark.django_db(transaction=True) - def test_unusable_reply_is_skipped_not_captured(self, setup_data: SetupData, llm_error: Exception) -> None: + def test_unusable_reply_is_skipped_not_captured( + self, setup_data: SetupData, llm_error: Exception, error_type: str + ) -> None: team = setup_data["team"] tagger = { "id": str(setup_data["tagger"].id), @@ -838,7 +846,97 @@ def test_unusable_reply_is_skipped_not_captured(self, setup_data: SetupData, llm with pytest.raises(ApplicationError) as exc_info: execute_tagger_activity(ExecuteTaggerInputs(tagger=tagger, event_data=create_mock_event_data(team.id))) - assert exc_info.value.details[0]["error_type"] == "parse_error" - assert exc_info.value.type == "tagger_parse_error" + assert exc_info.value.details[0]["error_type"] == error_type + assert exc_info.value.type == f"tagger_{error_type}" assert is_expected_activity_failure(exc_info.value) mock_capture_exception.assert_not_called() + + activity_error = ActivityError( + "Tagger activity failed", + scheduled_event_id=1, + started_event_id=2, + identity="test-worker", + activity_type="execute_tagger_activity", + activity_id="test-activity", + retry_state=None, + ) + activity_error.__cause__ = exc_info.value + with ( + patch("temporalio.workflow.deprecate_patch"), + patch("temporalio.workflow.now", return_value=datetime(2026, 1, 1, tzinfo=UTC)), + patch("temporalio.workflow.execute_activity", side_effect=[tagger, activity_error]), + ): + result = asyncio.run( + RunTaggerWorkflow().run( + RunTaggerInputs(tagger_id=tagger["id"], event_data=create_mock_event_data(team.id)) + ) + ) + + assert result == { + "tags": [], + "skipped": True, + "skip_reason": error_type, + "message": str(llm_error), + "tagger_id": tagger["id"], + } + + +@pytest.mark.parametrize("rate_limited", [False, True]) +def test_custom_provider_tagger_uses_bounded_completion(rate_limited: bool) -> None: + key = LLMProviderKey( + id=uuid.uuid4(), + provider="openai_compatible", + state=LLMProviderKey.State.OK, + encrypted_config={"api_key": "test-key", "base_url": "https://8.8.8.8/v1"}, + ) + payload = json.dumps( + { + "id": "fixture", + "object": "chat.completion", + "created": 0, + "model": "some-model", + "choices": [ + { + "index": 0, + "finish_reason": "stop", + "message": { + "role": "assistant", + "content": json.dumps({"tags": ["billing"], "reasoning": "Billing question"}), + }, + } + ], + } + ).encode() + responses = [httpx.Response(200, stream=httpx.ByteStream(payload))] + if rate_limited: + responses.insert(0, httpx.Response(429, headers={"Retry-After": "15"}, stream=httpx.ByteStream(b""))) + inputs = ExecuteTaggerInputs( + tagger={ + "id": "test-tagger", + "team_id": 1, + "tagger_config": make_tagger_config(), + "model_configuration": {"provider": "openai_compatible", "model": "some-model"}, + }, + event_data=create_mock_event_data(1), + ) + with ( + patch.object(key, "save"), + patch("posthog.temporal.ai_observability.model_resolution.EvaluationConfig") as configs, + patch( + "httpx.AsyncHTTPTransport.handle_async_request", + side_effect=responses, + ) as transport, + ): + configs.objects.get_or_create.return_value = (MagicMock(active_provider_key=key), False) + if rate_limited: + with pytest.raises(ApplicationError) as error: + execute_tagger_activity(inputs) + assert not error.value.non_retryable + assert error.value.next_retry_delay == timedelta(seconds=15) + assert error.value.details == ({"error_type": "provider_unavailable", "provider": "openai_compatible"},) + assert transport.call_count == 1 + result = execute_tagger_activity(inputs) + assert result["tags"] == ["billing"] + assert result["reasoning"] == "Billing question" + assert key.state == LLMProviderKey.State.OK + assert transport.call_count == (2 if rate_limited else 1) diff --git a/posthog/temporal/common/posthog_client.py b/posthog/temporal/common/posthog_client.py index 034e845f964f..b2c2b5c420d0 100644 --- a/posthog/temporal/common/posthog_client.py +++ b/posthog/temporal/common/posthog_client.py @@ -48,6 +48,7 @@ "SandboxControlPlaneUnavailableError", "tagger_disabled", "tagger_parse_error", + "tagger_request_rejected", "tagger_provider_key_required", "tagger_key_invalid", "tagger_no_default_model", diff --git a/posthog/temporal/oauth.py b/posthog/temporal/oauth.py index 32eeafa177a5..11bcd3001f00 100644 --- a/posthog/temporal/oauth.py +++ b/posthog/temporal/oauth.py @@ -4,14 +4,13 @@ from uuid import UUID from django.conf import settings -from django.utils import timezone import structlog from posthog.llm.wizard_blocklist import WIZARD_BLOCKED_DETAIL, wizard_identity_blocked -from posthog.models import OAuthAccessToken, OAuthApplication +from posthog.models import OAuthApplication +from posthog.models.oauth import mint_oauth_access_token from posthog.models.team.team import Team -from posthog.models.utils import generate_random_oauth_access_token from posthog.scopes import ( API_SCOPE_OBJECTS, INTERNAL_API_SCOPE_OBJECTS, @@ -582,22 +581,18 @@ def get_sandbox_oauth_app(application: SandboxOAuthApplication = "array") -> OAu return get_array_app() -def _mint_oauth_access_token( +def _mint_run_access_token( user, team_id: int, *, app: OAuthApplication, scopes: list[str], sandbox_task_id: UUID | None = None ) -> str: - token_value = generate_random_oauth_access_token(None) - - OAuthAccessToken.objects.create( - user=user, + access_token = mint_oauth_access_token( application=app, - token=token_value, - expires=timezone.now() + timedelta(seconds=TOKEN_EXPIRATION_SECONDS), + user=user, scope=" ".join(dict.fromkeys(scopes)), + lifetime=timedelta(seconds=TOKEN_EXPIRATION_SECONDS), scoped_teams=[team_id], sandbox_task_id=sandbox_task_id, ) - - return token_value + return access_token.token def create_oauth_access_token_for_user( @@ -630,7 +625,7 @@ def create_oauth_access_token_for_user( if include_slack_run_scope: resolved.append(SLACK_RUN_SCOPE) app = get_sandbox_oauth_app(application) - return _mint_oauth_access_token(user, team_id, app=app, scopes=list(resolved), sandbox_task_id=sandbox_task_id) + return _mint_run_access_token(user, team_id, app=app, scopes=list(resolved), sandbox_task_id=sandbox_task_id) def get_wizard_app() -> OAuthApplication: @@ -691,4 +686,4 @@ def create_wizard_oauth_access_token_for_user(user, team_id: int) -> str: if ceiling is None or len(ceiling) == 0: raise RuntimeError("Wizard app has no scope ceiling. Must be configured in the database.") - return _mint_oauth_access_token(user, team_id, app=app, scopes=sorted(ceiling)) + return _mint_run_access_token(user, team_id, app=app, scopes=sorted(ceiling)) diff --git a/posthog/test/test_csp_middleware.py b/posthog/test/test_csp_middleware.py index e0b670af33a8..2a695214f69e 100644 --- a/posthog/test/test_csp_middleware.py +++ b/posthog/test/test_csp_middleware.py @@ -190,7 +190,7 @@ def test_signed_out_page_without_the_flag_enforces_only_frame_ancestors( # Framing is enforced ahead of the flag because it is what lets posthog.com frame the app. # The enforced list has to be the one the reported policy names, or the two drift apart. enforced = response["Content-Security-Policy"] - assert enforced.startswith("frame-ancestors https://posthog.com") + assert enforced.startswith("frame-ancestors 'self' https://posthog.com") assert "default-src" not in enforced assert enforced in reported diff --git a/products/ai_observability/backend/api/proxy.py b/products/ai_observability/backend/api/proxy.py index 30333278d936..effb4d136fb0 100644 --- a/products/ai_observability/backend/api/proxy.py +++ b/products/ai_observability/backend/api/proxy.py @@ -9,6 +9,7 @@ import json import uuid from collections.abc import Callable, Generator +from contextlib import closing from time import perf_counter from typing import Any @@ -223,12 +224,13 @@ def _create_stream_generator( """Creates a generator that handles client disconnects and encodes responses""" started = perf_counter() try: - for chunk in client.stream(request_obj): - if not http_request.META.get("SERVER_NAME"): # Client disconnected - if on_error: - on_error(Exception("Client disconnected"), perf_counter() - started) - return - yield chunk.to_sse().encode() + with closing(client.stream(request_obj)) as stream: + for chunk in stream: + if not http_request.META.get("SERVER_NAME"): # Client disconnected + if on_error: + on_error(Exception("Client disconnected"), perf_counter() - started) + return + yield chunk.to_sse().encode() except ProviderConfigurationError as e: if on_error: on_error(e, perf_counter() - started) diff --git a/products/ai_observability/backend/api/test/test_proxy.py b/products/ai_observability/backend/api/test/test_proxy.py index 8fb9af94dc1b..90f12e448419 100644 --- a/products/ai_observability/backend/api/test/test_proxy.py +++ b/products/ai_observability/backend/api/test/test_proxy.py @@ -1,11 +1,17 @@ +import asyncio +from collections.abc import AsyncGenerator, AsyncIterator from types import SimpleNamespace from typing import cast from uuid import uuid4 +import pytest from posthog.test.base import APIBaseTest from unittest import TestCase from unittest.mock import patch +from django.http import StreamingHttpResponse + +import httpx from parameterized import parameterized from rest_framework.request import Request @@ -22,6 +28,8 @@ from products.ai_observability.backend.llm import ( PLAYGROUND_MODEL_IDS, PROVIDERS, + Client, + CompletionRequest, get_default_models, get_playground_models, ) @@ -31,6 +39,55 @@ BYOK_THROTTLES = (LLMProxyBYOKBurstRateThrottle, LLMProxyBYOKSustainedRateThrottle, LLMProxyBYOKDailyRateThrottle) +class TestPlaygroundStreamCleanup: + async def test_disconnect_closes_bounded_provider_stream(self) -> None: + class Body(httpx.AsyncByteStream): + closed = False + + async def __aiter__(self) -> AsyncIterator[bytes]: + yield b'data: {"id":"fixture","choices":[{"index":0,"delta":{"content":"hello"}}]}\n\n' + yield b"data: [DONE]\n\n" + + async def aclose(self) -> None: + self.closed = True + + body = Body() + client = Client( + provider_key=LLMProviderKey( + provider="openai_compatible", + encrypted_config={"api_key": "test-key", "base_url": "https://8.8.8.8/v1"}, + ), + capture_analytics=False, + ) + request = CompletionRequest(model="some-model", provider="openai_compatible", messages=[]) + view = LLMProxyViewSet() + stream = view._create_stream_generator(client, request, SimpleNamespace(META={"SERVER_NAME": "test"})) + with ( + patch("products.ai_observability.backend.api.proxy.SERVER_GATEWAY_INTERFACE", "ASGI"), + patch("httpx.AsyncHTTPTransport.handle_async_request", return_value=httpx.Response(200, stream=body)), + ): + response = await asyncio.to_thread(view._create_streaming_response, stream) + assert isinstance(response, StreamingHttpResponse) + iterator = cast(AsyncGenerator[bytes], aiter(response._iterator)) # type: ignore[attr-defined] + first_chunk = asyncio.Event() + + async def consume() -> None: + try: + assert b"hello" in await anext(iterator) + first_chunk.set() + await asyncio.Event().wait() + finally: + await iterator.aclose() + + task = asyncio.create_task(consume()) + await asyncio.wait_for(first_chunk.wait(), 5) + task.cancel() + with pytest.raises(asyncio.CancelledError): + await task + + assert body.closed + + class TestLLMProxyThrottles(APIBaseTest): def setUp(self) -> None: super().setUp() diff --git a/products/ai_observability/backend/llm/errors.py b/products/ai_observability/backend/llm/errors.py index 1d2471bb14b7..e9fa593091f9 100644 --- a/products/ai_observability/backend/llm/errors.py +++ b/products/ai_observability/backend/llm/errors.py @@ -1,4 +1,7 @@ +import math import logging +from datetime import UTC, datetime +from email.utils import parsedate_to_datetime from products.ai_observability.backend.llm.types import StreamChunk @@ -31,6 +34,22 @@ class RateLimitError(LLMError): """Raised when rate limit is exceeded""" +class RetryableRateLimitError(RateLimitError): + def __init__(self, message: str, retry_after: str | None = None) -> None: + super().__init__(message) + self.retry_after: float | None = None + if retry_after: + try: + delay = float(retry_after) + except ValueError: + try: + delay = (parsedate_to_datetime(retry_after) - datetime.now(UTC)).total_seconds() + except (ValueError, TypeError, OverflowError): + return + if math.isfinite(delay): + self.retry_after = max(1, min(delay, 60)) + + class QuotaExceededError(LLMError): """Raised when API quota is exceeded""" @@ -41,6 +60,23 @@ class ProviderConnectionError(LLMError): and should not log it as an exception since it's usually resolved on the next attempt.""" +class ProviderTimeoutError(ProviderConnectionError): + def __init__(self, timeout: float) -> None: + super().__init__( + f"The endpoint did not finish within {timeout:g} seconds. Check the endpoint's response time before trying again." + ) + + +RESPONSE_LIMIT_MESSAGE = ( + "The endpoint returned a compressed or oversized response. " + "Configure it to return uncompressed responses no larger than 1 MiB." +) + + +class ProviderRequestRejectedError(LLMError): + """A non-retryable request rejection with a message safe to show to the user.""" + + class ProviderConfigurationError(LLMError): """Raised when a provider key's stored configuration cannot be used as it stands — a base URL that no longer passes the SSRF allowlist, or a required endpoint that was never set. The user @@ -167,6 +203,8 @@ def user_facing_error_message(error: Exception | None) -> str: return "This conversation is too long for the model's context window. Shorten it, then try again." if isinstance(error, OutputTokenLimitError): return "The model ran out of room before it finished its reply. Ask for a shorter answer, then try again." + if isinstance(error, (ProviderTimeoutError, ProviderRequestRejectedError)): + return str(error) if isinstance(error, ProviderConnectionError): return "Could not reach the model provider. Try again." if isinstance(error, StructuredOutputParseError): diff --git a/products/ai_observability/backend/llm/providers/_diagnostics.py b/products/ai_observability/backend/llm/providers/_diagnostics.py index 6d5a97a4d79e..b413c9aa04be 100644 --- a/products/ai_observability/backend/llm/providers/_diagnostics.py +++ b/products/ai_observability/backend/llm/providers/_diagnostics.py @@ -30,6 +30,7 @@ def tagged_http_client( *, pin: tuple[str, ResolvedIPs] | None = None, follow_redirects: bool = True, + total_timeout: float | None = None, ) -> httpx.Client: """An httpx client that tags provider responses. @@ -47,4 +48,4 @@ def tagged_http_client( if pin is None: return httpx.Client(**kwargs) url, pinned_ips = pin - return pinned_client(url, pinned_ips, **kwargs) + return pinned_client(url, pinned_ips, total_timeout=total_timeout, **kwargs) diff --git a/products/ai_observability/backend/llm/providers/azure_openai.py b/products/ai_observability/backend/llm/providers/azure_openai.py index bc5d58e04583..7e38ae147339 100644 --- a/products/ai_observability/backend/llm/providers/azure_openai.py +++ b/products/ai_observability/backend/llm/providers/azure_openai.py @@ -14,7 +14,7 @@ from posthoganalytics.ai.openai import AzureOpenAI as WrappedAzureOpenAI from products.ai_observability.backend.llm.errors import error_field_for_message -from products.ai_observability.backend.llm.providers.openai import OpenAIAdapter, OpenAIConfig +from products.ai_observability.backend.llm.providers.openai import OpenAIAdapter from products.ai_observability.backend.llm.types import AnalyticsContext logger = logging.getLogger(__name__) @@ -144,14 +144,16 @@ def _create_client( api_key=api_key, azure_endpoint=self.azure_endpoint, api_version=self.api_version, - timeout=OpenAIConfig.TIMEOUT, + timeout=self.request_timeout, + max_retries=self.max_retries, http_client=http_client, ) return openai.AzureOpenAI( api_key=api_key, azure_endpoint=self.azure_endpoint, api_version=self.api_version, - timeout=OpenAIConfig.TIMEOUT, + timeout=self.request_timeout, + max_retries=self.max_retries, http_client=http_client, ) diff --git a/products/ai_observability/backend/llm/providers/openai.py b/products/ai_observability/backend/llm/providers/openai.py index dde02805dc0e..8dbe78fa0b68 100644 --- a/products/ai_observability/backend/llm/providers/openai.py +++ b/products/ai_observability/backend/llm/providers/openai.py @@ -109,6 +109,8 @@ class OpenAIAdapter: """OpenAI provider implementing the unified Client interface.""" name = "openai" + request_timeout: float = OpenAIConfig.TIMEOUT + max_retries: int = openai.DEFAULT_MAX_RETRIES def _create_client( self, @@ -125,14 +127,16 @@ def _create_client( api_key=api_key, posthog_client=posthog_client, base_url=base_url, - timeout=OpenAIConfig.TIMEOUT, + timeout=self.request_timeout, + max_retries=self.max_retries, default_headers=default_headers or None, http_client=http_client, ) return openai.OpenAI( api_key=api_key, base_url=base_url, - timeout=OpenAIConfig.TIMEOUT, + timeout=self.request_timeout, + max_retries=self.max_retries, default_headers=default_headers or None, http_client=http_client, ) @@ -145,7 +149,7 @@ def _build_http_client(self) -> httpx.Client: """ from products.ai_observability.backend.llm.providers._diagnostics import tagged_http_client - return tagged_http_client(timeout=OpenAIConfig.TIMEOUT) + return tagged_http_client(timeout=self.request_timeout) def complete( self, @@ -160,9 +164,8 @@ def complete( client = self._create_client(effective_api_key, effective_base_url, analytics) - messages: Any = self._build_messages(request) - try: + messages: Any = self._build_messages(request) if request.response_format and issubclass(request.response_format, BaseModel): try: # Try native structured output parsing first @@ -218,6 +221,8 @@ def complete( if mapped is not None: raise mapped from e raise + finally: + client.close() def _mapped_error(self, error: Exception, model: str) -> LLMError | None: """Normalize a provider exception into the shared taxonomy, or None when it isn't ours. @@ -318,12 +323,10 @@ def stream( client = self._create_client(effective_api_key, effective_base_url, analytics) - supports_reasoning = model_id in OpenAIConfig.SUPPORTED_MODELS_WITH_THINKING - reasoning_on = supports_reasoning and (request.thinking or bool(request.reasoning_level)) - - tools = self._convert_tools(request.tools) if request.tools else None - try: + supports_reasoning = model_id in OpenAIConfig.SUPPORTED_MODELS_WITH_THINKING + reasoning_on = supports_reasoning and (request.thinking or bool(request.reasoning_level)) + tools = self._convert_tools(request.tools) if request.tools else None effective_temperature = request.temperature if request.temperature is not None else OpenAIConfig.TEMPERATURE def build_common_kwargs() -> dict[str, Any]: @@ -396,6 +399,8 @@ def build_common_kwargs() -> dict[str, Any]: except Exception as e: yield stream_error_chunk(e, self._mapped_error(e, model_id), logger=logger, provider=self.name) + finally: + client.close() @staticmethod def validate_key(api_key: str, **kwargs: Any) -> tuple[str, str | None]: diff --git a/products/ai_observability/backend/llm/providers/openai_compatible.py b/products/ai_observability/backend/llm/providers/openai_compatible.py index 35be5b0de5dc..d3b5599a9d87 100644 --- a/products/ai_observability/backend/llm/providers/openai_compatible.py +++ b/products/ai_observability/backend/llm/providers/openai_compatible.py @@ -20,13 +20,23 @@ import httpx import openai +from temporalio.exceptions import CancelledError from posthog.security.pinned_requests import SSRFBlockedError from posthog.security.url_validation import is_url_allowed, validate_url_and_pin_ips -from products.ai_observability.backend.llm.errors import ProviderConfigurationError, error_field_for_message +from products.ai_observability.backend.llm.errors import ( + RESPONSE_LIMIT_MESSAGE, + LLMError, + ProviderConfigurationError, + ProviderRequestRejectedError, + ProviderTimeoutError, + RateLimitError, + RetryableRateLimitError, + error_field_for_message, +) from products.ai_observability.backend.llm.providers._diagnostics import tagged_http_client -from products.ai_observability.backend.llm.providers.openai import OpenAIAdapter, OpenAIConfig +from products.ai_observability.backend.llm.providers.openai import OpenAIAdapter from products.ai_observability.backend.llm.types import ( AnalyticsContext, CompletionRequest, @@ -60,6 +70,8 @@ ("Base URL must be", "base_url"), ("The endpoint did not return a model list", "base_url"), ("The endpoint redirected", "base_url"), + ("The endpoint returned a compressed or oversized response", "base_url"), + ("The endpoint did not finish", "base_url"), ("Could not connect to the endpoint", "base_url"), ("Invalid API key", "api_key"), ) @@ -92,7 +104,12 @@ def _pinned_http_client(base_url: str, timeout: float) -> httpx.Client: verdict = validate_url_and_pin_ips(base_url) if not verdict.allowed: raise SSRFBlockedError(verdict.reason or "URL blocked by SSRF protection") - return tagged_http_client(timeout=timeout, pin=(base_url, verdict.pinned_ips), follow_redirects=False) + return tagged_http_client( + pin=(base_url, verdict.pinned_ips), + timeout=timeout, + total_timeout=timeout, + follow_redirects=False, + ) class OpenAICompatibleAdapter(OpenAIAdapter): @@ -107,6 +124,9 @@ class OpenAICompatibleAdapter(OpenAIAdapter): """ name = "openai_compatible" + request_timeout = 60.0 + # Temporal owns retries; SDK retries can catch thread cancellation and start another request. + max_retries = 0 def __init__(self, base_url: str = ""): self.base_url = base_url @@ -124,7 +144,20 @@ def _require_allowed_base_url(self) -> str: def _build_http_client(self) -> httpx.Client: """Pin the connection to the configured endpoint's validated address.""" - return _pinned_http_client(self._require_allowed_base_url(), OpenAIConfig.TIMEOUT) + return _pinned_http_client(self._require_allowed_base_url(), self.request_timeout) + + def _mapped_error(self, error: Exception, model: str) -> LLMError | None: + cause = error.__cause__ if isinstance(error, openai.APIConnectionError) else error + if isinstance(cause, CancelledError): + raise cause + if isinstance(cause, httpx.DecodingError): + return ProviderRequestRejectedError(RESPONSE_LIMIT_MESSAGE) + if isinstance(cause, httpx.TimeoutException): + return ProviderTimeoutError(self.request_timeout) + mapped = super()._mapped_error(error, model) + if isinstance(error, openai.RateLimitError) and isinstance(mapped, RateLimitError): + return RetryableRateLimitError(str(error), error.response.headers.get("Retry-After")) + return mapped def complete( self, @@ -183,7 +216,11 @@ def validate_key(api_key: str, **kwargs: Any) -> tuple[str, str | None]: return (LLMProviderKey.State.INVALID, REDIRECT_MESSAGE) logger.exception("%s key validation error", PROVIDER_DISPLAY_NAME) return (LLMProviderKey.State.ERROR, "Validation failed, please try again") - except openai.APIConnectionError: + except openai.APIConnectionError as error: + if isinstance(error.__cause__, httpx.DecodingError): + return (LLMProviderKey.State.INVALID, RESPONSE_LIMIT_MESSAGE) + if isinstance(error.__cause__, httpx.TimeoutException): + return (LLMProviderKey.State.ERROR, str(ProviderTimeoutError(VALIDATION_TIMEOUT))) return (LLMProviderKey.State.ERROR, "Could not connect to the endpoint") except Exception: logger.exception("%s key validation error", PROVIDER_DISPLAY_NAME) diff --git a/products/ai_observability/backend/llm/providers/test/test_azure_openai.py b/products/ai_observability/backend/llm/providers/test/test_azure_openai.py index 44ddb3730960..c485bfd67d9e 100644 --- a/products/ai_observability/backend/llm/providers/test/test_azure_openai.py +++ b/products/ai_observability/backend/llm/providers/test/test_azure_openai.py @@ -270,6 +270,8 @@ def test_create_client_uses_azure_config(self, mock_azure): from products.ai_observability.backend.llm.types import AnalyticsContext adapter = AzureOpenAIAdapter(azure_endpoint=MOCK_ENDPOINT, api_version="2025-01-01") + adapter.request_timeout = 12.0 + adapter.max_retries = 0 analytics = AnalyticsContext(distinct_id="test", capture=False) adapter._create_client("test-key", None, analytics) @@ -278,6 +280,8 @@ def test_create_client_uses_azure_config(self, mock_azure): assert mock_azure.call_args.kwargs["api_key"] == "test-key" assert mock_azure.call_args.kwargs["azure_endpoint"] == MOCK_ENDPOINT assert mock_azure.call_args.kwargs["api_version"] == "2025-01-01" + assert mock_azure.call_args.kwargs["timeout"] == 12.0 + assert mock_azure.call_args.kwargs["max_retries"] == 0 @patch("products.ai_observability.backend.llm.providers.azure_openai.openai.AzureOpenAI") def test_create_client_ignores_base_url(self, mock_azure): @@ -300,6 +304,8 @@ def test_create_client_uses_wrapped_client_when_analytics_enabled(self, mock_wra from products.ai_observability.backend.llm.types import AnalyticsContext adapter = AzureOpenAIAdapter(azure_endpoint=MOCK_ENDPOINT) + adapter.request_timeout = 12.0 + adapter.max_retries = 0 analytics = AnalyticsContext(distinct_id="test", capture=True) adapter._create_client("test-key", None, analytics) @@ -307,3 +313,5 @@ def test_create_client_uses_wrapped_client_when_analytics_enabled(self, mock_wra mock_wrapped.assert_called_once() assert mock_wrapped.call_args.kwargs["api_key"] == "test-key" assert mock_wrapped.call_args.kwargs["azure_endpoint"] == MOCK_ENDPOINT + assert mock_wrapped.call_args.kwargs["timeout"] == 12.0 + assert mock_wrapped.call_args.kwargs["max_retries"] == 0 diff --git a/products/ai_observability/backend/llm/providers/test/test_openai_compatible.py b/products/ai_observability/backend/llm/providers/test/test_openai_compatible.py index 6a2bb6f4088d..9f5354ffd0cb 100644 --- a/products/ai_observability/backend/llm/providers/test/test_openai_compatible.py +++ b/products/ai_observability/backend/llm/providers/test/test_openai_compatible.py @@ -1,14 +1,28 @@ +import json +from collections.abc import AsyncIterator, Iterator +from contextlib import contextmanager + import pytest +from unittest import TestCase from unittest.mock import MagicMock, patch import httpx import openai from parameterized import parameterized +from pydantic import BaseModel +from temporalio.exceptions import CancelledError from posthog.security.pinned_requests import SSRFBlockedError from posthog.security.url_validation import PinnedUrlVerdict -from products.ai_observability.backend.llm.errors import ProviderConfigurationError +from products.ai_observability.backend.llm.errors import ( + AuthenticationError, + LLMError, + ProviderConfigurationError, + ProviderRequestRejectedError, + ProviderTimeoutError, + QuotaExceededError, +) from products.ai_observability.backend.llm.providers import openai_compatible from products.ai_observability.backend.llm.providers.openai_compatible import ( DISALLOWED_BASE_URL_MESSAGE, @@ -61,6 +75,8 @@ class TestErrorFieldForValidationMessage: ("not_found", "The endpoint did not return a model list, check the base URL", "base_url"), ("redirect", REDIRECT_MESSAGE, "base_url"), ("connection", "Could not connect to the endpoint", "base_url"), + ("response_limit", openai_compatible.RESPONSE_LIMIT_MESSAGE, "base_url"), + ("timeout", str(ProviderTimeoutError(VALIDATION_TIMEOUT)), "base_url"), ("bad_key", "Invalid API key", "api_key"), ("unattributed", "Rate limited, please try again later", None), ("none", None, None), @@ -216,3 +232,224 @@ def test_complete_without_api_key_raises(self, mock_openai): with pytest.raises(ValueError, match="BYOK-only"): adapter.complete(_completion_request(), None, AnalyticsContext()) mock_openai.assert_not_called() + + +class _Verdict(BaseModel): + verdict: bool + + +class _ResponseBody(httpx.SyncByteStream, httpx.AsyncByteStream): + def __init__(self, chunks: list[bytes], clock: list[float] | None = None) -> None: + self.chunks = chunks + self.clock = clock + self.closed = False + + def __iter__(self) -> Iterator[bytes]: + for chunk in self.chunks: + if self.clock is not None: + self.clock[0] += 1 + yield chunk + + async def __aiter__(self) -> AsyncIterator[bytes]: + for chunk in self: + yield chunk + + def close(self) -> None: + self.closed = True + + async def aclose(self) -> None: + self.close() + + +@contextmanager +def _mock_response(body: _ResponseBody, headers: dict[str, str] | None = None) -> Iterator[None]: + response = httpx.Response(200, stream=body, headers=headers or {}) + with ( + patch("httpx.HTTPTransport.handle_request", return_value=response), + patch("httpx.AsyncHTTPTransport.handle_async_request", return_value=response), + ): + try: + yield + finally: + assert body.closed + + +@contextmanager +def _response_over_limit(response_kind: str) -> Iterator[None]: + body = _ResponseBody([b"x" * 8192] * 129 if response_kind == "oversized" else [b"compressed"]) + headers = {"Content-Encoding": "gzip"} if response_kind == "compressed" else {} + with _mock_response(body, headers): + yield + + +@contextmanager +def _dripping_response(adapter: OpenAICompatibleAdapter) -> Iterator[None]: + clock = [0.0] + body = _ResponseBody([b" "] * 4, clock) + with ( + patch.object(adapter, "request_timeout", 2.0), + patch.object(openai_compatible, "VALIDATION_TIMEOUT", 2.0), + patch("asyncio.BaseEventLoop.time", side_effect=lambda: clock[0]), + _mock_response(body, {"Content-Type": "application/json"}), + ): + yield + + +class TestOpenAICompatibleRequestBounds(TestCase): + @parameterized.expand([("oversized",), ("compressed",)]) + def test_complete_rejects_unbounded_responses(self, response_kind: str) -> None: + adapter = OpenAICompatibleAdapter(base_url=ALLOWED_BASE_URL) + request = _completion_request() + request.response_format = _Verdict + with _response_over_limit(response_kind), pytest.raises(ProviderRequestRejectedError) as error: + adapter.complete(request, "test-key", AnalyticsContext(capture=False)) + assert str(error.value) == openai_compatible.RESPONSE_LIMIT_MESSAGE + + @parameterized.expand([("oversized",), ("compressed",)]) + def test_stream_reports_response_limit(self, response_kind: str) -> None: + adapter = OpenAICompatibleAdapter(base_url=ALLOWED_BASE_URL) + with _response_over_limit(response_kind): + chunks = list(adapter.stream(_completion_request(), "test-key", AnalyticsContext(capture=False))) + assert [(chunk.type, chunk.data) for chunk in chunks] == [ + ("error", {"error": openai_compatible.RESPONSE_LIMIT_MESSAGE}) + ] + + @parameterized.expand([("oversized",), ("compressed",)]) + def test_validate_key_reports_response_limit(self, response_kind: str) -> None: + with _response_over_limit(response_kind): + result = OpenAICompatibleAdapter.validate_key("test-key", base_url=ALLOWED_BASE_URL) + assert result == ("invalid", openai_compatible.RESPONSE_LIMIT_MESSAGE) + + @parameterized.expand([("oversized",), ("compressed",)]) + def test_list_models_logs_response_limit(self, response_kind: str) -> None: + with _response_over_limit(response_kind), self.assertLogs(openai_compatible.logger) as logs: + assert OpenAICompatibleAdapter.list_models("test-key", base_url=ALLOWED_BASE_URL) == [] + assert logs.records[0].exc_info is not None + error = logs.records[0].exc_info[1] + assert isinstance(error, openai.APIConnectionError) + assert isinstance(error.__cause__, httpx.DecodingError) + + @parameterized.expand([("complete", False), ("complete", True), ("stream", False), ("stream", True)]) + def test_preserves_cancellation_without_retrying(self, operation: str, capture: bool) -> None: + adapter = OpenAICompatibleAdapter(base_url=ALLOWED_BASE_URL) + request = _completion_request() + request.response_format = _Verdict + with ( + patch("httpx.HTTPTransport.handle_request", side_effect=CancelledError) as sync_send, + patch("httpx.AsyncHTTPTransport.handle_async_request", side_effect=CancelledError) as async_send, + patch("posthoganalytics.default_client", MagicMock()), + pytest.raises(CancelledError), + ): + if operation == "complete": + adapter.complete(request, "test-key", AnalyticsContext(capture=capture)) + else: + list(adapter.stream(request, "test-key", AnalyticsContext(capture=capture))) + assert sync_send.call_count + async_send.call_count == 1 + + def test_complete_stops_at_total_deadline(self) -> None: + adapter = OpenAICompatibleAdapter(base_url=ALLOWED_BASE_URL) + with _dripping_response(adapter), pytest.raises(ProviderTimeoutError, match="within 2 seconds"): + adapter.complete(_completion_request(), "test-key", AnalyticsContext(capture=False)) + + def test_stream_reports_total_deadline(self) -> None: + adapter = OpenAICompatibleAdapter(base_url=ALLOWED_BASE_URL) + with _dripping_response(adapter): + chunks = list(adapter.stream(_completion_request(), "test-key", AnalyticsContext(capture=False))) + assert [(chunk.type, chunk.data) for chunk in chunks] == [("error", {"error": str(ProviderTimeoutError(2))})] + + def test_validate_key_reports_total_deadline(self) -> None: + adapter = OpenAICompatibleAdapter(base_url=ALLOWED_BASE_URL) + with _dripping_response(adapter): + result = adapter.validate_key("test-key", base_url=ALLOWED_BASE_URL) + assert result == ("error", str(ProviderTimeoutError(2))) + + def test_list_models_logs_total_deadline(self) -> None: + adapter = OpenAICompatibleAdapter(base_url=ALLOWED_BASE_URL) + with _dripping_response(adapter), self.assertLogs(openai_compatible.logger) as logs: + assert adapter.list_models("test-key", base_url=ALLOWED_BASE_URL) == [] + assert logs.records[0].exc_info is not None + error = logs.records[0].exc_info[1] + assert isinstance(error, openai.APIConnectionError) + assert isinstance(error.__cause__, httpx.TimeoutException) + + @parameterized.expand( + [ + (401, {"message": "Invalid API key"}, AuthenticationError), + (429, {"message": "Quota exceeded", "code": "insufficient_quota"}, QuotaExceededError), + ] + ) + def test_complete_preserves_permanent_errors( + self, status: int, error_body: dict[str, str], expected_error: type[LLMError] + ) -> None: + response = httpx.Response(status, stream=httpx.ByteStream(json.dumps({"error": error_body}).encode())) + with ( + patch("httpx.AsyncHTTPTransport.handle_async_request", return_value=response), + pytest.raises(expected_error), + ): + OpenAICompatibleAdapter(base_url=ALLOWED_BASE_URL).complete( + _completion_request(), "test-key", AnalyticsContext(capture=False) + ) + + def test_structured_output_falls_back_without_sdk_retries(self) -> None: + fallback_body = _ResponseBody( + [ + json.dumps( + { + "id": "fixture", + "object": "chat.completion", + "created": 0, + "model": "some-model", + "choices": [ + { + "index": 0, + "finish_reason": "stop", + "message": {"role": "assistant", "content": '{"verdict": true}'}, + } + ], + } + ).encode() + ] + ) + responses = [ + httpx.Response( + 400, + stream=_ResponseBody([b'{"error":{"message":"response_format json_schema is not supported"}}']), + headers={"Content-Type": "application/json"}, + ), + httpx.Response(200, stream=fallback_body, headers={"Content-Type": "application/json"}), + ] + request = _completion_request() + request.response_format = _Verdict + + with patch("httpx.AsyncHTTPTransport.handle_async_request", side_effect=responses) as send: + result = OpenAICompatibleAdapter(base_url=ALLOWED_BASE_URL).complete( + request, "test-key", AnalyticsContext(capture=False) + ) + + assert result.parsed == _Verdict(verdict=True) + assert send.call_count == 2 + assert fallback_body.closed + + def test_closing_stream_closes_the_connection(self) -> None: + payload = json.dumps( + { + "id": "fixture", + "object": "chat.completion.chunk", + "created": 0, + "model": "some-model", + "choices": [{"index": 0, "delta": {"content": "hello"}, "finish_reason": None}], + } + ).encode() + body = _ResponseBody([b"data: " + payload + b"\n\n", b"data: [DONE]\n\n"]) + response = httpx.Response(200, stream=body, headers={"Content-Type": "text/event-stream"}) + adapter = OpenAICompatibleAdapter(base_url=ALLOWED_BASE_URL) + + with ( + patch("httpx.HTTPTransport.handle_request", return_value=response), + patch("httpx.AsyncHTTPTransport.handle_async_request", return_value=response), + ): + stream = adapter.stream(_completion_request(), "test-key", AnalyticsContext(capture=False)) + assert next(stream).data == {"text": "hello"} + stream.close() + + assert body.closed diff --git a/products/ai_observability/backend/llm/system_one.py b/products/ai_observability/backend/llm/system_one.py index ed563125927d..36ed0b117ac5 100644 --- a/products/ai_observability/backend/llm/system_one.py +++ b/products/ai_observability/backend/llm/system_one.py @@ -1,7 +1,4 @@ -import math from collections.abc import Mapping -from datetime import UTC, datetime -from email.utils import parsedate_to_datetime from urllib.parse import urlsplit import httpx @@ -17,22 +14,24 @@ ) from posthog.models import Team from posthog.ph_client import get_feature_flag_or_none -from posthog.security.pinned_httpx import pinned_client from posthog.security.pinned_requests import SSRFBlockedError from posthog.security.url_validation import has_authority_bypass_chars, validate_url_and_pin_ips from products.ai_observability.backend.llm.errors import ( + RESPONSE_LIMIT_MESSAGE, AuthenticationError, ContextWindowExceededError, LLMError, ModelNotFoundError, ModelPermissionError, ProviderConnectionError, + ProviderRequestRejectedError, RateLimitError, + RetryableRateLimitError, StructuredOutputParseError, is_context_window_error_message, ) -from products.ai_observability.backend.llm.providers._diagnostics import _tag_response +from products.ai_observability.backend.llm.providers._diagnostics import tagged_http_client def system_one_evaluations_enabled(team_id: int, *, base_url: str) -> bool: @@ -54,7 +53,7 @@ def system_one_evaluations_enabled(team_id: int, *, base_url: str) -> bool: ) -class SystemOneRequestRejectedError(LLMError): +class SystemOneRequestRejectedError(ProviderRequestRejectedError): pass @@ -62,20 +61,9 @@ class SystemOneEndpointBlockedError(LLMError): pass -class SystemOneRateLimitError(RateLimitError): +class SystemOneRateLimitError(RetryableRateLimitError): def __init__(self, retry_after: str | None) -> None: - super().__init__("The System One endpoint is temporarily unavailable. Try again later.") - self.retry_after: float | None = None - if retry_after: - try: - delay = float(retry_after) - except ValueError: - try: - delay = (parsedate_to_datetime(retry_after) - datetime.now(UTC)).total_seconds() - except (ValueError, TypeError, OverflowError): - return - if math.isfinite(delay): - self.retry_after = max(1, min(delay, 60)) + super().__init__("The System One endpoint is temporarily unavailable. Try again later.", retry_after) class SystemOneClient: @@ -118,13 +106,11 @@ def evaluate( verdict = validate_url_and_pin_ips(base_url) if not verdict.allowed: raise SSRFBlockedError(verdict.reason) - with pinned_client( - base_url, - verdict.pinned_ips, + with tagged_http_client( + pin=(base_url, verdict.pinned_ips), timeout=timeout, total_timeout=timeout, follow_redirects=False, - event_hooks={"response": [_tag_response]}, ) as client: response = client.post( f"{base_url}/systemone", @@ -134,10 +120,7 @@ def evaluate( except SSRFBlockedError as error: raise SystemOneEndpointBlockedError("This endpoint is not allowed. Use a public HTTPS endpoint.") from error except httpx.DecodingError as error: - raise SystemOneRequestRejectedError( - "The endpoint returned a compressed or oversized response. " - "Configure it to return uncompressed responses no larger than 1 MiB." - ) from error + raise SystemOneRequestRejectedError(RESPONSE_LIMIT_MESSAGE) from error except httpx.RequestError as error: raise ProviderConnectionError("Could not reach the System One endpoint. Try again.") from error diff --git a/products/autoresearch/backend/access.py b/products/autoresearch/backend/access.py index bddd66024ca1..0bc5ba1fff1b 100644 --- a/products/autoresearch/backend/access.py +++ b/products/autoresearch/backend/access.py @@ -1,7 +1,8 @@ -"""Feature-flag gate for the autoresearch product. +"""Feature-flag gates for the autoresearch product. -Access is controlled by the `autoresearch` feature flag. Rollout is configured -on the flag in PostHog, so code only asks whether it's enabled for this user/team. +Access is controlled by the `autoresearch` feature flag. The `autoresearch-report-notebook` +flag gates the report notebook that a training run builds. Rollout is configured on each +flag in PostHog, so code only asks whether it's enabled for this user/team. """ from django.conf import settings @@ -15,6 +16,7 @@ from products.feature_flags.backend.facade import api as feature_flags_facade AUTORESEARCH_FLAG = "autoresearch" +REPORT_NOTEBOOK_FLAG = "autoresearch-report-notebook" def has_autoresearch_access( @@ -22,6 +24,25 @@ def has_autoresearch_access( *, team_id: int | None = None, organization_id: str | None = None, +) -> bool: + return _flag_enabled(AUTORESEARCH_FLAG, user, team_id=team_id, organization_id=organization_id) + + +def has_report_notebook_access( + user: AbstractBaseUser | AnonymousUser | None, + *, + team_id: int | None = None, + organization_id: str | None = None, +) -> bool: + return _flag_enabled(REPORT_NOTEBOOK_FLAG, user, team_id=team_id, organization_id=organization_id) + + +def _flag_enabled( + flag: str, + user: AbstractBaseUser | AnonymousUser | None, + *, + team_id: int | None, + organization_id: str | None, ) -> bool: if not user or not user.is_authenticated: return False @@ -34,7 +55,7 @@ def has_autoresearch_access( # fail closed rather than grant access on any active flag row. # Don't apply this in TEST mode, because tests mock feature_enabled directly. if settings.DEBUG and not getattr(settings, "TEST", False): - return _local_flag_enabled(team_id=team_id) + return _local_flag_enabled(flag, team_id=team_id) if team_id is not None and organization_id is None: organization_id = _organization_id_for_team(team_id) @@ -46,7 +67,7 @@ def has_autoresearch_access( if organization_id is not None: return bool( posthog_feature_flag_value( - AUTORESEARCH_FLAG, + flag, distinct_id, organization_id=organization_id, team_id=team_id, @@ -62,7 +83,7 @@ def has_autoresearch_access( return bool( posthoganalytics.feature_enabled( - AUTORESEARCH_FLAG, + flag, distinct_id, groups=groups, group_properties=group_properties, @@ -77,5 +98,5 @@ def _organization_id_for_team(team_id: int) -> str | None: return str(organization_id) if organization_id else None -def _local_flag_enabled(*, team_id: int | None) -> bool: - return feature_flags_facade.flag_is_active(AUTORESEARCH_FLAG, team_id=team_id) +def _local_flag_enabled(flag: str, *, team_id: int | None) -> bool: + return feature_flags_facade.flag_is_active(flag, team_id=team_id) diff --git a/products/autoresearch/backend/facade/api.py b/products/autoresearch/backend/facade/api.py index 9305770ec29a..5a656ee37baf 100644 --- a/products/autoresearch/backend/facade/api.py +++ b/products/autoresearch/backend/facade/api.py @@ -1059,6 +1059,7 @@ def complete_run( model_explanation: dict[str, Any] | None = None, recommended_next: str = "", distillation: str = "", + report_notebook_short_id: str = "", ) -> TrainingRun: """Finalize a run. Promotion is server-side, so an agent cannot set the champion.""" # Promotion imports the inference sandbox, and with it pandas and pyarrow; the router imports @@ -1078,6 +1079,7 @@ def complete_run( model_explanation=model_explanation or {}, recommended_next=recommended_next or "", distillation=distillation or "", + report_notebook_short_id=report_notebook_short_id or "", ) except PromotionError as exc: raise AutoresearchConflict(str(exc)) from exc diff --git a/products/autoresearch/backend/facade/contracts.py b/products/autoresearch/backend/facade/contracts.py index 85e3b5b7277b..e2bd5e7b4c34 100644 --- a/products/autoresearch/backend/facade/contracts.py +++ b/products/autoresearch/backend/facade/contracts.py @@ -163,6 +163,7 @@ class TrainingRunSummary: dead_ends: list[TrainingRunSummaryLadderItem] recommended_next: str distillation: str + report_notebook_short_id: str @dataclass(frozen=True) diff --git a/products/autoresearch/backend/inference/scoring.py b/products/autoresearch/backend/inference/scoring.py index aa9ac74eba51..2f763a7dec18 100644 --- a/products/autoresearch/backend/inference/scoring.py +++ b/products/autoresearch/backend/inference/scoring.py @@ -94,6 +94,8 @@ class InferenceRunError(Exception): _RESERVED_COLS = frozenset({"distinct_id", _LABEL_COL, _FOLD_COL}) # The score columns scoring adds to a feature row, kept out of the features hash. _SCORE_KEYS = frozenset({"p_y", "p_y_raw"}) +# A capture error description can hold a URL and an exception repr, so the run error clips it. +_MAX_EMIT_ERROR_DESCRIPTION_CHARS = 200 # Namespace for deterministic prediction event UUIDs, so a retried scoring activity @@ -101,6 +103,20 @@ class InferenceRunError(Exception): _PREDICTION_UUID_NAMESPACE = uuid.UUID("6f9a4a24-0e5c-4a5a-9d0e-2f6a0f0b1c3d") +def _clip_middle(text: str, limit: int) -> str: + """ + Keep both ends of ``text``. A requests transport error starts with the target host and + ends with the cause, such as ``[Errno 111] Connection refused``, so a head-only clip + makes a refused connection and a failed DNS lookup look the same. + """ + if len(text) <= limit: + return text + marker = "..." + head = (limit - len(marker)) // 2 + tail = limit - len(marker) - head + return f"{text[:head]}{marker}{text[-tail:]}" + + def _is_uuid(value: str) -> bool: try: uuid.UUID(str(value)) @@ -500,11 +516,16 @@ def _emit_predictions( error=result.error, ) sample = [result.results.get(uid) for uid in result.warnings[:3]] + error_detail = "" + if result.error: + error_detail = f", {result.error.get('error')}" + if description := result.error.get("error_description"): + error_detail += f": {_clip_middle(str(description), _MAX_EMIT_ERROR_DESCRIPTION_CHARS)}" raise InferenceRunError( f"Prediction events were not all accepted ({len(result.dropped)} dropped, " f"{len(result.retried)} exhausted retries, {len(result.unaccounted)} unaccounted, " f"{len(result.warnings)} stored with a warning{f' e.g. {sample!r}' if sample else ''}" - f"{', ' + str(result.error.get('error')) if result.error else ''}); failing the run so it is retried" + f"{error_detail}); failing the run so it is retried" ) return _EmitResult( diff --git a/products/autoresearch/backend/inference/test_inference.py b/products/autoresearch/backend/inference/test_inference.py index fa2e5c6d616c..75799e28f97b 100644 --- a/products/autoresearch/backend/inference/test_inference.py +++ b/products/autoresearch/backend/inference/test_inference.py @@ -53,6 +53,11 @@ {"distinct_id": "user-1", "events_total_30d": 50, "days_since_last_seen": 2}, {"distinct_id": "user-2", "events_total_30d": 10, "days_since_last_seen": 15}, ] +_REFUSED_CONNECTION_ERROR = ( + "HTTPConnectionPool(host='capture.example.com', port=8010): Max retries exceeded with url: " + "/i/v1/analytics/events (Caused by NewConnectionError(': Failed to establish a new connection: [Errno 111] Connection refused'))" +) def _accepted(events: list[dict]) -> CaptureInternalResult: @@ -195,13 +200,26 @@ def test_run_inference_zero_rows_completes_without_emitting(self): @parameterized.expand( [ - ("transport_failure", Exception("capture unavailable"), None), + ("transport_failure", Exception("capture unavailable"), None, "capture unavailable"), + ( + "transport_error_result", + None, + lambda events: CaptureInternalResult( + status_code=0, + error={"error": "transport_error", "error_description": _REFUSED_CONNECTION_ERROR}, + unaccounted=[event["event_uuid"] for event in events], + ), + "transport_error: HTTPConnectionPool(host='capture.example.com', port=8010): Max retries exceeded " + "with url: /i/v1/an... object at 0x7f0000000000>: Failed to establish a new connection: " + "[Errno 111] Connection refused')))", + ), ( "one_event_dropped", None, lambda events: CaptureInternalResult( status_code=200, ok=[events[0]["event_uuid"]], dropped=[events[1]["event_uuid"]] ), + "1 dropped", ), ( "one_event_stored_with_a_warning", @@ -212,20 +230,22 @@ def test_run_inference_zero_rows_completes_without_emitting(self): warnings=[events[1]["event_uuid"]], results={events[1]["event_uuid"]: {"result": "warning", "message": "person processing disabled"}}, ), + "person processing disabled", ), ] ) - def test_any_emit_failure_fails_the_run(self, _name, side_effect, result_for): + def test_any_emit_failure_fails_the_run(self, _name, side_effect, result_for, expected_message): # Completing with a partial batch advanced last_scored_at past the people who never # received their prediction; the deterministic UUIDs make a full replay safe instead. pipeline, model = self._make_pipeline_and_model() capture = MagicMock(side_effect=side_effect or (lambda **kwargs: result_for(kwargs["events"]))) - with self.assertRaises(InferenceRunError): + with self.assertRaisesMessage(InferenceRunError, expected_message): self._run_live(pipeline, model, capture) run = AutoresearchRun.objects.filter(pipeline=pipeline).latest("created_at") assert run.status == AutoresearchRun.Status.FAILED + assert expected_message in run.error pipeline.refresh_from_db() assert pipeline.last_scored_at is None diff --git a/products/autoresearch/backend/presentation/views/serializers.py b/products/autoresearch/backend/presentation/views/serializers.py index 712deb36ba27..9f406fa81749 100644 --- a/products/autoresearch/backend/presentation/views/serializers.py +++ b/products/autoresearch/backend/presentation/views/serializers.py @@ -893,6 +893,12 @@ class TrainingRunSummarySerializer(serializers.Serializer): distillation = serializers.CharField( allow_blank=True, help_text="Agent's 1–2 sentence distillation of what this run learned. Empty if not provided." ) + report_notebook_short_id = serializers.CharField( + required=False, + default="", + allow_blank=True, + help_text="Short id of the report notebook the agent built for this run. Empty if there is none.", + ) @extend_schema_serializer(component_name="IterationTrail") @@ -1424,6 +1430,15 @@ class CompleteTrainingRunSerializer(serializers.Serializer): "dead-ends. Stored in the run summary as the cheapest thing the next run reads. Max 2000 characters." ), ) + report_notebook_short_id = serializers.CharField( + required=False, + allow_blank=True, + default="", + help_text=( + "Short id of the report notebook you built for this run. Stored in the run summary only if the " + "notebook exists in this project; an unknown id is dropped and does not fail the completion." + ), + ) # ── Feature materialization serializers ───────────────────────────────────── diff --git a/products/autoresearch/backend/presentation/views/views.py b/products/autoresearch/backend/presentation/views/views.py index 5149ef8bf31a..d70ba38d9a58 100644 --- a/products/autoresearch/backend/presentation/views/views.py +++ b/products/autoresearch/backend/presentation/views/views.py @@ -942,6 +942,7 @@ def complete(self, request: Request, *args: Any, **kwargs: Any) -> Response: model_explanation=data.get("model_explanation") or {}, recommended_next=data.get("recommended_next") or "", distillation=data.get("distillation") or "", + report_notebook_short_id=data.get("report_notebook_short_id") or "", ) except TrainingRunNotFound: raise NotFound("Training run not found.") diff --git a/products/autoresearch/backend/training/AGENTS.md b/products/autoresearch/backend/training/AGENTS.md index 703e0d3790de..5cc60d1bcac7 100644 --- a/products/autoresearch/backend/training/AGENTS.md +++ b/products/autoresearch/backend/training/AGENTS.md @@ -13,6 +13,7 @@ The other half is `../inference/`, which consumes what this package produces and The real path. `run_training()` creates the `AutoresearchTrainingRun` (status `RUNNING`) and fires `Task.create_and_run()` with `internal=True` and no repository, so the run shows up as an internal Task rather than in the normal Tasks list. The brief carries user-authored text, so the sandbox token holds only `TRAINING_MCP_SCOPES` (the `execute-sql` reads, the autoresearch scopes, and `user:read`, which the PostHog MCP server needs to start a session), and an empty connector allowlist keeps the team's shared MCP connectors out of the sandbox. `build_agent_description()` assembles the agent's brief — the target, the horizon, the population, and the contract for the bundle it must author. + When the `autoresearch-report-notebook` flag is on for the launching user, the token also holds `REPORT_NOTEBOOK_MCP_SCOPES` and the brief adds a Finalize step: the agent builds one report notebook from the `system.autoresearch_*` tables and passes its `short_id` to complete. `report.md` stays required either way. The agent drives the rest _itself_ through the `autoresearch-*` MCP tools: it records each iteration, uploads the bundle, and calls complete. Nothing polls it. - `stub.py` `run_stub_training()` — a hand-authored champion recipe with universal engagement features (event counts, distinct event types, days since first seen) that apply to any team and any target. @@ -27,6 +28,7 @@ The other half is `../inference/`, which consumes what this package produces and `_detect_uploaded_bundle()` decides whether the new model gets an `artifact_prefix` (bundle path) or only a recorded recipe (legacy path). The bundle is written once per run, so a losing iteration can overwrite it: the uploaded `features.sql` must match the `feature_sql` recorded by the selected iteration, whitespace aside, or promotion raises rather than publishing a champion whose recipe and score describe other code. `complete_training_run()` reads the bundle and enters the run's `team_scope()` before it opens the transaction, because the `TaskRun` safety net calls it from a worker thread with no request scope, and object-storage calls must not run under the row lock. + The agent's `report_notebook_short_id` goes into the run summary only if that notebook exists in the run's team. A bad id or a failed check stores an empty value and never fails completion. Only a promoted model is fitted. A challenger's `model.pkl` would never be read, because inference serves the champion and no path promotes a challenger row later. - `artifacts.py` Object storage for the bundle: `features.sql`, `train.py`, `predict.py`, plus the fitted `model.pkl` written at completion. diff --git a/products/autoresearch/backend/training/promotion.py b/products/autoresearch/backend/training/promotion.py index cf43c70feed5..e83a065b2f90 100644 --- a/products/autoresearch/backend/training/promotion.py +++ b/products/autoresearch/backend/training/promotion.py @@ -39,6 +39,7 @@ ) from products.autoresearch.backend.training import artifacts from products.autoresearch.backend.training.recipe_validation import RecipeValidationError, validate_model_class +from products.notebooks.backend.facade import api as notebooks_facade logger = structlog.get_logger(__name__) @@ -166,6 +167,7 @@ def _build_run_summary( champion_model_class: str, recommended_next: str, distillation: str, + report_notebook_short_id: str, ) -> dict[str, Any]: """Tier-1 cross-run memory: backend derives the structural facts; the agent supplies the two judgment fields (recommended_next, distillation). Read back by a new run before it iterates.""" @@ -186,6 +188,7 @@ def _build_run_summary( "dead_ends": [_summary_item(it) for it in dead_ends], "recommended_next": recommended_next or "", "distillation": distillation or "", + "report_notebook_short_id": report_notebook_short_id, } @@ -305,6 +308,7 @@ def complete_training_run( model_explanation: dict[str, Any] | None = None, recommended_next: str = "", distillation: str = "", + report_notebook_short_id: str = "", ) -> dict[str, Any]: """Finalize a run: pick the best iteration, decide champion vs challenger, persist the model.""" # The TaskRun safety net calls this from a worker thread, where no request has set a @@ -323,9 +327,28 @@ def complete_training_run( model_explanation=model_explanation, recommended_next=recommended_next, distillation=distillation, + report_notebook_short_id=_verified_report_notebook(current, report_notebook_short_id), ) +def _verified_report_notebook(training_run: AutoresearchTrainingRun, short_id: str) -> str: + """ + The agent's notebook short id if that notebook exists in the run's team, else "". + The model result matters more than the report, so a bad id or a failed check never fails completion. + """ + short_id = (short_id or "").strip() + if not short_id: + return "" + try: + if notebooks_facade.notebook_exists(training_run.team_id, short_id, include_deleted=False): + return short_id + except Exception: + logger.exception("autoresearch_report_notebook_check_failed", training_run_id=str(training_run.pk)) + return "" + logger.warning("autoresearch_report_notebook_not_found", training_run_id=str(training_run.pk)) + return "" + + def _activate_pipeline(pipeline: AutoresearchPipeline) -> None: """A pipeline with its first champion goes live: flip Draft/Bootstrapping -> Running so the daily coordinator starts scoring it. Mirrors the stub path (stub_training); pause/resume @@ -371,6 +394,7 @@ def _finalize_under_lock( model_explanation: dict[str, Any] | None, recommended_next: str, distillation: str, + report_notebook_short_id: str, ) -> dict[str, Any]: # Re-fetch under lock and re-check status inside the transaction. Both callers (the # complete API action and the TaskRun post_save safety net) guard on status outside @@ -468,6 +492,7 @@ def _finalize_under_lock( champion_model_class=_serving_model_class(promoted=promoted, model=model, incumbent=current), recommended_next=recommended_next, distillation=distillation, + report_notebook_short_id=report_notebook_short_id, ) training_run.save(update_fields=["status", "iteration_count", "best_holdout_score", "summary", "completed_at"]) diff --git a/products/autoresearch/backend/training/runner.py b/products/autoresearch/backend/training/runner.py index ba4293bcf27f..02f64872bdea 100644 --- a/products/autoresearch/backend/training/runner.py +++ b/products/autoresearch/backend/training/runner.py @@ -33,8 +33,10 @@ from posthog.hogql.property import action_to_expr from posthog.dataclasses import frozen +from posthog.models.user import User from products.actions.backend.models.action import Action +from products.autoresearch.backend.access import has_report_notebook_access from products.autoresearch.backend.dataset.labeling import TrainingSample, build_target_condition from products.autoresearch.backend.inference.sandbox import _resolve_acting_user, measure_training_sample from products.autoresearch.backend.models import AutoresearchPipeline, AutoresearchSuggestion, AutoresearchTrainingRun @@ -73,6 +75,11 @@ # The brief carries user-authored text, so the token grants nothing beyond that. TRAINING_MCP_SCOPES = ["query:read", "insight:read", "user:read", "autoresearch:read", "autoresearch:write"] +# Added only when the report notebook flag is on for the user who starts the run. +# notebook:write also exposes notebooks-partial-update and notebooks-destroy, so the brief +# limits the agent to the notebook it creates in this run. +REPORT_NOTEBOOK_MCP_SCOPES = ["notebook:read", "notebook:write"] + # Task.title is a 255-character column, and a pipeline name and target event can each # take all of it. _TASK_TITLE_MAX_CHARS = 255 @@ -141,12 +148,62 @@ def _describe_training_sample(sample: TrainingSample | None) -> str: return clause +def _report_notebook_step(pipeline: AutoresearchPipeline, *, training_run_id: str, today_iso: str) -> str: + """The Finalize step that builds the report notebook, indented to sit inside the brief.""" + step = textwrap.dedent(f""" + 3. **Build the report notebook** — a live copy of the report whose numbers come from SQL + cells, so a reader can check them and re-run them after scoring. `report.md` stays the + fallback: write it first, whatever happens in this step. + + Do this step only if `notebooks-create-markdown` and `notebooks-add-cell` are in your + tool list. If they are not, skip to the next step. + + Create exactly ONE notebook with `notebooks-create-markdown`. Title it + ` · model report · {today_iso}`, where the pipeline name is + {_wrap_untrusted(pipeline.name)}. Change and run only this notebook. Never update, + delete, or run any other notebook. + + Build it in this order, with markdown prose between the cells: + - **TL;DR** and **What it predicts** — the same content as `report.md`. + - **How training went** — a SQL cell over `system.autoresearch_iterations` where + `training_run_id = '{training_run_id}'`, then a Python cell that plots holdout AUC by + iteration and marks kept and discarded iterations. + - **How well it works** — a SQL cell over `system.autoresearch_models` where + `pipeline_id = '{pipeline.pk}'`: role, holdout AUC, realized AUC, calibration error, + and lift@10/@20 from `metrics` when present. Explain them in plain words. + - **What drives it** — a Python cell that charts the feature importances and direction + in `model_explanation` of this run's model row + (`source_training_run_id = '{training_run_id}'`), then prose on the intuition behind + each top feature. + - **Live performance** — a SQL cell over `events` where + `event = 'autoresearch_prediction'` and + `properties.$autoresearch_pipeline_id = '{pipeline.pk}'`, then Python cells for the + score histogram and the realized vs predicted rate by decile. A new model has no + predictions yet, so these cells must handle an empty result: print a clear message + such as "No predictions yet. Re-run after the first scoring run." and do not fail. + - **How it was built** and **Caveats and recommended use** — prose. + + Rules for every cell: + - Every number comes from a SQL cell. Do not type metrics into Python or prose tables. + - Python cells work only on the dataframes of earlier cells. No network access: no + `requests`, `urllib`, `http`, `socket`, or `subprocess`. No file reads or writes, + and no package installs. + - One figure per Python cell. The kernel keeps at most 8 figures and about 3 MB of + images per cell. + - Do not call `notebooks-configure-compute`. Use the default kernel. + - Run each cell. If a cell fails, fix it or delete it. Never leave a failed cell. + + Keep the notebook's `short_id` for the next step.""") + return textwrap.indent(step, " " * 8) + + def build_agent_description( pipeline: AutoresearchPipeline, iteration_budget: int, training_run_id: str, pending_suggestions: list[AutoresearchSuggestion] | None = None, training_sample: TrainingSample | None = None, + report_notebook: bool = False, ) -> str: """Build the Claude Code agent prompt for the autoresearch training loop.""" pop_clause = "" @@ -174,6 +231,16 @@ def build_agent_description( today_iso = date.today().isoformat() min_iters = min(3, iteration_budget) target = _describe_target(pipeline) + complete_step = 3 + notebook_step = "" + notebook_field = "" + if report_notebook: + complete_step = 4 + notebook_step = _report_notebook_step(pipeline, training_run_id=training_run_id, today_iso=today_iso) + notebook_field = ( + "\n - `report_notebook_short_id`: the `short_id` of the notebook from step 3. Omit it\n" + " if you skipped step 3 or the notebook does not exist." + ) prompt = textwrap.dedent(f""" # PostHog Autoresearch Agent @@ -530,14 +597,14 @@ def load_xy(fpath, lpath): Add a calibration line (predicted vs realized rate) if it aids the story. Where a chart would be overkill (or mermaid can't express it), fall back to compact ASCII/unicode bar charts inline — they render in any Markdown surface. Use plain GFM tables for the metrics - block. If a user suggestion asks for a particular audience or emphasis, honor it. - 3. Call `autoresearch-training-runs-complete-create` with `pipeline_id = "{pipeline.pk}"` + block. If a user suggestion asks for a particular audience or emphasis, honor it.{notebook_step} + {complete_step}. Call `autoresearch-training-runs-complete-create` with `pipeline_id = "{pipeline.pk}"` and `id = "{training_run_id}"`. The backend picks the best iteration, decides champion vs challenger, and attaches your uploaded bundle as the model's artifact. Also pass two short fields that become this run's learning memory for the NEXT run: - `distillation`: 1–2 sentences on what this run learned — the winning signal, the key transform, the dead-ends. This is the cheapest thing the next run reads. - - `recommended_next`: concretely what a future run should try next given what you found. + - `recommended_next`: concretely what a future run should try next given what you found.{notebook_field} The backend derives the rest of the summary (the kept ladder and dead-ends) from your recorded iterations, so keep these two fields to judgment only — do not restate the ladder. @@ -604,6 +671,16 @@ def _training_sample_for_brief(pipeline: AutoresearchPipeline) -> TrainingSample return None +def _report_notebook_enabled(pipeline: AutoresearchPipeline, *, user_id: int) -> bool: + """A flag check that fails leaves the notebook out rather than failing the launch.""" + try: + user = User.objects.filter(pk=user_id).first() + return has_report_notebook_access(user, team_id=pipeline.team_id) + except Exception: + logger.warning("autoresearch_report_notebook_flag_check_failed", pipeline_id=str(pipeline.pk), exc_info=True) + return False + + def run_training( pipeline: AutoresearchPipeline, iteration_budget: int, @@ -625,6 +702,9 @@ def run_training( # Completion fits the champion as the pipeline's creator, so a creator who has left # would consume the paid run and leave a champion that no scoring run can load. _resolve_acting_user(team=pipeline.team, pipeline=pipeline, user=None) + # The MCP token belongs to user_id, so the flag is evaluated for the same user. + report_notebook = _report_notebook_enabled(pipeline, user_id=user_id) + mcp_scopes = TRAINING_MCP_SCOPES + REPORT_NOTEBOOK_MCP_SCOPES if report_notebook else TRAINING_MCP_SCOPES # Every materialization labels through this condition, so a target it refuses (a deleted # action, or one with no steps) would fail the whole paid run. build_target_condition( @@ -662,6 +742,7 @@ def run_training( training_run_id=str(training_run.id), pending_suggestions=pending_suggestions or None, training_sample=_training_sample_for_brief(pipeline), + report_notebook=report_notebook, ) title = f"[autoresearch] {pipeline.name}: learn to predict '{pipeline.target_event}'" @@ -675,7 +756,7 @@ def run_training( create_pr=False, mode="background", internal=True, - posthog_mcp_scopes=TRAINING_MCP_SCOPES, + posthog_mcp_scopes=mcp_scopes, # The autoresearch image is the agent-capable base plus pandas/numpy/ # scikit-learn/pyarrow at system site. The base image lacks the ML libs; the # notebook image has the libs but cannot host the agent server — only this diff --git a/products/autoresearch/backend/training/test_promotion.py b/products/autoresearch/backend/training/test_promotion.py index a0542590d9ca..aee4e2399ec0 100644 --- a/products/autoresearch/backend/training/test_promotion.py +++ b/products/autoresearch/backend/training/test_promotion.py @@ -11,6 +11,7 @@ from parameterized import parameterized from posthog.models.scoping import unscoped +from posthog.models.team import Team from posthog.storage.object_storage import ObjectStorageError from products.autoresearch.backend.models import ( @@ -23,6 +24,7 @@ from products.autoresearch.backend.training.artifacts import ArtifactBundle, InvalidArtifactContent, PartialBundle from products.autoresearch.backend.training.promotion import PromotionError, complete_training_run from products.autoresearch.backend.training.stub import run_stub_training +from products.notebooks.backend.facade import api as notebooks_facade ANCHORED_FEATURE_SQL = "SELECT a.person_id AS distinct_id, count() AS c FROM {anchors} a GROUP BY a.person_id" _DEFAULT_PARAMS = object() @@ -328,6 +330,23 @@ def test_bundle_sql_the_fit_cannot_run_blocks_promotion(self, _name, features_sq assert not AutoresearchModel.objects.filter(pipeline=self.pipeline).exists() + @parameterized.expand([("own_team", "own", True), ("other_team", "other", False), ("missing", "none", False)]) + def test_report_notebook_is_linked_only_when_it_exists_in_the_run_team(self, _name, owner, linked): + if owner == "none": + short_id = "doesnotexist" + else: + team_id = self.team.pk if owner == "own" else Team.objects.create(organization=self.organization).pk + short_id = notebooks_facade.create_notebook(team_id, title="Report", content=None).short_id + run = self._run() + self._iteration(run, number=0, holdout=0.8) + + result = complete_training_run(run, report_notebook_short_id=short_id) + + assert result["promoted"] is True + run.refresh_from_db() + assert run.status == AutoresearchTrainingRun.Status.COMPLETED + assert run.summary["report_notebook_short_id"] == (short_id if linked else "") + def test_completion_runs_without_an_ambient_team_scope(self): # The TaskRun safety net finalizes a run from a worker thread, where no request has # set a scope. Every read in promotion goes through a fail-closed manager. diff --git a/products/autoresearch/backend/training/test_training.py b/products/autoresearch/backend/training/test_training.py index 3434935ddc89..775b0ac6cb31 100644 --- a/products/autoresearch/backend/training/test_training.py +++ b/products/autoresearch/backend/training/test_training.py @@ -15,6 +15,7 @@ from products.autoresearch.backend.models import AutoresearchPipeline, AutoresearchSuggestion, AutoresearchTrainingRun from products.autoresearch.backend.testing import TeamScopedTestMixin from products.autoresearch.backend.training.runner import ( + REPORT_NOTEBOOK_MCP_SCOPES, TRAINING_MCP_SCOPES, UNTRUSTED_DATA_TAG, build_agent_description, @@ -35,9 +36,12 @@ def _make_pipeline(self) -> AutoresearchPipeline: iteration_budget_remaining=10, ) - def test_prompt_renders_without_unresolved_placeholders(self) -> None: + @parameterized.expand([("without_notebook", False), ("with_notebook", True)]) + def test_prompt_renders_without_unresolved_placeholders(self, _name: str, report_notebook: bool) -> None: pipeline = self._make_pipeline() - prompt = build_agent_description(pipeline=pipeline, iteration_budget=5, training_run_id="run-123") + prompt = build_agent_description( + pipeline=pipeline, iteration_budget=5, training_run_id="run-123", report_notebook=report_notebook + ) # `{anchors}` and `{lookback_days}` are intentional — they are documented # placeholders the agent is taught to use inside its own SQL, and `{init}` # is the literal mermaid `%%{init}%%` directive the report section forbids. @@ -96,13 +100,19 @@ def test_prompt_drives_artifact_bundle_flow_not_set_output(self) -> None: assert "set_output/" not in prompt assert "recipe.json" not in prompt - def test_prompt_instructs_report_md(self) -> None: + @parameterized.expand([("without_notebook", False), ("with_notebook", True)]) + def test_prompt_instructs_report_md(self, _name: str, report_notebook: bool) -> None: pipeline = self._make_pipeline() - prompt = build_agent_description(pipeline=pipeline, iteration_budget=5, training_run_id="run-123") + prompt = build_agent_description( + pipeline=pipeline, iteration_budget=5, training_run_id="run-123", report_notebook=report_notebook + ) # The agent must author a portable report.md, uploaded like the bundle files, with charts. assert "report.md" in prompt assert "mermaid" in prompt assert "autoresearch-training-runs-artifacts-upload-create" in prompt + assert ("notebooks-create-markdown" in prompt) is report_notebook + assert ("report_notebook_short_id" in prompt) is report_notebook + assert prompt.index("report.md") < prompt.index("autoresearch-training-runs-complete-create") def test_prompt_excludes_autoresearch_feedback_events(self) -> None: pipeline = self._make_pipeline() @@ -172,13 +182,26 @@ def _dispatched(self, facade: MagicMock) -> None: facade.create_and_run_task.return_value = MagicMock(task_id=uuid.uuid4(), latest_run=MagicMock(id=uuid.uuid4())) facade.task_run_is_terminal.return_value = False - def test_dispatch_grants_only_training_scopes_and_stamps_the_run(self, facade: MagicMock) -> None: + @parameterized.expand( + [ + ("flag_off", False, TRAINING_MCP_SCOPES), + ("flag_on", True, TRAINING_MCP_SCOPES + REPORT_NOTEBOOK_MCP_SCOPES), + ] + ) + def test_dispatch_grants_only_training_scopes_and_stamps_the_run( + self, facade: MagicMock, _name: str, flag_on: bool, expected_scopes: list[str] + ) -> None: self._dispatched(facade) - training_run = run_training(self.pipeline, iteration_budget=5, user_id=self.user.id) + with patch( + "products.autoresearch.backend.training.runner.has_report_notebook_access", return_value=flag_on + ) as flag: + training_run = run_training(self.pipeline, iteration_budget=5, user_id=self.user.id) + assert flag.call_args.args[0] == self.user kwargs = facade.create_and_run_task.call_args.kwargs - assert kwargs["posthog_mcp_scopes"] == TRAINING_MCP_SCOPES + assert kwargs["posthog_mcp_scopes"] == expected_scopes + assert ("notebooks-create-markdown" in kwargs["description"]) is flag_on assert "user:read" in kwargs["posthog_mcp_scopes"] assert kwargs["extra_run_state"] == { "autoresearch_training_run_id": str(training_run.id), diff --git a/products/autoresearch/frontend/generated/api.schemas.ts b/products/autoresearch/frontend/generated/api.schemas.ts index 2f4645ea3176..368ca65827e3 100644 --- a/products/autoresearch/frontend/generated/api.schemas.ts +++ b/products/autoresearch/frontend/generated/api.schemas.ts @@ -674,6 +674,8 @@ export interface TrainingRunSummaryApi { recommended_next: string /** Agent's 1–2 sentence distillation of what this run learned. Empty if not provided. */ distillation: string + /** Short id of the report notebook the agent built for this run. Empty if there is none. */ + report_notebook_short_id?: string } /** @@ -917,6 +919,8 @@ export interface CompleteTrainingRunApi { * @maxLength 2000 */ distillation?: string + /** Short id of the report notebook you built for this run. Stored in the run summary only if the notebook exists in this project; an unknown id is dropped and does not fail the completion. */ + report_notebook_short_id?: string } export type RecordIterationApiRecipeSnapshotFeatureTransformsItem = { [key: string]: unknown } diff --git a/products/autoresearch/frontend/generated/api.zod.ts b/products/autoresearch/frontend/generated/api.zod.ts index 1d4df754342f..751f48e3d8b3 100644 --- a/products/autoresearch/frontend/generated/api.zod.ts +++ b/products/autoresearch/frontend/generated/api.zod.ts @@ -248,6 +248,8 @@ export const autoresearchTrainingRunsCompleteCreateBodyRecommendedNextMax = 2000 export const autoresearchTrainingRunsCompleteCreateBodyDistillationDefault = `` export const autoresearchTrainingRunsCompleteCreateBodyDistillationMax = 2000 +export const autoresearchTrainingRunsCompleteCreateBodyReportNotebookShortIdDefault = `` + export const AutoresearchTrainingRunsCompleteCreateBody = /* @__PURE__ */ zod .object({ best_iteration_id: zod @@ -274,6 +276,12 @@ export const AutoresearchTrainingRunsCompleteCreateBody = /* @__PURE__ */ zod .describe( 'A 1–2 sentence distillation of what this run learned — the winning signal, the key transform, the dead-ends. Stored in the run summary as the cheapest thing the next run reads. Max 2000 characters.' ), + report_notebook_short_id: zod + .string() + .default(autoresearchTrainingRunsCompleteCreateBodyReportNotebookShortIdDefault) + .describe( + 'Short id of the report notebook you built for this run. Stored in the run summary only if the notebook exists in this project; an unknown id is dropped and does not fail the completion.' + ), }) .describe('Input for finalizing a training run. The backend selects\/promotes the champion.') diff --git a/products/autoresearch/mcp/tools.yaml b/products/autoresearch/mcp/tools.yaml index 21a898760612..87c83c366bc2 100644 --- a/products/autoresearch/mcp/tools.yaml +++ b/products/autoresearch/mcp/tools.yaml @@ -507,7 +507,7 @@ tools: model card. Also pass distillation (1–2 sentences on what this run learned) and recommended_next (what a future run should try next) — these are stored in the run summary and read back by the next run during orientation; the backend derives the rest of the summary (kept ladder, dead-ends) from your recorded - iterations. + iterations. If you built a report notebook for this run, pass its short_id as report_notebook_short_id. feature_flag: autoresearch response: include: diff --git a/products/data_warehouse/frontend/scenes/DataOpsScene/DataWarehouseScene.tsx b/products/data_warehouse/frontend/scenes/DataOpsScene/DataWarehouseScene.tsx index 09268ff869e6..3d3fdc130639 100644 --- a/products/data_warehouse/frontend/scenes/DataOpsScene/DataWarehouseScene.tsx +++ b/products/data_warehouse/frontend/scenes/DataOpsScene/DataWarehouseScene.tsx @@ -12,8 +12,6 @@ import { SceneContent } from '~/layout/scenes/components/SceneContent' import { SceneTitleSection } from '~/layout/scenes/components/SceneTitleSection' import { ProductKey } from '~/queries/schema/schema-general' -import { DataQualityOverview } from 'products/data_quality/frontend/overview/DataQualityOverview' - import { DataWarehouseTab, dataWarehouseSceneLogic } from './dataWarehouseSceneLogic' import { MonitoringTab } from './tabs/MonitoringTab' import { OverviewTab } from './tabs/OverviewTab' @@ -29,7 +27,6 @@ const TAB_LABELS: Record = { [DataWarehouseTab.OVERVIEW]: 'Overview', [DataWarehouseTab.MONITORING]: 'Monitoring', [DataWarehouseTab.SETTINGS]: 'Settings', - [DataWarehouseTab.DATA_QUALITY]: 'Data quality', } function tabContent(tab: DataWarehouseTab): JSX.Element { @@ -40,8 +37,6 @@ function tabContent(tab: DataWarehouseTab): JSX.Element { return case DataWarehouseTab.SETTINGS: return - case DataWarehouseTab.DATA_QUALITY: - return } } diff --git a/products/data_warehouse/frontend/scenes/DataOpsScene/dataWarehouseSceneLogic.ts b/products/data_warehouse/frontend/scenes/DataOpsScene/dataWarehouseSceneLogic.ts index 562af57d5232..fd7d93728646 100644 --- a/products/data_warehouse/frontend/scenes/DataOpsScene/dataWarehouseSceneLogic.ts +++ b/products/data_warehouse/frontend/scenes/DataOpsScene/dataWarehouseSceneLogic.ts @@ -15,7 +15,6 @@ export enum DataWarehouseTab { OVERVIEW = 'overview', MONITORING = 'monitoring', SETTINGS = 'settings', - DATA_QUALITY = 'data-quality', } function isDataWarehouseTab(tab: unknown): tab is DataWarehouseTab { @@ -122,9 +121,6 @@ export const dataWarehouseSceneLogic = kea([ tabs.push(DataWarehouseTab.OVERVIEW) tabs.push(DataWarehouseTab.MONITORING) } - if (featureFlags[FEATURE_FLAGS.DATA_QUALITY_CHECKS]) { - tabs.push(DataWarehouseTab.DATA_QUALITY) - } if (featureFlags[FEATURE_FLAGS.DATA_WAREHOUSE_SCENE]) { tabs.push(DataWarehouseTab.SETTINGS) } diff --git a/products/metrics/backend/diagnostics.py b/products/metrics/backend/diagnostics.py deleted file mode 100644 index 0ee3419a0c49..000000000000 --- a/products/metrics/backend/diagnostics.py +++ /dev/null @@ -1,330 +0,0 @@ -"""Recompute one chart point from its raw samples, and show the working. - -A metrics chart is several reductions deep by the time it reaches a pixel, and -every one of them returns a plausible-looking number when it is wrong. The only -way to know a point is right is to take the bucket apart: which series reported, -what each one sent, what each collapsed to, and how those combined. - -`decompose_bucket` does that twice over. It reduces the raw samples in Python -through `fundamentals`, and separately asks `MetricQueryRunner` for the same -point. Two independent paths to one number means a disagreement is visible -rather than inferred — and the per-series breakdown alongside it shows which -step diverged. - -The Python side is deliberately not built from the HogQL builders. A reference -that shares its assumptions with the thing it checks agrees with it by -construction and catches nothing. -""" - -from __future__ import annotations - -import datetime as dt -from collections.abc import Sequence -from dataclasses import replace - -from posthog.hogql import ast -from posthog.hogql.parser import parse_select -from posthog.hogql.query import execute_hogql_query - -from posthog.clickhouse.client.connection import Workload -from posthog.models import Team - -from products.metrics.backend.facade.contracts import ( - MetricBucketDecomposition, - MetricFilter, - MetricSampleView, - MetricSeriesBreakdown, -) -from products.metrics.backend.fundamentals import Sample, TemporalReducer, apply_plan, plan_reduction, reduce_temporal -from products.metrics.backend.metric_query_runner import ( - _QUERY_SETTINGS, - _interval_step, - counter_lookback, - points_query, - series_labels_query, - series_scope_expr, - type_filter_expr, -) -from products.metrics.backend.metric_samples_query_runner import build_metric_query_runner -from products.metrics.backend.metrics4_samples import reads_metrics4_only - -# How much of a bucket the breakdown lists. Totals are computed over everything -# in the bucket; these only bound what gets rendered, and the decomposition says -# so when it has trimmed something. -DEFAULT_MAX_SERIES = 20 -DEFAULT_MAX_SAMPLES_PER_SERIES = 12 - -# A bucket on a wide metric can hold millions of rows. Reading every one to -# explain a single point is not worth the cluster time, so the raw read is -# bounded and reports when it hit the bound. -_MAX_ROWS_READ = 50000 - - -def _as_utc(timestamp: dt.datetime) -> dt.datetime: - """ClickHouse hands timestamps back naive; the bucket edges are aware.""" - return timestamp.replace(tzinfo=dt.UTC) if timestamp.tzinfo is None else timestamp.astimezone(dt.UTC) - - -def _raw_samples_query( - *, - metric_name: str, - date_from: dt.datetime, - bucket_end: dt.datetime, - filters: Sequence[MetricFilter], - metric_type: str | None, - timezone: str, -) -> ast.SelectQuery: - # The labels are joined on after the LIMIT so the row bound applies to the - # data points read, not to the join output. A series without a row yet - # keeps its samples and shows empty labels. - query = parse_select( - """ - SELECT - s.series_fingerprint, - s.service_name, - ser.attributes, - ser.resource_attributes, - s.metric_type, - s.aggregation_temporality, - s.timestamp, - s.value - FROM ( - SELECT - series_fingerprint, - service_name, - metric_type, - aggregation_temporality, - timestamp, - value - FROM {points} - ORDER BY timestamp ASC - LIMIT {row_limit} - ) AS s - LEFT JOIN {series_labels} AS ser ON s.series_fingerprint = ser.series_fingerprint - ORDER BY s.timestamp ASC - """, - placeholders={ - "points": points_query( - from_samples=reads_metrics4_only(date_from), - columns=( - "series_fingerprint", - "service_name", - "metric_type", - "aggregation_temporality", - "timestamp", - "value", - ), - metric_names=(metric_name,), - date_from=date_from, - date_to=bucket_end, - timezone=timezone, - row_filters=(series_scope_expr(metric_name, filters), type_filter_expr(metric_type)), - ), - "row_limit": ast.Constant(value=_MAX_ROWS_READ), - "series_labels": series_labels_query(metric_name), - }, - ) - assert isinstance(query, ast.SelectQuery) - return query - - -def _actual_value( - *, - team: Team, - metric_name: str, - aggregation: str, - bucket_start: dt.datetime, - bucket_end: dt.datetime, - interval: str, - filters: Sequence[MetricFilter], - metric_type: str | None, - quantile: float | None, -) -> float | None: - """What the product would plot for this point, through the real runner. - - The runner reaches back past `date_from` on its own for the counter - functions' predecessor sample, so this asks for exactly the one bucket the - decomposition is explaining. - """ - rows = build_metric_query_runner( - team=team, - metric_name=metric_name, - aggregation=aggregation, - date_from=bucket_start, - date_to=bucket_end, - interval=interval, - filters=filters, - metric_type=metric_type, - quantile=quantile, - ).run() - for row in rows: - if _as_utc(dt.datetime.fromisoformat(row["time"])) == bucket_start: - return row["value"] - return None - - -def decompose_bucket( - *, - team: Team, - metric_name: str, - aggregation: str, - bucket_start: dt.datetime, - interval: str, - filters: Sequence[MetricFilter] = (), - metric_type: str | None = None, - quantile: float | None = None, - max_series: int = DEFAULT_MAX_SERIES, - max_samples_per_series: int = DEFAULT_MAX_SAMPLES_PER_SERIES, -) -> MetricBucketDecomposition: - """Take one chart point apart into the series and samples behind it.""" - bucket_start = _as_utc(bucket_start) - step = _interval_step(interval) - bucket_end = bucket_start + step - # The counter functions diff against the newest sample before the bucket, - # the way the chart's window function does, so their raw read reaches back - # over exactly the runner's lookback. A shorter reach here would find a - # different predecessor and report a disagreement the chart does not have. - needs_boundary = aggregation in ("rate", "increase") - read_from = bucket_start - counter_lookback(interval) if needs_boundary else bucket_start - - response = execute_hogql_query( - query_type="MetricBucketDecomposition", - query=_raw_samples_query( - metric_name=metric_name, - date_from=read_from, - bucket_end=bucket_end, - filters=filters, - metric_type=metric_type, - timezone=team.timezone, - ), - team=team, - workload=Workload.LOGS, - settings=_QUERY_SETTINGS, - ) - rows = response.results or [] - rows_truncated = len(rows) >= _MAX_ROWS_READ - - # Group the raw rows into series by the fingerprint ingest assigned, which - # is the same identity the chart's window functions partition on. - grouped: dict[int, list[Sample]] = {} - predecessors: dict[int, Sample] = {} - identities: dict[int, tuple[str, dict[str, str], dict[str, str]]] = {} - resolved_type = metric_type or "" - temporality = "" - for ( - key, - service_name, - attributes, - resource_attributes, - row_metric_type, - row_temporality, - timestamp, - value, - ) in rows: - sample = Sample(timestamp=_as_utc(timestamp), value=float(value)) - if sample.timestamp < bucket_start: - # Only a series' newest pre-bucket reading matters: it is the - # baseline its first in-bucket diff runs against. - held = predecessors.get(key) - if held is None or sample.timestamp > held.timestamp: - predecessors[key] = sample - else: - grouped.setdefault(key, []).append(sample) - identities.setdefault(key, (service_name, dict(attributes or {}), dict(resource_attributes or {}))) - # A bucket normally holds one type and one temporality; when a name has - # been ingested as several, the first is enough to plan a reduction and - # the type check reports the blend separately. - resolved_type = resolved_type or row_metric_type - temporality = temporality or row_temporality - - plan = plan_reduction( - aggregation=aggregation, - metric_type=resolved_type, - temporality=temporality, - interval_seconds=step.total_seconds(), - ) - if quantile is not None: - plan = replace(plan, quantile=quantile) - - # Delta increments before the bucket belong to the previous point, so only - # the odometer-style reduction gets its baseline prepended. - if plan.temporal is TemporalReducer.INCREASE: - reduction_input = { - key: ([predecessors[key], *samples] if key in predecessors else samples) for key, samples in grouped.items() - } - else: - reduction_input = grouped - - reference_value = apply_plan(reduction_input, plan) - - # Largest contributors first — that is what someone reading a surprising - # total wants to see, and it makes the trimmed tail the least interesting part. - ordered_keys = sorted(grouped, key=lambda key: (-len(grouped[key]), identities[key][0])) - breakdown: list[MetricSeriesBreakdown] = [] - for key in ordered_keys[:max_series]: - samples = grouped[key] - service_name, labels, resource_labels = identities[key] - if plan.temporal is TemporalReducer.POOLED_SAMPLES: - series_value = None - else: - # Normalized the same way as the bucket's total, so the series - # a reader adds up still reach the number they are explaining. - reduced = reduce_temporal(reduction_input[key], plan.temporal) - series_value = None if reduced is None else reduced / plan.divisor - breakdown.append( - MetricSeriesBreakdown( - service_name=service_name, - labels=labels, - resource_labels=resource_labels, - samples=tuple( - MetricSampleView(time=sample.timestamp.isoformat(), value=sample.value) - for sample in samples[:max_samples_per_series] - ), - sample_count=len(samples), - samples_truncated=len(samples) > max_samples_per_series, - value=series_value, - ) - ) - - actual_value = _actual_value( - team=team, - metric_name=metric_name, - aggregation=aggregation, - bucket_start=bucket_start, - bucket_end=bucket_end, - interval=interval, - filters=filters, - metric_type=metric_type, - quantile=quantile, - ) - - return MetricBucketDecomposition( - metric_name=metric_name, - metric_type=resolved_type, - temporality=temporality, - aggregation=aggregation, - bucket_start=bucket_start.isoformat(), - interval=interval, - temporal_reducer=plan.temporal.value, - spatial_reducer=plan.spatial.value, - series=tuple(breakdown), - series_count=len(grouped), - sample_count=sum(len(samples) for samples in grouped.values()), - series_truncated=len(grouped) > max_series, - rows_truncated=rows_truncated, - reference_value=reference_value, - actual_value=actual_value, - # A truncated read means the reference covers only part of the bucket, - # so any verdict would be an artifact of the unequal inputs. - agrees=None if rows_truncated else _agrees(reference_value, actual_value), - ) - - -def _agrees(reference: float | None, actual: float | None) -> bool: - """Float reductions in ClickHouse and Python accumulate in different orders, - so exact equality would report noise as disagreement. The tolerance is far - tighter than any real reduction bug, which move totals by whole multiples.""" - if reference is None or actual is None: - return reference is None and actual is None - scale = max(abs(reference), abs(actual), 1.0) - return abs(reference - actual) <= 1e-9 * scale diff --git a/products/metrics/backend/facade/api.py b/products/metrics/backend/facade/api.py index 2365e90d6ca5..a6304e09fcd4 100644 --- a/products/metrics/backend/facade/api.py +++ b/products/metrics/backend/facade/api.py @@ -21,13 +21,11 @@ from products.error_tracking.backend.facade.api import list_spike_events from products.metrics.backend.anomaly import characterize_anomaly as _characterize_anomaly -from products.metrics.backend.diagnostics import decompose_bucket as _decompose_bucket from products.metrics.backend.facade.contracts import ( CompanionMetric, IncidentContext, InvestigationResult, MetricAnomalyReport, - MetricBucketDecomposition, MetricErrorSpike, MetricEventSample, MetricFilter, @@ -569,34 +567,3 @@ def investigate_incident(*, team: Team, context: IncidentContext) -> Investigati filters=filters, companions=context.companions, ) - - -def explain_metric_bucket( - *, - team: Team, - metric_name: str, - aggregation: str, - bucket_start: dt.datetime, - interval: str, - filters: Sequence[MetricFilter] = (), - metric_type: MetricType | None = None, - quantile: float | None = None, -) -> MetricBucketDecomposition: - """Take one chart point apart and show how it was built. - - Returns the series that reported in the bucket, the samples each sent, and - the two reductions that combined them, alongside both the value the product - would plot and the value recomputed independently from the raw samples. - Reading them side by side is what makes an aggregation bug visible instead - of merely plausible. The presentation layer surfaces `ValueError` as a 400. - """ - return _decompose_bucket( - team=team, - metric_name=metric_name, - aggregation=aggregation, - bucket_start=bucket_start, - interval=interval, - filters=filters, - metric_type=metric_type.value if metric_type is not None else None, - quantile=quantile, - ) diff --git a/products/metrics/backend/facade/contracts.py b/products/metrics/backend/facade/contracts.py index 38967707adde..fe3b1aba222b 100644 --- a/products/metrics/backend/facade/contracts.py +++ b/products/metrics/backend/facade/contracts.py @@ -37,12 +37,6 @@ # Staff-only while it is a proof of concept. METRICS_ERROR_OVERLAYS_FEATURE_FLAG = "metrics-error-overlays" -# Fundamentals recomputes a chart point from its raw samples so the viewer's own -# reductions can be checked. That makes it a tool for the people who build the -# viewer, not a feature for the teams on the alpha, so it needs a gate of its own -# on top of METRICS_FEATURE_FLAG. -METRICS_FUNDAMENTALS_FEATURE_FLAG = "metrics-fundamentals" - @dataclass(frozen=True, slots=True) class MetricFilter: @@ -370,61 +364,3 @@ class MetricsOverview: series: int lookback_seconds: int services: tuple[MetricsServiceOverview, ...] - - -@dataclass(frozen=True, slots=True) -class MetricSampleView: - """One raw reading, as it sits in storage before any reduction.""" - - time: str - value: float - - -@dataclass(frozen=True, slots=True) -class MetricSeriesBreakdown: - """One physical series inside a bucket, and the value it contributed. - - `samples` is trimmed for display; `sample_count` always reports how many - the series really sent, so a trimmed list can't be mistaken for a quiet one. - """ - - service_name: str - labels: dict[str, str] - resource_labels: dict[str, str] - samples: tuple[MetricSampleView, ...] - sample_count: int - samples_truncated: bool - # None when the aggregation has no per-series step, as percentiles do not: - # they read the pooled readings, so no single number is this series' - # contribution. - value: float | None - - -@dataclass(frozen=True, slots=True) -class MetricBucketDecomposition: - """One chart point taken apart into the series and samples behind it. - - `reference_value` is recomputed from the raw samples independently of the - query builders; `actual_value` is what the product would plot. `agrees` - compares them, and is the part worth reading first — a mismatch means one - of the two reductions is wrong, and the breakdown shows where they parted. - """ - - metric_name: str - metric_type: str - temporality: str - aggregation: str - bucket_start: str - interval: str - temporal_reducer: str - spatial_reducer: str - series: tuple[MetricSeriesBreakdown, ...] - series_count: int - sample_count: int - series_truncated: bool - rows_truncated: bool - reference_value: float | None - actual_value: float | None - # None when the raw read was truncated: the reference then covers only part - # of the bucket, so comparing it to the chart proves nothing either way. - agrees: bool | None diff --git a/products/metrics/backend/fundamentals.py b/products/metrics/backend/fundamentals.py deleted file mode 100644 index 5fa5753ce4b5..000000000000 --- a/products/metrics/backend/fundamentals.py +++ /dev/null @@ -1,273 +0,0 @@ -"""The reduction rules every metric aggregation has to follow, as data. - -A bucket is not a bag of numbers. It holds a set of *series*, and each series -holds a set of *samples*. Collapsing it to one number is therefore two ordered -steps, never one: - - value(bucket) = spatial( over each series: temporal(its samples) ) - -`plan_reduction` picks both steps from the metric's type and temporality, which -is the part that is easy to get wrong by hand: a gauge sample is a re-reading -(take the last), a cumulative counter sample is an odometer (diff it), and a -delta counter sample is itself an increment (add them up). Applying one reducer -to all three silently returns a number that tracks the scrape rate instead of -the data. - -The reducers here are deliberately pure and independent of the HogQL builders in -`metric_query_runner`, so they can serve as the reference a query result is -checked against rather than a second copy of the same assumptions. -""" - -from __future__ import annotations - -import datetime as dt -from collections.abc import Mapping, Sequence -from enum import StrEnum -from typing import TypeVar - -from posthog.dataclasses import frozen - -# What `p95` means when a caller doesn't spell the percentile out. -_DEFAULT_QUANTILE = 0.95 - -# The reducers never read a series key; it only has to identify the series. -K = TypeVar("K") - - -@frozen -class Sample: - """One raw reading of one series.""" - - timestamp: dt.datetime - value: float - - -class TemporalReducer(StrEnum): - """How one series' samples collapse to that series' value for the bucket.""" - - # No temporal step: every raw sample flows into the spatial reducer. This is - # the shape of the bug this module exists to catch, kept nameable so a - # decomposition can report it rather than only failing a check. - NONE = "none" - # Gauges under an instant aggregation: the bucket's value is the most - # recent reading, matching PromQL's instant vector. - LAST = "last" - # Gauges under an average: the readings inside the bucket are all real - # observations, so the series' value for the bucket is their mean. - AVG_OVER_TIME = "avg_over_time" - # Percentiles: there is no per-series step at all. A percentile describes a - # distribution, and collapsing each series first would compute a percentile - # of summaries, which is not a percentile of anything. Samples are deduped - # by timestamp and pooled instead. - POOLED_SAMPLES = "pooled_samples" - # Delta counters: each sample is an increment already. - SUM_OVER_TIME = "sum_over_time" - # Cumulative counters: diff consecutive readings, treating a drop as a restart. - INCREASE = "increase" - - -class SpatialReducer(StrEnum): - """How one value per series collapses to the bucket's number.""" - - SUM = "sum" - AVG = "avg" - MIN = "min" - MAX = "max" - QUANTILE = "quantile" - COUNT_SERIES = "count_series" - - -@frozen -class ReductionPlan: - temporal: TemporalReducer - spatial: SpatialReducer - quantile: float | None = None - # `rate` is an increase per second, so its bucket total is divided by the - # time that total accumulated over. Every other aggregation plots the total. - divisor: float = 1.0 - - -_SPATIAL_BY_AGGREGATION: dict[str, SpatialReducer] = { - "sum": SpatialReducer.SUM, - "avg": SpatialReducer.AVG, - "min": SpatialReducer.MIN, - "max": SpatialReducer.MAX, - "count": SpatialReducer.COUNT_SERIES, - "p95": SpatialReducer.QUANTILE, - "quantile": SpatialReducer.QUANTILE, - "rate": SpatialReducer.SUM, - "increase": SpatialReducer.SUM, -} - -_COUNTER_FUNCTIONS = frozenset({"rate", "increase"}) - - -def _is_delta(temporality: str) -> bool: - return temporality == "delta" - - -def _rate_divisor(aggregation: str, interval_seconds: float | None) -> float: - """How long the bucket's total accumulated over, for the aggregations that - plot a per-second figure rather than the total itself. - - Refusing to default the interval keeps a plan built without one from - quietly reporting an increase where a rate was asked for — off by the - bucket length, which is the whole difference between the two. - """ - if aggregation != "rate": - return 1.0 - if interval_seconds is None or interval_seconds <= 0: - raise ValueError("rate is a per-second figure, so it needs a positive interval_seconds") - return float(interval_seconds) - - -def plan_reduction( - *, - aggregation: str, - metric_type: str, - temporality: str = "", - interval_seconds: float | None = None, -) -> ReductionPlan: - """Pick the two reduction steps for one aggregation on one kind of metric. - - `temporality` is the OTel `aggregation_temporality` column; gauges leave it - empty. It matters even for the instant aggregations, because a delta sample - is an increment rather than a reading. - - `interval_seconds` is the bucket's width, which only `rate` needs. - """ - try: - spatial = _SPATIAL_BY_AGGREGATION[aggregation] - except KeyError: - raise ValueError(f"Unsupported aggregation: {aggregation!r}") - - quantile = _DEFAULT_QUANTILE if spatial == SpatialReducer.QUANTILE else None - divisor = _rate_divisor(aggregation, interval_seconds) - - if _is_delta(temporality): - # Delta samples are increments whatever the caller asked for, so summing - # them over the bucket is the only reduction that keeps the total whole. - return ReductionPlan( - temporal=TemporalReducer.SUM_OVER_TIME, spatial=spatial, quantile=quantile, divisor=divisor - ) - if aggregation in _COUNTER_FUNCTIONS: - return ReductionPlan(temporal=TemporalReducer.INCREASE, spatial=spatial, quantile=quantile, divisor=divisor) - if spatial == SpatialReducer.QUANTILE: - return ReductionPlan( - temporal=TemporalReducer.POOLED_SAMPLES, spatial=spatial, quantile=quantile, divisor=divisor - ) - if spatial == SpatialReducer.AVG: - return ReductionPlan( - temporal=TemporalReducer.AVG_OVER_TIME, spatial=spatial, quantile=quantile, divisor=divisor - ) - return ReductionPlan(temporal=TemporalReducer.LAST, spatial=spatial, quantile=quantile, divisor=divisor) - - -def _deduped_in_time_order(samples: Sequence[Sample]) -> list[Sample]: - """One reading per timestamp, oldest first. - - A series re-delivered by the collector arrives as two rows sharing a - timestamp. That is one observation, so anything that adds samples together - has to collapse it first or the total moves with delivery luck. - """ - by_timestamp: dict[dt.datetime, Sample] = {} - for sample in sorted(samples, key=lambda s: s.timestamp): - by_timestamp.setdefault(sample.timestamp, sample) - return list(by_timestamp.values()) - - -def reduce_temporal(samples: Sequence[Sample], reducer: TemporalReducer) -> float | None: - """Collapse one series' samples to that series' value for the bucket. - - Returns None when the value is unknowable: a lone cumulative reading has - no predecessor to diff against, and 0 would read as a flat counter. - """ - if reducer in (TemporalReducer.NONE, TemporalReducer.POOLED_SAMPLES): - raise ValueError(f"{reducer!r} has no single per-series value; apply it through a plan") - ordered = _deduped_in_time_order(samples) - if reducer == TemporalReducer.INCREASE: - # A reading below its predecessor means the counter restarted, and the - # post-restart reading is itself the increase. - if len(ordered) < 2: - return None - total = 0.0 - for previous, current in zip(ordered, ordered[1:]): - total += current.value - previous.value if current.value >= previous.value else current.value - return total - if not ordered: - return 0.0 - - if reducer == TemporalReducer.LAST: - return ordered[-1].value - if reducer == TemporalReducer.SUM_OVER_TIME: - return sum(sample.value for sample in ordered) - if reducer == TemporalReducer.AVG_OVER_TIME: - return sum(sample.value for sample in ordered) / len(ordered) - raise ValueError(f"Unsupported temporal reducer: {reducer!r}") - - -def _quantile(sorted_values: Sequence[float], quantile: float) -> float: - """Linear interpolation between the closest ranks.""" - if len(sorted_values) == 1: - return sorted_values[0] - position = quantile * (len(sorted_values) - 1) - lower_index = int(position) - upper_index = min(lower_index + 1, len(sorted_values) - 1) - weight = position - lower_index - return sorted_values[lower_index] * (1 - weight) + sorted_values[upper_index] * weight - - -def reduce_spatial(values: Sequence[float], reducer: SpatialReducer, *, quantile: float | None = None) -> float | None: - """Combine one value per series into the bucket's number. - - Returns None for an empty bucket, which consumers render as a gap rather - than as a zero. - """ - # An empty bucket has no value at all, including no series count. Returning - # 0 here would make every gap look like a real zero. - if not values: - return None - if reducer == SpatialReducer.COUNT_SERIES: - return float(len(values)) - - if reducer == SpatialReducer.SUM: - return sum(values) - if reducer == SpatialReducer.AVG: - return sum(values) / len(values) - if reducer == SpatialReducer.MIN: - return min(values) - if reducer == SpatialReducer.MAX: - return max(values) - if reducer == SpatialReducer.QUANTILE: - return _quantile(sorted(values), quantile if quantile is not None else _DEFAULT_QUANTILE) - raise ValueError(f"Unsupported spatial reducer: {reducer!r}") - - -def apply_plan(series_samples: Mapping[K, Sequence[Sample]], plan: ReductionPlan) -> float | None: - """Run both reduction steps over a bucket's series and return its number.""" - if plan.temporal == TemporalReducer.NONE: - per_series_values = [sample.value for samples in series_samples.values() for sample in samples] - elif plan.temporal == TemporalReducer.POOLED_SAMPLES: - per_series_values = [ - sample.value for samples in series_samples.values() for sample in _deduped_in_time_order(samples) - ] - else: - # An unknowable series value contributes nothing rather than a fake 0, - # and a bucket holding only unknowns has no value at all. - reduced = (reduce_temporal(samples, plan.temporal) for samples in series_samples.values() if samples) - per_series_values = [value for value in reduced if value is not None] - value = reduce_spatial(per_series_values, plan.spatial, quantile=plan.quantile) - # An empty bucket has no number, and normalizing None would invent one. - return value if value is None else value / plan.divisor - - -def is_duplicate_invariant(series_samples: Mapping[K, Sequence[Sample]], plan: ReductionPlan) -> bool: - """Whether re-delivering every sample leaves the bucket's number unchanged. - - Duplicating a scrape is the cheapest way to ask whether a reduction counts - series or counts rows, and it needs no reference implementation to compare - against — a correct plan simply returns the same number twice. - """ - baseline = apply_plan(series_samples, plan) - doubled = {key: [*samples, *samples] for key, samples in series_samples.items()} - return apply_plan(doubled, plan) == baseline diff --git a/products/metrics/backend/metrics_overview_query_runner.py b/products/metrics/backend/metrics_overview_query_runner.py index 90289bb0be84..c64f82909769 100644 --- a/products/metrics/backend/metrics_overview_query_runner.py +++ b/products/metrics/backend/metrics_overview_query_runner.py @@ -1,4 +1,4 @@ -"""No FINAL: `uniqExact` and `max(last_seen)` give the same result on unmerged duplicate rows.""" +"""No FINAL: the distinct counts and `max(last_seen)` give the same result on unmerged duplicate rows.""" import datetime as dt import contextvars @@ -7,7 +7,7 @@ from opentelemetry import trace from opentelemetry.trace import Span -from posthog.schema import HogQLQueryResponse +from posthog.schema import HogQLQueryModifiers, HogQLQueryResponse from posthog.hogql import ast from posthog.hogql.constants import HogQLGlobalSettings @@ -112,18 +112,20 @@ def _run_metric_names_count(self) -> int: def _run_services(self) -> _ServicesRollup: with tracer.start_as_current_span("metrics.overview.services") as span: span.set_attribute("team_id", self.team.pk) - # Each series has one service, so the sum of the service counts is exact. + # The services_by_hour projection answers this query only while it filters on time_bucket, uses uniq, + # and aggregates the bare last_seen column. The time zone conversion stays outside max() for that reason. + # Each series has one service, so the sum of the service counts counts each series once. query = parse_select( """ SELECT service_name, uniqExact(metric_name) AS metric_names, - uniqExact(series_fingerprint) AS series, - max(last_seen) AS last_seen_at, - sum(uniqExact(series_fingerprint)) OVER () AS total_series, - max(max(last_seen)) OVER () AS total_last_seen_at + uniq(series_fingerprint) AS series, + toTimeZone(max(last_seen), 'UTC') AS last_seen_at, + sum(uniq(series_fingerprint)) OVER () AS total_series, + toTimeZone(max(max(last_seen)) OVER (), 'UTC') AS total_last_seen_at FROM posthog.metric_series - WHERE last_seen > now() - {lookback} + WHERE time_bucket >= toStartOfHour(toTimeZone(now() - {lookback}, 'UTC')) GROUP BY service_name ORDER BY series DESC, service_name ASC LIMIT {limit} @@ -138,6 +140,7 @@ def _run_services(self) -> _ServicesRollup: team=self.team, workload=Workload.LOGS, settings=_QUERY_SETTINGS, + modifiers=HogQLQueryModifiers(convertToProjectTimezone=False), ) _set_query_timing_attributes(span, response) span.set_attribute("services.count", len(response.results)) diff --git a/products/metrics/backend/presentation/api.py b/products/metrics/backend/presentation/api.py index aacdba9ab09c..e457cd51783c 100644 --- a/products/metrics/backend/presentation/api.py +++ b/products/metrics/backend/presentation/api.py @@ -28,7 +28,6 @@ from products.metrics.backend.facade.api import ( characterize_metric_anomaly, - explain_metric_bucket, get_metrics_overview, list_metric_attribute_keys, list_metric_attribute_values, @@ -44,7 +43,6 @@ MAX_SPARKLINE_BATCH_SIZE, METRICS_ERROR_OVERLAYS_FEATURE_FLAG, METRICS_FEATURE_FLAG, - METRICS_FUNDAMENTALS_FEATURE_FLAG, MetricFilter, MetricGroupBy, MetricQueryClause, @@ -734,130 +732,6 @@ class _MetricErrorSpikesResponseSerializer(serializers.Serializer): ) -class _MetricExplainBodySerializer(serializers.Serializer): - metricName = serializers.CharField( - max_length=255, - help_text="Exact metric name whose bucket should be taken apart.", - ) - metricType = serializers.ChoiceField( - choices=[t.value for t in MetricType], - required=False, - allow_null=True, - help_text="Constrain the bucket to one metric type. A name can exist as several types; without this, rows of every type sharing the name are decomposed together.", - ) - aggregation = serializers.ChoiceField( - choices=["sum", "avg", "count", "min", "max", "p95", "rate", "increase", "histogram_quantile"], - default="sum", - help_text="The aggregation whose result should be explained. 'histogram_quantile' is rejected: it reduces bucket-count arrays rather than scalar samples, so there is no per-series value to lay out.", - ) - quantile = serializers.FloatField( - required=False, - allow_null=True, - min_value=0.0, - max_value=1.0, - help_text="Quantile in (0, 1) applied across series. Defaults to 0.95 for the 'p95' aggregation.", - ) - filters = _MetricFilterSerializer( - many=True, - required=False, - default=list, - help_text="Label predicates ANDed together, matching the chart the point came from.", - ) - bucketStart = serializers.DateTimeField( - help_text="Start of the bucket to explain, as returned in a query result's 'time'. ISO 8601.", - ) - interval = serializers.ChoiceField( - choices=MetricQueryInterval.choices, - help_text="Bucket size the point was plotted at. Must match the query that produced it, or the decomposition explains a different span.", - ) - - def validate(self, attrs: dict) -> dict: - if attrs.get("aggregation") == "histogram_quantile": - raise serializers.ValidationError( - "'histogram_quantile' cannot be decomposed: it reduces bucket-count arrays rather than scalar samples." - ) - return attrs - - -class _MetricExplainRequestSerializer(serializers.Serializer): - query = _MetricExplainBodySerializer(help_text="The chart point to take apart.") - - -class _MetricSampleViewSerializer(serializers.Serializer): - time = serializers.CharField(help_text="Sample timestamp, ISO 8601.") - value = serializers.FloatField(help_text="Raw stored reading, before any reduction.") - - -class _MetricSeriesBreakdownSerializer(serializers.Serializer): - service_name = serializers.CharField(help_text="Service that reported this series.") - labels = serializers.DictField( - child=serializers.CharField(), - help_text="Per-data-point attributes identifying the series.", - ) - resource_labels = serializers.DictField( - child=serializers.CharField(), - help_text="Resource attributes identifying the scrape target.", - ) - samples = _MetricSampleViewSerializer( - many=True, - help_text="The series' raw samples in this bucket, oldest first, trimmed for display.", - ) - sample_count = serializers.IntegerField( - help_text="How many samples the series actually sent, even when 'samples' was trimmed." - ) - samples_truncated = serializers.BooleanField(help_text="Whether 'samples' lists fewer samples than arrived.") - value = serializers.FloatField( - allow_null=True, - help_text="What this series contributed after the per-series reduction. Null for percentiles, which read the pooled readings and so have no single per-series contribution.", - ) - - -class _MetricBucketDecompositionSerializer(serializers.Serializer): - metric_name = serializers.CharField(help_text="Metric that was decomposed.") - metric_type = serializers.CharField(help_text="OTel metric type observed in the bucket.") - temporality = serializers.CharField( - allow_blank=True, - help_text="OTel aggregation temporality observed in the bucket ('cumulative', 'delta', or empty for gauges).", - ) - aggregation = serializers.CharField(help_text="Aggregation that was explained.") - bucket_start = serializers.CharField(help_text="Start of the explained bucket, ISO 8601.") - interval = serializers.CharField(help_text="Bucket size the point was plotted at.") - temporal_reducer = serializers.ChoiceField( - choices=["none", "last", "avg_over_time", "sum_over_time", "increase", "pooled_samples"], - help_text="How each series' samples were collapsed to one value: 'last' for an instant gauge reading, 'avg_over_time' for an average, 'sum_over_time' for delta counters, 'increase' for cumulative counters, and 'pooled_samples' for percentiles, which skip the per-series step entirely.", - ) - spatial_reducer = serializers.ChoiceField( - choices=["sum", "avg", "min", "max", "quantile", "count_series"], - help_text="How the per-series values were combined into the bucket's number.", - ) - series = _MetricSeriesBreakdownSerializer( - many=True, - help_text="The series behind the point, largest contributors first, trimmed for display.", - ) - series_count = serializers.IntegerField(help_text="How many series reported in the bucket.") - sample_count = serializers.IntegerField(help_text="How many raw samples the bucket held across all series.") - series_truncated = serializers.BooleanField(help_text="Whether 'series' lists fewer series than reported.") - rows_truncated = serializers.BooleanField( - help_text="Whether the bucket held more raw rows than the decomposition reads. Totals are computed only over the rows that were read." - ) - reference_value = serializers.FloatField( - allow_null=True, - help_text="The bucket's value recomputed from the raw samples, independently of the query builders. Null when no series reported.", - ) - actual_value = serializers.FloatField( - allow_null=True, - help_text="The value the product would plot for this point. Null when the query returned no row.", - ) - agrees = serializers.BooleanField( - allow_null=True, - help_text="Whether the two values match. False means one of the reductions is wrong, and the series breakdown shows where they parted. Null when the raw read was truncated, so the two are not comparable.", - ) - - -class _MetricExplainResponseSerializer(serializers.Serializer): - decomposition = _MetricBucketDecompositionSerializer(help_text="The bucket taken apart.") - - @extend_schema(tags=["metrics"]) class MetricsViewSet(TeamAndOrgViewSetMixin, viewsets.ViewSet): scope_object = "metrics" @@ -1175,72 +1049,6 @@ def error_spikes(self, request: Request, *args, **kwargs) -> Response: return Response({"results": [asdict(s) for s in spikes]}, status=status.HTTP_200_OK) - @extend_schema(request=_MetricExplainRequestSerializer, responses={200: _MetricExplainResponseSerializer}) - @action( - detail=False, - methods=["POST"], - required_scopes=["metrics:read"], - throttle_classes=[ClickHouseBurstRateThrottle, ClickHouseSustainedRateThrottle], - ) - def explain(self, request: Request, *args, **kwargs) -> Response: - """Take one chart point apart into the series and samples behind it, - and recompute it independently so the plotted number can be checked - rather than trusted.""" - # The class-level gate admits every team on the metrics alpha, which is wider - # than this action should be. Fundamentals is a correctness tool for the people - # who build the viewer, so it carries its own flag. Without this check the tab - # is hidden in the UI but the data behind it stays one POST away. - if not posthog_feature_flag_enabled( - METRICS_FUNDAMENTALS_FEATURE_FLAG, - str(cast(User, request.user).distinct_id), - organization_id=self.team.organization_id, - team_id=self.team.pk, - ): - raise PermissionDenied( - f"This action requires feature flag {METRICS_FUNDAMENTALS_FEATURE_FLAG!r} to be enabled for your organization." - ) - - tag_queries(product=Product.METRICS, feature=Feature.QUERY) - - body = _MetricExplainRequestSerializer(data=request.data) - body.is_valid(raise_exception=True) - query_data = body.validated_data["query"] - - filters = tuple( - MetricFilter(key=f["key"], op=FilterOp(f["op"]), value=f["value"], scope=AttributeScope(f["scope"])) - for f in query_data.get("filters") or [] - ) - try: - decomposition = explain_metric_bucket( - team=self.team, - metric_name=query_data["metricName"], - aggregation=query_data["aggregation"], - bucket_start=query_data["bucketStart"], - interval=query_data["interval"], - filters=filters, - metric_type=MetricType(query_data["metricType"]) if query_data.get("metricType") else None, - quantile=query_data.get("quantile"), - ) - except ValueError as exc: - raise ParseError(str(exc)) - - report_user_action( - request.user, - "metrics bucket explained", - { - "aggregation": decomposition.aggregation, - "metric_type": decomposition.metric_type, - "series_count": decomposition.series_count, - "agrees": decomposition.agrees, - }, - team=self.team, - request=request, - ) - - return Response( - _MetricExplainResponseSerializer({"decomposition": decomposition}).data, status=status.HTTP_200_OK - ) - @extend_schema(request=_MetricAnomalyRequestSerializer, responses={200: _MetricAnomalyReportSerializer}) @action( detail=False, diff --git a/products/metrics/backend/tests/conftest.py b/products/metrics/backend/tests/conftest.py index 477d97eedeb2..d40800c3c666 100644 --- a/products/metrics/backend/tests/conftest.py +++ b/products/metrics/backend/tests/conftest.py @@ -5,18 +5,15 @@ from posthog.api.snuffle_proxy import SNUFFLE_API_FEATURE_FLAG -from products.metrics.backend.facade.contracts import METRICS_FUNDAMENTALS_FEATURE_FLAG - @pytest.fixture(autouse=True) def enable_metrics_feature_flag() -> Iterator[None]: # Enable the flags needed by the metrics endpoint tests. # MetricsViewSet needs `metrics`. - # The explain action needs `METRICS_FUNDAMENTALS_FEATURE_FLAG`. # The Prometheus proxy needs `SNUFFLE_API_FEATURE_FLAG`. # Gate tests set their flag to False. def _feature_enabled(flag_key: str, *args: object, **kwargs: object) -> bool: - return flag_key in ("metrics", METRICS_FUNDAMENTALS_FEATURE_FLAG, SNUFFLE_API_FEATURE_FLAG) + return flag_key in ("metrics", SNUFFLE_API_FEATURE_FLAG) with patch("posthoganalytics.feature_enabled", side_effect=_feature_enabled): yield diff --git a/products/metrics/backend/tests/test_api.py b/products/metrics/backend/tests/test_api.py index 7b8b37f9d273..48b23e6e1fcd 100644 --- a/products/metrics/backend/tests/test_api.py +++ b/products/metrics/backend/tests/test_api.py @@ -23,7 +23,6 @@ ) from products.access_control.backend.models.access_control import AccessControl from products.error_tracking.backend.facade.testing import create_issue, create_spike_event -from products.metrics.backend.facade.contracts import METRICS_FUNDAMENTALS_FEATURE_FLAG def test_metrics_app_is_installed(): @@ -123,23 +122,6 @@ def test_metrics_flag_gates_the_api(self, _name: str, flag_enabled: bool, expect assert response.status_code == expected_status - @parameterized.expand( - [ - ("enabled", True, status.HTTP_400_BAD_REQUEST), - ("disabled", False, status.HTTP_403_FORBIDDEN), - ] - ) - def test_fundamentals_flag_gates_the_explain_action( - self, _name: str, fundamentals_enabled: bool, expected_status: int - ) -> None: - def feature_enabled(flag: str, *args: object, **kwargs: object) -> bool: - return fundamentals_enabled if flag == METRICS_FUNDAMENTALS_FEATURE_FLAG else True - - with patch("posthoganalytics.feature_enabled", side_effect=feature_enabled): - response = self.client.post(f"/api/projects/{self.team.id}/metrics/explain/", {}, format="json") - - assert response.status_code == expected_status - @pytest.mark.ee class TestMetricsAccessControl(APIBaseTest): @@ -196,7 +178,6 @@ def test_access_level_controls_metrics_queries( ("query", "POST", {}), ("samples", "POST", {}), ("error_spikes", "GET", {}), - ("explain", "POST", {}), ("characterize", "POST", {}), ] ) diff --git a/products/metrics/backend/tests/test_diagnostics.py b/products/metrics/backend/tests/test_diagnostics.py deleted file mode 100644 index 191b16bf2466..000000000000 --- a/products/metrics/backend/tests/test_diagnostics.py +++ /dev/null @@ -1,361 +0,0 @@ -import datetime as dt - -import pytest -from posthog.test.base import APIBaseTest, ClickhouseTestMixin -from unittest.mock import patch - -from products.metrics.backend import diagnostics -from products.metrics.backend.diagnostics import decompose_bucket -from products.metrics.backend.fundamentals import SpatialReducer, TemporalReducer -from products.metrics.backend.tests._seeder import seed_metric, truncate_metrics_tables - -BUCKET = dt.datetime(2026, 9, 15, 0, 0, 0, tzinfo=dt.UTC) - - -class TestBucketDecomposition(ClickhouseTestMixin, APIBaseTest): - """The decomposition recomputes a chart point from raw samples in Python and - reports it next to what the HogQL runner returned. The two disagreeing is the - signal — it means one of the reductions is wrong, and the breakdown shows which.""" - - def setUp(self): - super().setUp() - truncate_metrics_tables() - - def _seed_gauge_pair(self) -> None: - # Two pods reporting the same gauge, three scrapes each inside one bucket. - seed_metric( - team_id=self.team.pk, - metric_name="cache_size", - metric_type="gauge", - aggregation_temporality="", - labels={"pod": "a"}, - points=[ - (BUCKET, 5.0), - (BUCKET + dt.timedelta(seconds=60), 8.0), - (BUCKET + dt.timedelta(seconds=120), 11.0), - ], - ) - seed_metric( - team_id=self.team.pk, - metric_name="cache_size", - metric_type="gauge", - aggregation_temporality="", - labels={"pod": "b"}, - points=[ - (BUCKET, 20.0), - (BUCKET + dt.timedelta(seconds=60), 21.0), - (BUCKET + dt.timedelta(seconds=120), 22.0), - ], - ) - - def test_gauge_breakdown_reduces_each_series_to_its_latest_reading(self) -> None: - self._seed_gauge_pair() - - decomposition = decompose_bucket( - team=self.team, - metric_name="cache_size", - aggregation="sum", - bucket_start=BUCKET, - interval="minute_5", - ) - - assert decomposition.temporal_reducer == TemporalReducer.LAST - assert decomposition.spatial_reducer == SpatialReducer.SUM - assert decomposition.series_count == 2 - assert decomposition.sample_count == 6 - # 11 and 22 are the latest readings; the four earlier samples are re-readings. - contributions = [series.value for series in decomposition.series] - assert sorted(value for value in contributions if value is not None) == [11.0, 22.0] - assert None not in contributions - assert decomposition.reference_value == 33.0 - - def test_reports_disagreement_between_the_runner_and_the_reference(self) -> None: - """Whether these agree depends on the runner, which is the point: the check - holds a reduction the runner does not share, so a regression on either side - shows up as a disagreement rather than as a plausible-looking number.""" - self._seed_gauge_pair() - - decomposition = decompose_bucket( - team=self.team, - metric_name="cache_size", - aggregation="sum", - bucket_start=BUCKET, - interval="minute_5", - ) - - assert decomposition.actual_value is not None - assert decomposition.agrees == (decomposition.actual_value == decomposition.reference_value) - - def test_delta_counter_totals_every_increment_rather_than_the_last_one(self) -> None: - # Each delta sample IS an increment, so keeping only the newest would drop - # the rest of the bucket's traffic. - seed_metric( - team_id=self.team.pk, - metric_name="requests_total", - metric_type="sum", - aggregation_temporality="delta", - is_monotonic=True, - labels={"pod": "a"}, - points=[(BUCKET, 3.0), (BUCKET + dt.timedelta(seconds=60), 4.0), (BUCKET + dt.timedelta(seconds=120), 5.0)], - ) - - decomposition = decompose_bucket( - team=self.team, - metric_name="requests_total", - aggregation="sum", - bucket_start=BUCKET, - interval="minute_5", - ) - - assert decomposition.temporality == "delta" - assert decomposition.temporal_reducer == TemporalReducer.SUM_OVER_TIME - assert decomposition.reference_value == 12.0 - - def test_cumulative_counter_increase_diffs_within_the_series(self) -> None: - seed_metric( - team_id=self.team.pk, - metric_name="bytes_total", - metric_type="sum", - aggregation_temporality="cumulative", - is_monotonic=True, - labels={"pod": "a"}, - points=[ - (BUCKET, 100.0), - (BUCKET + dt.timedelta(seconds=60), 120.0), - (BUCKET + dt.timedelta(seconds=120), 5.0), - ], - ) - - decomposition = decompose_bucket( - team=self.team, - metric_name="bytes_total", - aggregation="increase", - bucket_start=BUCKET, - interval="minute_5", - ) - - assert decomposition.temporal_reducer == TemporalReducer.INCREASE - # +20, then a restart whose post-reset reading is itself the increase. - assert decomposition.reference_value == 25.0 - - def test_lone_cumulative_sample_has_no_increase_on_either_side(self) -> None: - seed_metric( - team_id=self.team.pk, - metric_name="bytes_total", - metric_type="sum", - aggregation_temporality="cumulative", - is_monotonic=True, - points=[(BUCKET, 100.0)], - ) - - decomposition = decompose_bucket( - team=self.team, - metric_name="bytes_total", - aggregation="increase", - bucket_start=BUCKET, - interval="minute_5", - ) - - # The sample's history is unknown, so both the reference and the chart - # return no value — a 0 on either side would fabricate a flat counter. - assert decomposition.reference_value is None - assert decomposition.actual_value is None - assert decomposition.agrees is True - - def test_empty_bucket_reports_no_series_rather_than_zero(self) -> None: - decomposition = decompose_bucket( - team=self.team, - metric_name="nothing_here", - aggregation="sum", - bucket_start=BUCKET, - interval="minute_5", - ) - - assert decomposition.series_count == 0 - assert decomposition.reference_value is None - - def test_truncation_is_reported_rather_than_silently_dropping_series(self) -> None: - for index in range(4): - seed_metric( - team_id=self.team.pk, - metric_name="wide_metric", - metric_type="gauge", - aggregation_temporality="", - labels={"pod": f"pod-{index}"}, - points=[(BUCKET, float(index))], - ) - - decomposition = decompose_bucket( - team=self.team, - metric_name="wide_metric", - aggregation="sum", - bucket_start=BUCKET, - interval="minute_5", - max_series=2, - ) - - # The totals stay whole; only the per-series listing is shortened. - assert decomposition.series_count == 4 - assert len(decomposition.series) == 2 - assert decomposition.series_truncated is True - assert decomposition.reference_value == 6.0 - - -class TestExplainEndpoint(ClickhouseTestMixin, APIBaseTest): - def setUp(self): - super().setUp() - truncate_metrics_tables() - - def test_explain_returns_the_series_behind_a_point(self) -> None: - seed_metric( - team_id=self.team.pk, - metric_name="cache_size", - metric_type="gauge", - aggregation_temporality="", - labels={"pod": "a"}, - points=[(BUCKET, 5.0), (BUCKET + dt.timedelta(seconds=60), 11.0)], - ) - - response = self.client.post( - f"/api/projects/{self.team.id}/metrics/explain/", - { - "query": { - "metricName": "cache_size", - "aggregation": "sum", - "bucketStart": BUCKET.isoformat(), - "interval": "minute_5", - } - }, - format="json", - ) - - assert response.status_code == 200, response.json() - decomposition = response.json()["decomposition"] - assert decomposition["temporal_reducer"] == "last" - assert decomposition["series_count"] == 1 - assert decomposition["reference_value"] == 11.0 - assert [sample["value"] for sample in decomposition["series"][0]["samples"]] == [5.0, 11.0] - - def test_rejects_an_interval_the_chart_could_not_have_used(self) -> None: - response = self.client.post( - f"/api/projects/{self.team.id}/metrics/explain/", - { - "query": { - "metricName": "cache_size", - "aggregation": "sum", - "bucketStart": BUCKET.isoformat(), - "interval": "fortnight", - } - }, - format="json", - ) - - assert response.status_code == 400 - - -class TestCounterBoundary(ClickhouseTestMixin, APIBaseTest): - def setUp(self): - super().setUp() - truncate_metrics_tables() - # The predecessor sample sits in the previous bucket; the chart's window - # function diffs across that edge, so the check has to as well. - seed_metric( - team_id=self.team.pk, - metric_name="bytes_total", - metric_type="sum", - aggregation_temporality="cumulative", - is_monotonic=True, - labels={"pod": "a"}, - points=[ - (BUCKET - dt.timedelta(seconds=60), 100.0), - (BUCKET + dt.timedelta(seconds=60), 120.0), - (BUCKET + dt.timedelta(seconds=120), 140.0), - ], - ) - - def test_increase_counts_the_rise_across_the_bucket_edge(self) -> None: - decomposition = decompose_bucket( - team=self.team, - metric_name="bytes_total", - aggregation="increase", - bucket_start=BUCKET, - interval="minute_5", - ) - - # 100 -> 120 -> 140: the bucket rose by 40, of which 20 crosses the - # edge. Read in isolation, both sides would drop that 20 and agree on - # a value the chart never plotted. - assert decomposition.reference_value == 40.0 - assert decomposition.actual_value == 40.0 - assert decomposition.agrees is True - - def test_agrees_when_the_predecessor_sits_further_back_than_one_bucket(self) -> None: - # A minute chart of a series scraped every few minutes: the reference - # reduction and the runner have to reach back over the same window, or - # one of them finds a predecessor the other doesn't and the tab reports - # a disagreement the chart never had. - seed_metric( - team_id=self.team.pk, - metric_name="packets_total", - metric_type="sum", - aggregation_temporality="cumulative", - is_monotonic=True, - points=[ - (BUCKET - dt.timedelta(minutes=3), 100.0), - (BUCKET + dt.timedelta(seconds=30), 120.0), - ], - ) - - decomposition = decompose_bucket( - team=self.team, - metric_name="packets_total", - aggregation="increase", - bucket_start=BUCKET, - interval="minute", - ) - - assert decomposition.reference_value == 20.0 - assert decomposition.actual_value == 20.0 - assert decomposition.agrees is True - - def test_rate_normalizes_the_boundary_increase_too(self) -> None: - decomposition = decompose_bucket( - team=self.team, - metric_name="bytes_total", - aggregation="rate", - bucket_start=BUCKET, - interval="minute_5", - ) - - assert decomposition.reference_value == pytest.approx(40.0 / 300.0) - assert decomposition.agrees is True - - -class TestTruncatedBucket(ClickhouseTestMixin, APIBaseTest): - def setUp(self): - super().setUp() - truncate_metrics_tables() - - def test_truncated_read_reports_not_comparable_instead_of_a_verdict(self) -> None: - seed_metric( - team_id=self.team.pk, - metric_name="cache_size", - metric_type="gauge", - aggregation_temporality="", - labels={"pod": "a"}, - points=[(BUCKET + dt.timedelta(seconds=10 * i), float(i)) for i in range(6)], - ) - - with patch.object(diagnostics, "_MAX_ROWS_READ", 5): - decomposition = decompose_bucket( - team=self.team, - metric_name="cache_size", - aggregation="sum", - bucket_start=BUCKET, - interval="minute_5", - ) - - # The reference saw 5 of 6 rows while the runner saw all of them, so - # any verdict would be an artifact of the unequal inputs. - assert decomposition.rows_truncated is True - assert decomposition.agrees is None diff --git a/products/metrics/backend/tests/test_fundamentals.py b/products/metrics/backend/tests/test_fundamentals.py deleted file mode 100644 index 5a2bdc5603e9..000000000000 --- a/products/metrics/backend/tests/test_fundamentals.py +++ /dev/null @@ -1,206 +0,0 @@ -import datetime as dt - -import pytest - -from parameterized import parameterized - -from products.metrics.backend.fundamentals import ( - ReductionPlan, - Sample, - SpatialReducer, - TemporalReducer, - apply_plan, - is_duplicate_invariant, - plan_reduction, - reduce_spatial, - reduce_temporal, -) - -BUCKET = dt.datetime(2026, 1, 1, 0, 0, 0, tzinfo=dt.UTC) - - -def _samples(*values: float, step_seconds: int = 30) -> list[Sample]: - return [Sample(timestamp=BUCKET + dt.timedelta(seconds=i * step_seconds), value=v) for i, v in enumerate(values)] - - -class TestPlanReduction: - @parameterized.expand( - [ - # A gauge sample is a re-reading, so the bucket's current value is the last one. - ("gauge_sum", "sum", "gauge", "", TemporalReducer.LAST, SpatialReducer.SUM, 1.0), - # A gauge that moves inside the bucket has a meaningful mean, so the - # per-series step averages over time rather than keeping one reading. - ("gauge_avg", "avg", "gauge", "", TemporalReducer.AVG_OVER_TIME, SpatialReducer.AVG, 1.0), - ("gauge_count", "count", "gauge", "", TemporalReducer.LAST, SpatialReducer.COUNT_SERIES, 1.0), - # A percentile describes a distribution, so it needs the readings - # themselves rather than one summary number per series. - ("gauge_p95", "p95", "gauge", "", TemporalReducer.POOLED_SAMPLES, SpatialReducer.QUANTILE, 1.0), - # A cumulative counter carries an absolute odometer reading. - ("cumulative_sum", "sum", "sum", "cumulative", TemporalReducer.LAST, SpatialReducer.SUM, 1.0), - ( - "cumulative_increase", - "increase", - "sum", - "cumulative", - TemporalReducer.INCREASE, - SpatialReducer.SUM, - 1.0, - ), - # Same two reduction steps as `increase`; only the divisor separates them. - ("cumulative_rate", "rate", "sum", "cumulative", TemporalReducer.INCREASE, SpatialReducer.SUM, 300.0), - # A delta sample IS an increment, so the bucket total is their sum. Taking the - # last sample here keeps one increment and discards the rest. - ("delta_sum", "sum", "sum", "delta", TemporalReducer.SUM_OVER_TIME, SpatialReducer.SUM, 1.0), - ("delta_increase", "increase", "sum", "delta", TemporalReducer.SUM_OVER_TIME, SpatialReducer.SUM, 1.0), - ("delta_rate", "rate", "sum", "delta", TemporalReducer.SUM_OVER_TIME, SpatialReducer.SUM, 300.0), - ] - ) - def test_plan_maps_type_and_temporality( - self, - _name: str, - aggregation: str, - metric_type: str, - temporality: str, - expected_temporal: TemporalReducer, - expected_spatial: SpatialReducer, - expected_divisor: float, - ) -> None: - plan = plan_reduction( - aggregation=aggregation, metric_type=metric_type, temporality=temporality, interval_seconds=300 - ) - assert plan.temporal == expected_temporal - assert plan.spatial == expected_spatial - assert plan.divisor == expected_divisor - - -class TestTemporalReduction: - def test_last_uses_latest_timestamp_not_largest_value(self) -> None: - # A falling gauge: the peak is stale, the current reading is the tail. - assert reduce_temporal(_samples(100, 50, 10), TemporalReducer.LAST) == 10 - - def test_sum_over_time_dedupes_duplicate_timestamps(self) -> None: - # Two rows at one timestamp are one increment delivered twice, not two increments. - duplicated = [*_samples(3, 4), Sample(timestamp=BUCKET, value=3)] - assert reduce_temporal(duplicated, TemporalReducer.SUM_OVER_TIME) == 7 - - def test_increase_corrects_counter_reset(self) -> None: - # 100 -> 120 is +20; the drop to 5 is a restart, so 5 itself is the increase; 5 -> 25 is +20. - assert reduce_temporal(_samples(100, 120, 5, 25), TemporalReducer.INCREASE) == 45 - - def test_increase_of_a_lone_sample_is_unknown_not_zero(self) -> None: - # One reading has no predecessor to diff against; 0 would read as "flat". - assert reduce_temporal(_samples(100), TemporalReducer.INCREASE) is None - - def test_avg_over_time_keeps_the_whole_bucket_not_just_the_tail(self) -> None: - # A queue that spiked to 240 and settled at 8 did not average 8. - assert reduce_temporal(_samples(6, 240, 5, 210, 7, 8), TemporalReducer.AVG_OVER_TIME) == pytest.approx( - 79.33, abs=0.01 - ) - - -class TestSpatialReduction: - @parameterized.expand( - [ - ("sum", SpatialReducer.SUM, 30.0), - ("avg", SpatialReducer.AVG, 10.0), - ("min", SpatialReducer.MIN, 5.0), - ("max", SpatialReducer.MAX, 15.0), - ("count_series", SpatialReducer.COUNT_SERIES, 3.0), - ] - ) - def test_combines_one_value_per_series(self, _name: str, reducer: SpatialReducer, expected: float) -> None: - assert reduce_spatial([10.0, 5.0, 15.0], reducer) == expected - - def test_quantile_runs_over_series_values(self) -> None: - assert reduce_spatial([1.0, 2.0, 3.0, 4.0], SpatialReducer.QUANTILE, quantile=0.5) == pytest.approx(2.5) - - @parameterized.expand([("sum", SpatialReducer.SUM), ("count_series", SpatialReducer.COUNT_SERIES)]) - def test_empty_bucket_has_no_value(self, _name: str, reducer: SpatialReducer) -> None: - # The runner returns no row for an empty bucket, so a reference that - # returned 0 here would report every empty bucket as a disagreement. - assert reduce_spatial([], reducer) is None - - def test_unknown_series_values_drop_out_rather_than_zeroing_the_bucket(self) -> None: - plan = plan_reduction(aggregation="increase", metric_type="sum", temporality="cumulative") - # A lone-sample series adds nothing to the total, and a bucket holding - # only such series has no value at all — mirroring the runner, which - # drops the bucket instead of plotting 0. - assert apply_plan({"a": _samples(100), "b": _samples(10, 25)}, plan) == 15.0 - assert apply_plan({"a": _samples(100)}, plan) is None - - -class TestPooledQuantile: - def test_percentile_reads_the_samples_rather_than_one_value_per_series(self) -> None: - # One series that swings inside the bucket still has a tail. - plan = plan_reduction(aggregation="p95", metric_type="gauge") - spiky = {"a": _samples(6, 240, 5, 210, 7, 8)} - assert apply_plan(spiky, plan) == pytest.approx(232.5) - - def test_percentile_pools_across_series(self) -> None: - plan = plan_reduction(aggregation="p95", metric_type="gauge") - pooled = apply_plan({"a": _samples(1, 2), "b": _samples(3, 4)}, plan) - assert pooled == pytest.approx(_quantile_of([1.0, 2.0, 3.0, 4.0])) - - -class TestRateNormalization: - @parameterized.expand( - [ - # A counter climbing 60 over a five-minute bucket is 0.2/s. - ("rate_is_per_second", "rate", 0.2), - ("increase_is_the_total", "increase", 60.0), - ] - ) - def test_only_rate_divides_by_the_bucket_length(self, _name: str, aggregation: str, expected: float) -> None: - # The runner divides a rate by the bucket length, so a reference that - # skips it disagrees with every correct rate chart by that length. - plan = plan_reduction( - aggregation=aggregation, metric_type="sum", temporality="cumulative", interval_seconds=300 - ) - assert apply_plan({"a": _samples(10, 70)}, plan) == pytest.approx(expected) - - def test_rate_refuses_to_plan_without_a_bucket_length(self) -> None: - # Defaulting the interval would silently plot an increase as a rate. - with pytest.raises(ValueError): - plan_reduction(aggregation="rate", metric_type="sum", temporality="cumulative") - - -def _quantile_of(values: list[float]) -> float: - position = 0.95 * (len(values) - 1) - lower = int(position) - upper = min(lower + 1, len(values) - 1) - weight = position - lower - return values[lower] * (1 - weight) + values[upper] * weight - - -class TestDuplicateSampleInvariance: - """Re-delivering a scrape must not move the number. This is the property that - both known aggregation bugs violate, in opposite directions.""" - - @parameterized.expand( - [ - ("gauge_sum", "sum", "gauge", ""), - ("gauge_avg", "avg", "gauge", ""), - ("gauge_p95", "p95", "gauge", ""), - ("gauge_count", "count", "gauge", ""), - ("delta_sum", "sum", "sum", "delta"), - ("cumulative_increase", "increase", "sum", "cumulative"), - ] - ) - def test_planned_reduction_is_invariant( - self, _name: str, aggregation: str, metric_type: str, temporality: str - ) -> None: - plan = plan_reduction(aggregation=aggregation, metric_type=metric_type, temporality=temporality) - series = {"a": _samples(5, 7, 9), "b": _samples(2, 4)} - assert is_duplicate_invariant(series, plan) is True - - @parameterized.expand( - [ - ("sum", SpatialReducer.SUM), - ("count_series", SpatialReducer.COUNT_SERIES), - ] - ) - def test_detects_a_plan_with_no_temporal_step(self, _name: str, spatial: SpatialReducer) -> None: - # NONE routes every raw sample to the spatial reducer, so the answer tracks the - # scrape rate rather than the data. - plan = ReductionPlan(temporal=TemporalReducer.NONE, spatial=spatial) - assert is_duplicate_invariant({"a": _samples(5, 5, 5)}, plan) is False diff --git a/products/metrics/backend/tests/test_metrics_overview_query_runner.py b/products/metrics/backend/tests/test_metrics_overview_query_runner.py index f3d59195110c..7bfdfb8ac7da 100644 --- a/products/metrics/backend/tests/test_metrics_overview_query_runner.py +++ b/products/metrics/backend/tests/test_metrics_overview_query_runner.py @@ -1,4 +1,5 @@ import datetime as dt +from typing import Any from posthog.test.base import APIBaseTest, ClickhouseTestMixin from unittest.mock import patch @@ -8,6 +9,10 @@ from parameterized import parameterized from rest_framework import status +from posthog.schema import HogQLQueryResponse + +from posthog.hogql.query import execute_hogql_query + from products.metrics.backend import metrics_overview_query_runner from products.metrics.backend.metrics_overview_query_runner import MetricsOverviewQueryRunner from products.metrics.backend.tests._seeder import seed_metric, seed_metric_event, truncate_metrics_tables @@ -65,6 +70,20 @@ def test_rolls_up_services_within_the_window(self, _name: str, max_services: int self.assertEqual(api_row.series, 3) self.assertEqual(dt.datetime.fromisoformat(api_row.last_seen), anchor) + def test_services_query_reads_the_hourly_projection(self): + anchor = timezone.now().replace(microsecond=0) - dt.timedelta(minutes=5) + seed_metric(team_id=self.team.id, metric_name="http.duration", points=[(anchor, 1.0)], service_name="api") + + def force_projection(**kwargs: Any) -> HogQLQueryResponse: + if kwargs["query_type"] == "MetricsOverviewServicesQuery": + kwargs["settings"] = kwargs["settings"].model_copy(update={"force_optimize_projection": True}) + return execute_hogql_query(**kwargs) + + with patch.object(metrics_overview_query_runner, "execute_hogql_query", side_effect=force_projection): + overview = MetricsOverviewQueryRunner(team=self.team).run() + + self.assertEqual([(s.service_name, s.series) for s in overview.services], [("api", 1)]) + def test_quiet_project_keeps_overall_last_seen_but_lists_no_services(self): stale = timezone.now().replace(microsecond=0) - dt.timedelta(days=3) seed_metric(team_id=self.team.id, metric_name="http.duration", points=[(stale, 1.0)], service_name="api") diff --git a/products/metrics/frontend/MetricsScene.tsx b/products/metrics/frontend/MetricsScene.tsx index 970aee6728c6..0688dd06e09c 100644 --- a/products/metrics/frontend/MetricsScene.tsx +++ b/products/metrics/frontend/MetricsScene.tsx @@ -3,7 +3,6 @@ import posthog from 'posthog-js' import { LemonBanner, LemonButton, LemonTabs } from '@posthog/lemon-ui' -import { useFeatureFlag } from 'lib/hooks/useFeatureFlag' import { IconFeedback } from 'lib/lemon-ui/icons' import { getAccessControlDisabledReason } from 'lib/utils/accessControlUtils' import { sceneConfigurations } from 'scenes/scenes' @@ -18,15 +17,13 @@ import { AccessControlLevel, AccessControlResourceType } from '~/types' import { metricNamePickerLogic } from './components/metricNamePickerLogic' import { MetricsCatalog } from './components/MetricsCatalog' import { metricsCatalogLogic } from './components/metricsCatalogLogic' -import { MetricsFundamentals } from './components/MetricsFundamentals' -import { metricsFundamentalsLogic } from './components/metricsFundamentalsLogic' import { MetricsOverview } from './components/MetricsOverview' import { MetricsSqlEditor } from './components/MetricsSqlEditor' import { metricsUsageTrackingLogic } from './components/metricsUsageTrackingLogic' import { MetricsViewer } from './components/MetricsViewer' import { metricsEmptyState } from './emptyState/metricsEmptyState' import { metricsFeaturePreviewGate } from './featurePreviewGate' -import { DEFAULT_ACTIVE_TAB, MetricsSceneActiveTab, metricsSceneLogic } from './metricsSceneLogic' +import { MetricsSceneActiveTab, metricsSceneLogic } from './metricsSceneLogic' export const METRICS_LOGIC_KEY = 'metrics' @@ -37,7 +34,6 @@ const TABS: { key: MetricsSceneActiveTab; label: string; 'data-attr': string }[] { key: 'explore', label: 'Explore', 'data-attr': 'metrics-scene-tab-explore' }, { key: 'viewer', label: 'Viewer', 'data-attr': 'metrics-scene-tab-viewer' }, { key: 'sql', label: 'SQL', 'data-attr': 'metrics-scene-tab-sql' }, - { key: 'fundamentals', label: 'Fundamentals', 'data-attr': 'metrics-scene-tab-fundamentals' }, ] export const scene: SceneExport = { @@ -60,13 +56,6 @@ export function MetricsScene(): JSX.Element { const MetricsSceneContent = (): JSX.Element => { const { activeTab } = useValues(metricsSceneLogic) const { setActiveTab } = useActions(metricsSceneLogic) - // Fundamentals checks the viewer's own reductions against the raw samples, so it is - // built for the people who work on the viewer rather than for the teams on the alpha. - const fundamentalsEnabled = useFeatureFlag('METRICS_FUNDAMENTALS') - const visibleTabs = fundamentalsEnabled ? TABS : TABS.filter((tab) => tab.key !== 'fundamentals') - // A guessed ?activeTab=fundamentals must not render the tab either, so fall back to the - // default tab instead of leaving the scene with no visible content. - const effectiveTab = activeTab === 'fundamentals' && !fundamentalsEnabled ? DEFAULT_ACTIVE_TAB : activeTab const metricsViewerDisabledReason = getAccessControlDisabledReason( AccessControlResourceType.Metrics, AccessControlLevel.Viewer @@ -80,7 +69,6 @@ const MetricsSceneContent = (): JSX.Element => { explore: metricsViewerDisabledReason, viewer: metricsViewerDisabledReason, sql: metricsSqlDisabledReason, - fundamentals: metricsViewerDisabledReason, } // Scene-level so tab switches in both directions are captured; keeps the viewer // and samples logics (its connect targets) mounted across tab flips as a side effect. @@ -88,11 +76,9 @@ const MetricsSceneContent = (): JSX.Element => { // Prime the metric-name list here rather than inside MetricsViewer, so the fetch // races the has_metrics check instead of waiting on the setup prompt to resolve. useMountedLogic(metricNamePickerLogic) - // These two hold cross-tab state: a catalog card click preloads the viewer, and - // the viewer's explain button preloads fundamentals. Mounted here, a tab flip - // cannot unmount the logic and reset the handoff before the destination reads it. + // Holds cross-tab state: a catalog card click preloads the viewer. Mounted here, a tab + // flip cannot unmount the logic and reset the handoff before the destination reads it. useMountedLogic(metricsCatalogLogic) - useMountedLogic(metricsFundamentalsLogic) const onFeedbackClick = (): void => { posthog.displaySurvey(METRICS_FEEDBACK_SURVEY_ID) @@ -124,24 +110,23 @@ const MetricsSceneContent = (): JSX.Element => { Metrics is in alpha. Please share feedback on how to improve the product. - activeKey={effectiveTab} + activeKey={activeTab} onChange={(tab) => { if (!tabDisabledReasons[tab]) { setActiveTab(tab) } }} - tabs={visibleTabs.map((tab) => ({ + tabs={TABS.map((tab) => ({ ...tab, disabledReason: tabDisabledReasons[tab.key] ?? undefined, }))} sceneInset />
- {effectiveTab === 'overview' && } - {effectiveTab === 'explore' && } - {effectiveTab === 'viewer' && } - {effectiveTab === 'sql' && } - {effectiveTab === 'fundamentals' && } + {activeTab === 'overview' && } + {activeTab === 'explore' && } + {activeTab === 'viewer' && } + {activeTab === 'sql' && }
) diff --git a/products/metrics/frontend/components/MetricsClauseRow.tsx b/products/metrics/frontend/components/MetricsClauseRow.tsx index 54189c621f76..872c86ba42a5 100644 --- a/products/metrics/frontend/components/MetricsClauseRow.tsx +++ b/products/metrics/frontend/components/MetricsClauseRow.tsx @@ -1,7 +1,7 @@ import { useActions, useValues } from 'kea' import { useMemo } from 'react' -import { IconEllipsis, IconInfo } from '@posthog/icons' +import { IconEllipsis } from '@posthog/icons' import { LemonButton, LemonMenu, LemonSelect, LemonTag, Tooltip } from '@posthog/lemon-ui' import { TaxonomicFilterGroupType } from 'lib/components/TaxonomicFilter/types' @@ -9,10 +9,8 @@ import UniversalFilters from 'lib/components/UniversalFilters/UniversalFilters' import { FilterLogicalOperator, UniversalFiltersGroup } from '~/types' -import { metricsSceneLogic } from '../metricsSceneLogic' import { MetricNameFilter } from './MetricNameFilter' import { MetricsClauseFilterBar } from './MetricsClauseFilterBar' -import { metricsFundamentalsLogic } from './metricsFundamentalsLogic' import { MetricsGroupByButton } from './MetricsGroupByButton' import { MAX_CLAUSES, @@ -85,8 +83,6 @@ export function MetricsClauseRow({ const recommendedAggregation = clause.selectedMetricType ? RECOMMENDED_AGGREGATION_BY_TYPE[clause.selectedMetricType] : undefined - const { setActiveTab } = useActions(metricsSceneLogic) - const { explainMetric } = useActions(metricsFundamentalsLogic) return (
@@ -110,31 +106,12 @@ export function MetricsClauseRow({ )}
-
- - {clause.metricName && clause.selectedMetricType && ( - - } - onClick={() => { - explainMetric({ - metricName: clause.metricName, - aggregation: clause.aggregation, - }) - setActiveTab('fundamentals') - }} - data-attr="metrics-clause-explain" - /> - - )} -
+ {clause.selectedMetricType && recommendedAggregation && (clause.aggregation !== recommendedAggregation ? ( diff --git a/products/metrics/frontend/components/MetricsFundamentals.tsx b/products/metrics/frontend/components/MetricsFundamentals.tsx deleted file mode 100644 index c80326c6fe29..000000000000 --- a/products/metrics/frontend/components/MetricsFundamentals.tsx +++ /dev/null @@ -1,235 +0,0 @@ -import { useActions, useValues } from 'kea' - -import { LemonBanner, LemonButton, LemonCollapse, LemonInput, LemonSelect, LemonTag } from '@posthog/lemon-ui' - -import type { _MetricBucketDecompositionApi, _MetricSeriesBreakdownApi } from '../generated/api.schemas' -import { metricsFundamentalsLogic } from './metricsFundamentalsLogic' -import type { MetricAggregation } from './metricsViewerLogic' - -// Each rule states what should happen, then the formula, then a worked example -// small enough to check by eye. Someone who has never thought about metric -// aggregation should be able to read one card and know what to look for. -const RULES: { key: string; title: string; should: string; formula: string; example: string }[] = [ - { - key: 'two-axes', - title: 'A bucket holds series, not numbers', - should: 'Every value is reduced twice. First each series is collapsed on its own, then those results are combined across series. Doing it in one step counts a series once per scrape, so the answer follows how often you collect rather than what you measured.', - formula: 'value = combine(over each series: collapse(its samples))', - example: - 'Two pods, three scrapes each, one bucket. That is 6 samples but only 2 series. A total should add 2 numbers, not 6.', - }, - { - key: 'typing', - title: 'The metric type decides how a series collapses', - should: 'A gauge sample is a fresh reading, so the newest one wins. A cumulative counter is an odometer, so you subtract consecutive readings. A delta counter reports an increment each time, so you add them up. One rule applied to all three is wrong for two of them.', - formula: 'gauge: last · cumulative counter: sum of diffs · delta counter: sum', - example: - 'A counter reading 100, 120, 5, 25 rose by 45. The drop to 5 is a restart, so 5 is itself an increase.', - }, - { - key: 'scrape-rate', - title: 'Collecting more often must not change the answer', - should: 'Sending the same reading twice is one observation delivered twice. If a value moves when a scrape is duplicated, dropped, or a bucket is still filling, the reduction is counting rows instead of series.', - formula: 'value(samples) == value(samples delivered twice)', - example: - 'A gauge scraped 10 times a bucket that reads 10x too high is the classic case. The multiplier moves with the scrape rate, so the chart jumps for no real reason.', - }, - { - key: 'staleness', - title: 'A series that goes quiet is not a zero', - should: 'A series that reports in one bucket and not the next has not dropped to zero, it just has not been heard from. Totals over sparsely reported metrics swing on how many series happened to report, which reads as a real change but is not one.', - formula: 'absent series should carry forward or drop out, never count as 0', - example: - 'A total across 300 series where only 5 report each minute swings by millions between buckets purely on who reported. The check above cannot catch this one. It reads a single bucket, so a series that never reported is invisible to it. Compare neighbouring buckets by hand.', - }, - { - key: 'ordering', - title: 'Percentiles and rates do not survive being averaged', - should: 'A percentile of percentiles is not a percentile, and a rate of summed counters misreads restarts. Rates reduce inside each series first, then combine. Percentiles go the other way and read every reading in the bucket, because collapsing a series to one number throws away the tail the percentile is asking about. The tradeoff is that a series collected more often contributes more readings to that tail.', - formula: 'sum(rate(x)), never rate(sum(x))', - example: - 'Host A serves 1,000 requests at p95 of 1ms, host B serves 10 at 2,000ms. Averaging the two p95s gives about 1,000ms. The real combined p95 is about 1ms.', - }, -] - -// These describe the reduction the check itself applied, which is only also -// what the chart did when the two agree. The wording says so either way. -const TEMPORAL_REDUCER_COPY: Record = { - last: 'took each series latest reading', - avg_over_time: 'averaged each series readings over the bucket', - sum_over_time: 'added up each series increments', - increase: 'measured how much each series rose', - pooled_samples: 'used every reading rather than one value per series', - none: 'did not reduce per series, so every raw sample counted', -} - -const SPATIAL_REDUCER_COPY: Record = { - sum: 'added the series together', - avg: 'averaged across series', - min: 'took the smallest series', - max: 'took the largest series', - quantile: 'took a percentile across series', - count_series: 'counted how many series reported', -} - -const AGGREGATION_OPTIONS: { value: MetricAggregation; label: string }[] = [ - { value: 'sum', label: 'Sum' }, - { value: 'avg', label: 'Average' }, - { value: 'count', label: 'Count' }, - { value: 'min', label: 'Min' }, - { value: 'max', label: 'Max' }, - { value: 'p95', label: 'p95' }, - { value: 'rate', label: 'Rate (/s)' }, - { value: 'increase', label: 'Increase' }, -] - -// Float reductions land on values like 79.33333333333333, which are unreadable -// next to each other and imply a precision the comparison does not use. -const formatValue = (value: number | null): string => - value === null ? 'no value' : Number(value.toPrecision(10)).toLocaleString('en-US', { maximumFractionDigits: 4 }) - -const SeriesRow = ({ series }: { series: _MetricSeriesBreakdownApi }): JSX.Element => { - const labelText = - Object.entries(series.labels) - .map(([key, value]) => `${key}=${value}`) - .join(' ') || 'no labels' - - return ( -
-
- - {series.service_name} {labelText} - - - {series.value === null ? `${series.sample_count} readings` : formatValue(series.value)} - -
-
- {series.samples.map((sample) => sample.value).join(', ')} - {series.samples_truncated && ` and ${series.sample_count - series.samples.length} more`} -
-
- ) -} - -const Decomposition = ({ decomposition }: { decomposition: _MetricBucketDecompositionApi }): JSX.Element => { - const temporal = TEMPORAL_REDUCER_COPY[decomposition.temporal_reducer] ?? decomposition.temporal_reducer - const spatial = SPATIAL_REDUCER_COPY[decomposition.spatial_reducer] ?? decomposition.spatial_reducer - - return ( -
- - {decomposition.agrees === null ? ( - <> - This bucket holds more raw samples than the check reads, so the recomputed value covers only - part of the data and proves nothing about the chart's {formatValue(decomposition.actual_value)}. - Narrow with a filter and check again. - - ) : decomposition.agrees ? ( - <> - The chart shows {formatValue(decomposition.actual_value)} for this bucket, and recomputing it - from the raw samples gives the same number. - - ) : ( - <> - The chart shows {formatValue(decomposition.actual_value)} for this bucket, but recomputing it - from the raw samples gives {formatValue(decomposition.reference_value)}. One of the two is - wrong. The series below show what the data actually contains. - - )} - - -
- {decomposition.metric_type || 'unknown type'} - {decomposition.temporality && {decomposition.temporality}} - {decomposition.series_count} series - {decomposition.sample_count} samples -
- -

- To get {formatValue(decomposition.reference_value)}, the check {temporal}, then {spatial}. - {decomposition.aggregation === 'rate' && - ' The result is divided by the bucket length, so it is per second.'} - {decomposition.agrees === false && ' The chart reached its number a different way.'} -

- -
-

Series in this bucket

- {decomposition.series.map((series, index) => ( - - ))} - {decomposition.series_truncated && ( -

- Showing the {decomposition.series.length} largest of {decomposition.series_count} series. The - totals above cover all of them. -

- )} -
-
- ) -} - -export function MetricsFundamentals(): JSX.Element { - const { metricName, aggregation, checkResult, checkResultLoading } = useValues(metricsFundamentalsLogic) - const { setMetricName, setAggregation, runCheck } = useActions(metricsFundamentalsLogic) - - return ( -
-

- What a metrics chart shows depends on how its numbers were combined, and a wrong combination still looks - like a normal chart. This page explains the rules a correct chart follows, then checks a real point - against them. -

- -
-

Check a point

-

- Pick a metric and we take its most recent complete 5 minute bucket apart. The value is recomputed - from the raw samples and compared against what the chart would draw. -

-
- runCheck()} - placeholder="Metric name" - className="w-80" - /> - - value={aggregation} - onChange={setAggregation} - options={AGGREGATION_OPTIONS} - /> - runCheck()} - loading={checkResultLoading} - disabledReason={!metricName ? 'Enter a metric name' : undefined} - > - Check - -
-
- - {checkResult && !checkResultLoading && } - -
-

The rules

- ({ - key: rule.key, - header: rule.title, - content: ( -
-

{rule.should}

- {rule.formula} -

{rule.example}

-
- ), - }))} - /> -
-
- ) -} diff --git a/products/metrics/frontend/components/metricsFundamentalsLogic.test.ts b/products/metrics/frontend/components/metricsFundamentalsLogic.test.ts deleted file mode 100644 index af32645d3d39..000000000000 --- a/products/metrics/frontend/components/metricsFundamentalsLogic.test.ts +++ /dev/null @@ -1,113 +0,0 @@ -import { expectLogic } from 'kea-test-utils' - -import { initKeaTests } from '~/test/init' -import { AppContext } from '~/types' - -import { metricsExplainCreate, metricsQueryCreate } from 'products/metrics/frontend/generated/api' -import type { - _MetricBucketDecompositionApi, - _MetricQueryPointApi, - _MetricQueryResponseApi, -} from 'products/metrics/frontend/generated/api.schemas' - -import { metricsFundamentalsLogic } from './metricsFundamentalsLogic' - -jest.mock('products/metrics/frontend/generated/api', () => ({ - ...jest.requireActual('products/metrics/frontend/generated/api'), - metricsQueryCreate: jest.fn(), - metricsExplainCreate: jest.fn(), -})) - -const decomposition: _MetricBucketDecompositionApi = { - metric_name: 'cache_size', - metric_type: 'gauge', - temporality: '', - aggregation: 'sum', - bucket_start: '2026-01-01T00:05:00Z', - interval: '5m', - temporal_reducer: 'last', - spatial_reducer: 'sum', - series: [], - series_count: 0, - sample_count: 0, - series_truncated: false, - rows_truncated: false, - reference_value: 33, - actual_value: 33, - agrees: true, -} - -const queryResponse = (points: _MetricQueryPointApi[]): _MetricQueryResponseApi => ({ - results: [{ labels: {}, points }], -}) - -describe('metricsFundamentalsLogic', () => { - let logic: ReturnType - - beforeEach(() => { - window.POSTHOG_APP_CONTEXT = { current_project: { id: 997 } } as unknown as AppContext - initKeaTests() - logic = metricsFundamentalsLogic() - logic.mount() - jest.mocked(metricsExplainCreate).mockResolvedValue({ decomposition }) - }) - - afterEach(() => { - logic.unmount() - jest.clearAllMocks() - }) - - it('explains the newest bucket that has a value, not the newest bucket', async () => { - // The most recent bucket is usually still filling, so it comes back empty. - // Explaining it would decompose nothing while looking like it worked. - jest.mocked(metricsQueryCreate).mockResolvedValue( - queryResponse([ - { time: '2026-01-01T00:00:00Z', value: 1 }, - { time: '2026-01-01T00:05:00Z', value: 2 }, - { time: '2026-01-01T00:10:00Z', value: null }, - ]) - ) - - logic.actions.setMetricName('cache_size') - await expectLogic(logic, () => logic.actions.runCheck()).toFinishAllListeners() - - expect(jest.mocked(metricsExplainCreate).mock.calls[0][1].query.bucketStart).toEqual('2026-01-01T00:05:00Z') - }) - - it('drops the previous result when a new check starts', async () => { - // Otherwise the old metric's decomposition sits under the new metric's - // name while the new one loads, and reads as its answer. - jest.mocked(metricsQueryCreate).mockResolvedValue(queryResponse([{ time: '2026-01-01T00:00:00Z', value: 1 }])) - logic.actions.setMetricName('cache_size') - await expectLogic(logic, () => logic.actions.runCheck()).toFinishAllListeners() - expect(logic.values.checkResult).not.toBeNull() - - logic.actions.setMetricName('other_metric') - expectLogic(logic, () => logic.actions.runCheck()) - expect(logic.values.checkResult).toBeNull() - }) - - it('does not explain anything when the metric reported no values', async () => { - jest.mocked(metricsQueryCreate).mockResolvedValue(queryResponse([])) - - logic.actions.setMetricName('cache_size') - await expectLogic(logic, () => logic.actions.runCheck()).toFinishAllListeners() - - expect(metricsExplainCreate).not.toHaveBeenCalled() - expect(logic.values.checkResult).toBeNull() - }) - - it('explainMetric prefills the name and aggregation, then runs the check', async () => { - // The viewer's "explain this number" hands over a metric the user is already - // looking at, so the check must not make them retype it. - jest.mocked(metricsQueryCreate).mockResolvedValue(queryResponse([{ time: '2026-01-01T00:05:00Z', value: 2 }])) - - await expectLogic(logic, () => - logic.actions.explainMetric({ metricName: 'cache_size', aggregation: 'avg' }) - ).toFinishAllListeners() - - expect(logic.values.metricName).toBe('cache_size') - expect(logic.values.aggregation).toBe('avg') - expect(metricsExplainCreate).toHaveBeenCalled() - }) -}) diff --git a/products/metrics/frontend/components/metricsFundamentalsLogic.tsx b/products/metrics/frontend/components/metricsFundamentalsLogic.tsx deleted file mode 100644 index cfa3fba23f12..000000000000 --- a/products/metrics/frontend/components/metricsFundamentalsLogic.tsx +++ /dev/null @@ -1,174 +0,0 @@ -import { MakeLogicType, actions, kea, listeners, path, reducers, selectors } from 'kea' -import { loaders } from 'kea-loaders' - -import { lemonToast } from '@posthog/lemon-ui' - -import { dayjs } from 'lib/dayjs' - -import { metricsExplainCreate, metricsQueryCreate } from 'products/metrics/frontend/generated/api' -import type { _MetricBucketDecompositionApi } from 'products/metrics/frontend/generated/api.schemas' - -import type { MetricAggregation } from './metricsViewerLogic' - -// Bucket size the check runs at. Fixed rather than auto-picked, because a -// decomposition is only meaningful next to the interval it was computed for. -export const CHECK_INTERVAL = 'minute_5' - -// How far back to look for a bucket with data in it. The most recent bucket is -// usually still filling, so the check walks back to the last complete one. -const LOOKBACK_MINUTES = 60 - -export interface FundamentalsCheckResult { - decomposition: _MetricBucketDecompositionApi - checkedAt: string -} - -// Generated by kea-typegen. Update if you're an agent, ignore if you're human. -export interface metricsFundamentalsLogicValues { - aggregation: MetricAggregation - checkResult: FundamentalsCheckResult | null - checkResultLoading: boolean - metricName: string - projectId: string -} - -// Generated by kea-typegen. Update if you're an agent, ignore if you're human. -export interface metricsFundamentalsLogicActions { - explainMetric: (payload: { aggregation: MetricAggregation; metricName: string }) => { - aggregation: MetricAggregation - metricName: string - } - runCheck: () => { - value: true - } - runCheckFailure: ( - error: string, - errorObject?: any - ) => { - error: string - errorObject?: any - } - runCheckSuccess: ( - checkResult: { - checkedAt: string - decomposition: _MetricBucketDecompositionApi - } | null, - payload?: { - value: true - } - ) => { - checkResult: { - checkedAt: string - decomposition: _MetricBucketDecompositionApi - } | null - payload?: { - value: true - } - } - setAggregation: (aggregation: MetricAggregation) => { - aggregation: MetricAggregation - } - setMetricName: (metricName: string) => { - metricName: string - } -} - -export type metricsFundamentalsLogicType = MakeLogicType< - metricsFundamentalsLogicValues, - metricsFundamentalsLogicActions -> - -export const metricsFundamentalsLogic = kea([ - path(['products', 'metrics', 'frontend', 'components', 'metricsFundamentalsLogic']), - - actions({ - setMetricName: (metricName: string) => ({ metricName }), - setAggregation: (aggregation: MetricAggregation) => ({ aggregation }), - runCheck: true, - // The viewer's "explain this number" entry: prefill the metric the user is - // already looking at and run the check, so the handoff needs no retyping. - explainMetric: (payload: { metricName: string; aggregation: MetricAggregation }) => payload, - }), - - reducers({ - metricName: [ - '', - { - setMetricName: (_, { metricName }) => metricName, - explainMetric: (_, { metricName }) => metricName, - }, - ], - aggregation: [ - 'sum' as MetricAggregation, - { - setAggregation: (_, { aggregation }) => aggregation, - explainMetric: (_, { aggregation }) => aggregation, - }, - ], - // Drop the previous decomposition the moment a new check starts. The - // loading flag alone leaves a frame where the old answer is still on - // screen under the new metric name, which reads as the new answer. - checkResult: [null as FundamentalsCheckResult | null, { runCheck: () => null }], - }), - - loaders(({ values }) => ({ - checkResult: [ - null as FundamentalsCheckResult | null, - { - runCheck: async (_, breakpoint) => { - if (!values.metricName) { - return null - } - await breakpoint(300) - - const dateTo = dayjs().startOf('minute') - const dateFrom = dateTo.subtract(LOOKBACK_MINUTES, 'minute') - - // Ask the chart what it would plot first, so the bucket being - // explained is one the product actually drew rather than one - // picked blind — an empty bucket explains nothing. - const series = await metricsQueryCreate(values.projectId, { - query: { - metricName: values.metricName, - aggregation: values.aggregation, - interval: CHECK_INTERVAL, - dateFrom: dateFrom.toISOString(), - dateTo: dateTo.toISOString(), - }, - }) - breakpoint() - - const points = series.results?.[0]?.points ?? [] - const latest = [...points].reverse().find((point) => point.value !== null) - if (!latest) { - lemonToast.info(`No data for ${values.metricName} in the last hour`) - return null - } - - const response = await metricsExplainCreate(values.projectId, { - query: { - metricName: values.metricName, - aggregation: values.aggregation, - bucketStart: latest.time, - interval: CHECK_INTERVAL, - }, - }) - breakpoint() - - return { decomposition: response.decomposition, checkedAt: dayjs().toISOString() } - }, - }, - ], - })), - - selectors({ - projectId: [() => [], () => window.POSTHOG_APP_CONTEXT?.current_project?.id?.toString() ?? ''], - }), - - listeners(({ actions }) => ({ - explainMetric: () => { - // Reducers have already prefilled the name and aggregation; run the check. - actions.runCheck() - }, - })), -]) diff --git a/products/metrics/frontend/components/metricsHandoff.test.ts b/products/metrics/frontend/components/metricsHandoff.test.ts index 72222257fccc..66d788767cd3 100644 --- a/products/metrics/frontend/components/metricsHandoff.test.ts +++ b/products/metrics/frontend/components/metricsHandoff.test.ts @@ -5,19 +5,16 @@ import { AccessControlLevel, AccessControlResourceType, AppContext } from '~/typ import { metricsNamesRetrieve, metricsValuesRetrieve } from '../generated/api' import { metricsCatalogLogic } from './metricsCatalogLogic' -import { metricsFundamentalsLogic } from './metricsFundamentalsLogic' jest.mock('../generated/api', () => ({ ...jest.requireActual('../generated/api'), metricsNamesRetrieve: jest.fn(), metricsValuesRetrieve: jest.fn(), metricsQueryCreate: jest.fn(), - metricsExplainCreate: jest.fn(), })) -// The catalog and fundamentals logics are keyed (global) logics whose state must -// survive a tab flip: a card click preloads the viewer, and the viewer's explain -// button preloads fundamentals. The scene mounts both, so unmounting the tab +// The catalog logic is a keyed (global) logic whose state must survive a tab flip: +// a card click preloads the viewer. The scene mounts it, so unmounting the tab // component that also subscribes must not reset the preloaded state. describe('metrics cross-tab handoffs', () => { beforeEach(() => { @@ -32,29 +29,6 @@ describe('metrics cross-tab handoffs', () => { jest.mocked(metricsNamesRetrieve).mockResolvedValue({ results: [] } as any) }) - it('explainMetric state survives the viewer tab unmounting', async () => { - // Stand-in for the scene-level mount that keeps the logic alive across tabs. - const sceneHold = metricsFundamentalsLogic() - sceneHold.mount() - // Stand-in for the MetricsClauseRow subscription inside the viewer tab. - const viewerHold = metricsFundamentalsLogic() - viewerHold.mount() - - await expectLogic(sceneHold, () => - sceneHold.actions.explainMetric({ metricName: 'cache_size', aggregation: 'avg' }) - ).toFinishAllListeners() - expect(sceneHold.values.metricName).toBe('cache_size') - - // The viewer tab unmounts while fundamentals renders. With only the tab - // holding a subscription, kea would unmount the logic and drop the - // prefill; the scene hold must keep it alive. - viewerHold.unmount() - - expect(sceneHold.values.metricName).toBe('cache_size') - expect(sceneHold.values.aggregation).toBe('avg') - sceneHold.unmount() - }) - it('a loaded catalog survives the explore tab unmounting', async () => { const items = [{ name: 'jobs.processed', metric_type: 'sum' }] jest.mocked(metricsNamesRetrieve).mockResolvedValue({ results: items } as any) diff --git a/products/metrics/frontend/components/metricsOverviewLogic.test.ts b/products/metrics/frontend/components/metricsOverviewLogic.test.ts index baeccfeb8994..bc8cdd38ce12 100644 --- a/products/metrics/frontend/components/metricsOverviewLogic.test.ts +++ b/products/metrics/frontend/components/metricsOverviewLogic.test.ts @@ -14,7 +14,6 @@ jest.mock('../generated/api', () => ({ metricsAttributeValuesRetrieve: jest.fn(), metricsAttributesRetrieve: jest.fn(), metricsCharacterizeCreate: jest.fn(), - metricsExplainCreate: jest.fn(), metricsHasMetricsRetrieve: jest.fn(), metricsOverviewRetrieve: jest.fn(), metricsQueryCreate: jest.fn(), diff --git a/products/metrics/frontend/generated/api.schemas.ts b/products/metrics/frontend/generated/api.schemas.ts index c272af48f380..1f356aeffb82 100644 --- a/products/metrics/frontend/generated/api.schemas.ts +++ b/products/metrics/frontend/generated/api.schemas.ts @@ -295,239 +295,6 @@ export interface _MetricErrorSpikesResponseApi { results: _MetricErrorSpikeApi[] } -/** - * * `gauge` - gauge - * * `sum` - sum - * * `histogram` - histogram - * * `exponential_histogram` - exponential_histogram - * * `summary` - summary - */ -export type OtelMetricTypeEnumApi = (typeof OtelMetricTypeEnumApi)[keyof typeof OtelMetricTypeEnumApi] - -export const OtelMetricTypeEnumApi = { - Gauge: 'gauge', - Sum: 'sum', - Histogram: 'histogram', - ExponentialHistogram: 'exponential_histogram', - Summary: 'summary', -} as const - -/** - * * `second` - second - * * `minute` - minute - * * `minute_5` - minute_5 - * * `minute_15` - minute_15 - * * `hour` - hour - * * `hour_6` - hour_6 - * * `day` - day - * * `week` - week - */ -export type MetricQueryIntervalEnumApi = (typeof MetricQueryIntervalEnumApi)[keyof typeof MetricQueryIntervalEnumApi] - -export const MetricQueryIntervalEnumApi = { - Second: 'second', - Minute: 'minute', - Minute5: 'minute_5', - Minute15: 'minute_15', - Hour: 'hour', - Hour6: 'hour_6', - Day: 'day', - Week: 'week', -} as const - -export interface _MetricExplainBodyApi { - /** - * Exact metric name whose bucket should be taken apart. - * @maxLength 255 - */ - metricName: string - /** Constrain the bucket to one metric type. A name can exist as several types; without this, rows of every type sharing the name are decomposed together. - * - * * `gauge` - gauge - * * `sum` - sum - * * `histogram` - histogram - * * `exponential_histogram` - exponential_histogram - * * `summary` - summary */ - metricType?: OtelMetricTypeEnumApi | null - /** The aggregation whose result should be explained. 'histogram_quantile' is rejected: it reduces bucket-count arrays rather than scalar samples, so there is no per-series value to lay out. - * - * * `sum` - sum - * * `avg` - avg - * * `count` - count - * * `min` - min - * * `max` - max - * * `p95` - p95 - * * `rate` - rate - * * `increase` - increase - * * `histogram_quantile` - histogram_quantile */ - aggregation?: AggregationEnumApi - /** - * Quantile in (0, 1) applied across series. Defaults to 0.95 for the 'p95' aggregation. - * @minimum 0 - * @maximum 1 - * @nullable - */ - quantile?: number | null - /** Label predicates ANDed together, matching the chart the point came from. */ - filters?: _MetricFilterApi[] - /** Start of the bucket to explain, as returned in a query result's 'time'. ISO 8601. */ - bucketStart: string - /** Bucket size the point was plotted at. Must match the query that produced it, or the decomposition explains a different span. - * - * * `second` - second - * * `minute` - minute - * * `minute_5` - minute_5 - * * `minute_15` - minute_15 - * * `hour` - hour - * * `hour_6` - hour_6 - * * `day` - day - * * `week` - week */ - interval: MetricQueryIntervalEnumApi -} - -export interface _MetricExplainRequestApi { - /** The chart point to take apart. */ - query: _MetricExplainBodyApi -} - -/** - * * `none` - none - * * `last` - last - * * `avg_over_time` - avg_over_time - * * `sum_over_time` - sum_over_time - * * `increase` - increase - * * `pooled_samples` - pooled_samples - */ -export type TemporalReducerEnumApi = (typeof TemporalReducerEnumApi)[keyof typeof TemporalReducerEnumApi] - -export const TemporalReducerEnumApi = { - None: 'none', - Last: 'last', - AvgOverTime: 'avg_over_time', - SumOverTime: 'sum_over_time', - Increase: 'increase', - PooledSamples: 'pooled_samples', -} as const - -/** - * * `sum` - sum - * * `avg` - avg - * * `min` - min - * * `max` - max - * * `quantile` - quantile - * * `count_series` - count_series - */ -export type SpatialReducerEnumApi = (typeof SpatialReducerEnumApi)[keyof typeof SpatialReducerEnumApi] - -export const SpatialReducerEnumApi = { - Sum: 'sum', - Avg: 'avg', - Min: 'min', - Max: 'max', - Quantile: 'quantile', - CountSeries: 'count_series', -} as const - -export interface _MetricSampleViewApi { - /** Sample timestamp, ISO 8601. */ - time: string - /** Raw stored reading, before any reduction. */ - value: number -} - -/** - * Per-data-point attributes identifying the series. - */ -export type _MetricSeriesBreakdownApiLabels = { [key: string]: string } - -/** - * Resource attributes identifying the scrape target. - */ -export type _MetricSeriesBreakdownApiResourceLabels = { [key: string]: string } - -export interface _MetricSeriesBreakdownApi { - /** Service that reported this series. */ - service_name: string - /** Per-data-point attributes identifying the series. */ - labels: _MetricSeriesBreakdownApiLabels - /** Resource attributes identifying the scrape target. */ - resource_labels: _MetricSeriesBreakdownApiResourceLabels - /** The series' raw samples in this bucket, oldest first, trimmed for display. */ - samples: _MetricSampleViewApi[] - /** How many samples the series actually sent, even when 'samples' was trimmed. */ - sample_count: number - /** Whether 'samples' lists fewer samples than arrived. */ - samples_truncated: boolean - /** - * What this series contributed after the per-series reduction. Null for percentiles, which read the pooled readings and so have no single per-series contribution. - * @nullable - */ - value: number | null -} - -export interface _MetricBucketDecompositionApi { - /** Metric that was decomposed. */ - metric_name: string - /** OTel metric type observed in the bucket. */ - metric_type: string - /** OTel aggregation temporality observed in the bucket ('cumulative', 'delta', or empty for gauges). */ - temporality: string - /** Aggregation that was explained. */ - aggregation: string - /** Start of the explained bucket, ISO 8601. */ - bucket_start: string - /** Bucket size the point was plotted at. */ - interval: string - /** How each series' samples were collapsed to one value: 'last' for an instant gauge reading, 'avg_over_time' for an average, 'sum_over_time' for delta counters, 'increase' for cumulative counters, and 'pooled_samples' for percentiles, which skip the per-series step entirely. - * - * * `none` - none - * * `last` - last - * * `avg_over_time` - avg_over_time - * * `sum_over_time` - sum_over_time - * * `increase` - increase - * * `pooled_samples` - pooled_samples */ - temporal_reducer: TemporalReducerEnumApi - /** How the per-series values were combined into the bucket's number. - * - * * `sum` - sum - * * `avg` - avg - * * `min` - min - * * `max` - max - * * `quantile` - quantile - * * `count_series` - count_series */ - spatial_reducer: SpatialReducerEnumApi - /** The series behind the point, largest contributors first, trimmed for display. */ - series: _MetricSeriesBreakdownApi[] - /** How many series reported in the bucket. */ - series_count: number - /** How many raw samples the bucket held across all series. */ - sample_count: number - /** Whether 'series' lists fewer series than reported. */ - series_truncated: boolean - /** Whether the bucket held more raw rows than the decomposition reads. Totals are computed only over the rows that were read. */ - rows_truncated: boolean - /** - * The bucket's value recomputed from the raw samples, independently of the query builders. Null when no series reported. - * @nullable - */ - reference_value: number | null - /** - * The value the product would plot for this point. Null when the query returned no row. - * @nullable - */ - actual_value: number | null - /** - * Whether the two values match. False means one of the reductions is wrong, and the series breakdown shows where they parted. Null when the raw read was truncated, so the two are not comparable. - * @nullable - */ - agrees: boolean | null -} - -export interface _MetricExplainResponseApi { - /** The bucket taken apart. */ - decomposition: _MetricBucketDecompositionApi -} - export interface _HasMetricsResponseApi { /** Whether the team has ingested any metrics. */ hasMetrics: boolean @@ -572,6 +339,23 @@ export interface _MetricsOverviewResponseApi { services: _MetricsOverviewServiceApi[] } +/** + * * `gauge` - gauge + * * `sum` - sum + * * `histogram` - histogram + * * `exponential_histogram` - exponential_histogram + * * `summary` - summary + */ +export type OtelMetricTypeEnumApi = (typeof OtelMetricTypeEnumApi)[keyof typeof OtelMetricTypeEnumApi] + +export const OtelMetricTypeEnumApi = { + Gauge: 'gauge', + Sum: 'sum', + Histogram: 'histogram', + ExponentialHistogram: 'exponential_histogram', + Summary: 'summary', +} as const + export interface _MetricGroupByApi { /** * Attribute name to split series by (e.g. 'k8s.pod.name', 'env'). @@ -586,6 +370,29 @@ export interface _MetricGroupByApi { scope?: MetricAttributeScopeEnumApi } +/** + * * `second` - second + * * `minute` - minute + * * `minute_5` - minute_5 + * * `minute_15` - minute_15 + * * `hour` - hour + * * `hour_6` - hour_6 + * * `day` - day + * * `week` - week + */ +export type MetricQueryIntervalEnumApi = (typeof MetricQueryIntervalEnumApi)[keyof typeof MetricQueryIntervalEnumApi] + +export const MetricQueryIntervalEnumApi = { + Second: 'second', + Minute: 'minute', + Minute5: 'minute_5', + Minute15: 'minute_15', + Hour: 'hour', + Hour6: 'hour_6', + Day: 'day', + Week: 'week', +} as const + export interface _MetricClauseApi { /** * Clause name a formula refers to (e.g. 'a'). diff --git a/products/metrics/frontend/generated/api.ts b/products/metrics/frontend/generated/api.ts index 411333ea8d95..db916a058a92 100644 --- a/products/metrics/frontend/generated/api.ts +++ b/products/metrics/frontend/generated/api.ts @@ -23,8 +23,6 @@ import type { _MetricAttributeValuesResponseApi, _MetricCatalogValuesParamsApi, _MetricErrorSpikesResponseApi, - _MetricExplainRequestApi, - _MetricExplainResponseApi, _MetricNamesResponseApi, _MetricPickerNamesResponseApi, _MetricQueryRequestApi, @@ -194,28 +192,6 @@ export const metricsErrorSpikesRetrieve = async ( }) } -export const getMetricsExplainCreateUrl = (projectId: string) => { - return `/api/projects/${projectId}/metrics/explain/` -} - -/** - * Take one chart point apart into the series and samples behind it, - * and recompute it independently so the plotted number can be checked - * rather than trusted. - */ -export const metricsExplainCreate = async ( - projectId: string, - _metricExplainRequestApi: _MetricExplainRequestApi, - options?: RequestInit -): Promise<_MetricExplainResponseApi> => { - return apiMutator<_MetricExplainResponseApi>(getMetricsExplainCreateUrl(projectId), { - ...options, - method: 'POST', - headers: { 'Content-Type': 'application/json', ...options?.headers }, - body: JSON.stringify(_metricExplainRequestApi), - }) -} - export const getMetricsHasMetricsRetrieveUrl = (projectId: string) => { return `/api/projects/${projectId}/metrics/has_metrics/` } diff --git a/products/metrics/frontend/generated/api.zod.ts b/products/metrics/frontend/generated/api.zod.ts index 0fe892683a90..93bbc160fc82 100644 --- a/products/metrics/frontend/generated/api.zod.ts +++ b/products/metrics/frontend/generated/api.zod.ts @@ -114,105 +114,6 @@ export const MetricsCharacterizeCreateBody = /* @__PURE__ */ zod.object({ .describe('The anomaly characterization to run.'), }) -/** - * Take one chart point apart into the series and samples behind it, - * and recompute it independently so the plotted number can be checked - * rather than trusted. - */ -export const metricsExplainCreateBodyQueryOneMetricNameMax = 255 - -export const metricsExplainCreateBodyQueryOneAggregationDefault = `sum` -export const metricsExplainCreateBodyQueryOneQuantileMin = 0 -export const metricsExplainCreateBodyQueryOneQuantileMax = 1 - -export const metricsExplainCreateBodyQueryOneFiltersItemKeyMax = 255 - -export const metricsExplainCreateBodyQueryOneFiltersItemOpDefault = `eq` -export const metricsExplainCreateBodyQueryOneFiltersItemValueMax = 1024 - -export const metricsExplainCreateBodyQueryOneFiltersItemScopeDefault = `auto` - -export const MetricsExplainCreateBody = /* @__PURE__ */ zod.object({ - query: zod - .object({ - metricName: zod - .string() - .max(metricsExplainCreateBodyQueryOneMetricNameMax) - .describe('Exact metric name whose bucket should be taken apart.'), - metricType: zod - .union([ - zod - .enum(['gauge', 'sum', 'histogram', 'exponential_histogram', 'summary']) - .describe( - '\* `gauge` - gauge\n\* `sum` - sum\n\* `histogram` - histogram\n\* `exponential_histogram` - exponential_histogram\n\* `summary` - summary' - ), - zod.null(), - ]) - .optional() - .describe( - 'Constrain the bucket to one metric type. A name can exist as several types; without this, rows of every type sharing the name are decomposed together.\n\n\* `gauge` - gauge\n\* `sum` - sum\n\* `histogram` - histogram\n\* `exponential_histogram` - exponential_histogram\n\* `summary` - summary' - ), - aggregation: zod - .enum(['sum', 'avg', 'count', 'min', 'max', 'p95', 'rate', 'increase', 'histogram_quantile']) - .describe( - '\* `sum` - sum\n\* `avg` - avg\n\* `count` - count\n\* `min` - min\n\* `max` - max\n\* `p95` - p95\n\* `rate` - rate\n\* `increase` - increase\n\* `histogram_quantile` - histogram_quantile' - ) - .default(metricsExplainCreateBodyQueryOneAggregationDefault) - .describe( - "The aggregation whose result should be explained. 'histogram_quantile' is rejected: it reduces bucket-count arrays rather than scalar samples, so there is no per-series value to lay out.\n\n\* `sum` - sum\n\* `avg` - avg\n\* `count` - count\n\* `min` - min\n\* `max` - max\n\* `p95` - p95\n\* `rate` - rate\n\* `increase` - increase\n\* `histogram_quantile` - histogram_quantile" - ), - quantile: zod - .number() - .min(metricsExplainCreateBodyQueryOneQuantileMin) - .max(metricsExplainCreateBodyQueryOneQuantileMax) - .nullish() - .describe("Quantile in (0, 1) applied across series. Defaults to 0.95 for the 'p95' aggregation."), - filters: zod - .array( - zod.object({ - key: zod - .string() - .max(metricsExplainCreateBodyQueryOneFiltersItemKeyMax) - .describe( - "Attribute name to filter on, without any type-tag suffix (e.g. 'k8s.pod.name', 'env')." - ), - op: zod - .enum(['eq', 'neq', 'regex', 'not_regex']) - .describe('\* `eq` - eq\n\* `neq` - neq\n\* `regex` - regex\n\* `not_regex` - not_regex') - .default(metricsExplainCreateBodyQueryOneFiltersItemOpDefault) - .describe( - "Comparison operator. 'regex'\/'not_regex' use RE2 syntax. Negative operators also match rows that lack the key entirely, mirroring Prometheus negative matchers.\n\n\* `eq` - eq\n\* `neq` - neq\n\* `regex` - regex\n\* `not_regex` - not_regex" - ), - value: zod - .string() - .max(metricsExplainCreateBodyQueryOneFiltersItemValueMax) - .describe('Value to compare against. For regex operators this is the pattern.'), - scope: zod - .enum(['resource', 'attribute', 'auto']) - .describe('\* `resource` - resource\n\* `attribute` - attribute\n\* `auto` - auto') - .default(metricsExplainCreateBodyQueryOneFiltersItemScopeDefault) - .describe( - "Where the attribute lives: 'resource' = per-target resource attributes (k8s.pod.name, service.version), 'attribute' = per-datapoint attributes (http.method, path), 'auto' = resource first with per-datapoint fallback. Use 'auto' unless you know the exact scope.\n\n\* `resource` - resource\n\* `attribute` - attribute\n\* `auto` - auto" - ), - }) - ) - .optional() - .describe('Label predicates ANDed together, matching the chart the point came from.'), - bucketStart: zod.iso - .datetime({ offset: true }) - .describe("Start of the bucket to explain, as returned in a query result's 'time'. ISO 8601."), - interval: zod - .enum(['second', 'minute', 'minute_5', 'minute_15', 'hour', 'hour_6', 'day', 'week']) - .describe( - '\* `second` - second\n\* `minute` - minute\n\* `minute_5` - minute_5\n\* `minute_15` - minute_15\n\* `hour` - hour\n\* `hour_6` - hour_6\n\* `day` - day\n\* `week` - week' - ) - .describe( - 'Bucket size the point was plotted at. Must match the query that produced it, or the decomposition explains a different span.\n\n\* `second` - second\n\* `minute` - minute\n\* `minute_5` - minute_5\n\* `minute_15` - minute_15\n\* `hour` - hour\n\* `hour_6` - hour_6\n\* `day` - day\n\* `week` - week' - ), - }) - .describe('The chart point to take apart.'), -}) - export const metricsQueryCreateBodyQueryOneMetricNameMax = 255 export const metricsQueryCreateBodyQueryOneAggregationDefault = `sum` diff --git a/products/metrics/frontend/metricsSceneLogic.tsx b/products/metrics/frontend/metricsSceneLogic.tsx index bb6b562974c2..ef34f5050753 100644 --- a/products/metrics/frontend/metricsSceneLogic.tsx +++ b/products/metrics/frontend/metricsSceneLogic.tsx @@ -34,8 +34,8 @@ import { export const METRICS_SQL_EDITOR_TAB_ID = 'metrics-sql-editor' -export type MetricsSceneActiveTab = 'overview' | 'explore' | 'viewer' | 'sql' | 'fundamentals' -const VALID_ACTIVE_TABS: MetricsSceneActiveTab[] = ['overview', 'explore', 'viewer', 'sql', 'fundamentals'] +export type MetricsSceneActiveTab = 'overview' | 'explore' | 'viewer' | 'sql' +const VALID_ACTIVE_TABS: MetricsSceneActiveTab[] = ['overview', 'explore', 'viewer', 'sql'] export const DEFAULT_ACTIVE_TAB: MetricsSceneActiveTab = 'overview' // kea-router pre-parses JSON-looking params, so anything a user types into the URL can reach diff --git a/products/metrics/mcp/tools.yaml b/products/metrics/mcp/tools.yaml index 644d59e29e96..c58954d60137 100644 --- a/products/metrics/mcp/tools.yaml +++ b/products/metrics/mcp/tools.yaml @@ -47,9 +47,6 @@ tools: metrics-error-spikes-retrieve: operation: metrics_error_spikes_retrieve enabled: false - metrics-explain-create: - operation: metrics_explain_create - enabled: false metrics-has-metrics-retrieve: operation: metrics_has_metrics_retrieve enabled: false diff --git a/products/posthog_ai/frontend/components/composer/ComposerModelEffortPickers.tsx b/products/posthog_ai/frontend/components/composer/ComposerModelEffortPickers.tsx index 651beda01c0c..6f288ae949b3 100644 --- a/products/posthog_ai/frontend/components/composer/ComposerModelEffortPickers.tsx +++ b/products/posthog_ai/frontend/components/composer/ComposerModelEffortPickers.tsx @@ -74,7 +74,8 @@ export interface ComposerModelEffortPickersProps { */ lockedRuntimeAdapter?: string | null /** The selection shown is the resolved default (user/project preference), not an explicit pick for - * this run — the model trigger renders a "Default ·" prefix so that's visible at a glance. */ + * this run — the lemon model trigger renders a "Default ·" prefix so that's visible at a glance. The quill + * trigger shows only the model, like PostHog Desktop. */ isDefaultSelection?: boolean /** Clears the explicit pick so the run falls back to the resolved default. Omit on a surface with no * configured default and the reset row falls back to the ladder's balanced notch. */ @@ -99,9 +100,12 @@ interface PickerSectionProps { } /** One `label … current ›` row of the cascade, opening a radio list. */ -const PICKER_CHROME: Record = { - lemon: { triggerVariant: 'outline', icons: true }, - quill: { triggerVariant: 'default', icons: false }, +const PICKER_CHROME: Record< + ThreadSkin, + { triggerVariant: 'outline' | 'default'; icons: boolean; defaultPrefix: boolean } +> = { + lemon: { triggerVariant: 'outline', icons: true, defaultPrefix: true }, + quill: { triggerVariant: 'default', icons: false, defaultPrefix: false }, } function PickerSection({ title, current, value, onValueChange, children, footer }: PickerSectionProps): JSX.Element { @@ -242,7 +246,7 @@ export function ComposerModelEffortPickers({ - {isDefaultSelection ? `Default · ${modelLabel}` : modelLabel} + {isDefaultSelection && chrome.defaultPrefix ? `Default · ${modelLabel}` : modelLabel} {effortOptions.length > 0 && ( {getEffortLabel(selectedEffort)} )} diff --git a/products/posthog_ai/frontend/scenes/TaskTracker/TaskRunArtifacts.stories.tsx b/products/posthog_ai/frontend/scenes/TaskTracker/TaskRunArtifacts.stories.tsx index 8994a9be02a1..8a2fde3632ff 100644 --- a/products/posthog_ai/frontend/scenes/TaskTracker/TaskRunArtifacts.stories.tsx +++ b/products/posthog_ai/frontend/scenes/TaskTracker/TaskRunArtifacts.stories.tsx @@ -11,7 +11,6 @@ import { SceneLayout } from '~/layout/scenes/SceneLayout' import { TodayShell } from '~/layout/today/TodayShell' import { todayShellLogic } from '~/layout/today/todayShellLogic' import { mswDecorator } from '~/mocks/browser' -import TRENDS_LINE_INSIGHT from '~/mocks/fixtures/api/projects/team_id/insights/trendsLine.json' import type { MockSignature } from '~/mocks/utils' import type { @@ -20,6 +19,8 @@ import type { } from 'products/tasks/frontend/generated/api.schemas' import { TaskRuntimeEnumApi } from 'products/tasks/frontend/generated/api.schemas' +import { expect, userEvent, waitFor } from 'storybook/test' + import { OriginProduct, Task, TaskRun, TaskRunEnvironment, TaskRunStatus } from '../../types/taskTypes' import { TaskDetailPage } from './components/TaskDetailPage' import { TaskRunTab } from './taskRunArtifacts' @@ -203,7 +204,7 @@ const WALKTHROUGH_WEBM_BASE64 = function objectReference( id: string, name: string, - objectKind: 'insight' | 'dashboard' | 'flag' | 'experiment' | 'cohort' | 'survey', + objectKind: string, objectId: string, uploadedAt: string ): TaskRunArtifactResponseApi { @@ -231,21 +232,10 @@ const OBJECT_REFERENCES = [ objectReference('phref_experiment', 'Plan picker layout test', 'experiment', '12', '2026-09-28T18:09:00Z'), objectReference('phref_cohort', 'Trial starters on laptops', 'cohort', '3', '2026-09-28T18:08:00Z'), objectReference('phref_survey', 'Plan picker feedback', 'survey', 'survey-plan-picker', '2026-09-28T18:07:00Z'), + // No kind called `note` has a page, so this reference shows the card. + objectReference('phref_note', 'Pricing notes', 'note', 'pricing-notes', '2026-09-28T18:06:00Z'), ] -const CITED_INSIGHT = { ...TRENDS_LINE_INSIGHT, short_id: 'aBcD1234', name: 'Trial funnel by step' } - -// The live insight embed loads the saved insight, then runs its query. -const OBJECT_MOCKS = { - get: { - '/api/environments/:team_id/insights/': { count: 1, results: [CITED_INSIGHT] }, - '/api/projects/:team_id/insights/': { count: 1, results: [CITED_INSIGHT] }, - }, - post: { - '/api/environments/:team_id/query/': { results: CITED_INSIGHT.result }, - }, -} - const VIDEO_ARTIFACT: TaskRunArtifactResponseApi = { id: 'artifact-walkthrough', name: 'plan-picker-walkthrough.webm', @@ -534,8 +524,7 @@ export const Video: Story = { } function objectMocks(): ReturnType { - const mocks = taskMocks([...ARTIFACTS, ...OBJECT_REFERENCES]) - return { get: { ...mocks.get, ...OBJECT_MOCKS.get }, post: { ...mocks.post, ...OBJECT_MOCKS.post } } + return taskMocks([...ARTIFACTS, ...OBJECT_REFERENCES]) } export const PostHogObjects: Story = { @@ -543,9 +532,9 @@ export const PostHogObjects: Story = { render: () => , } -export const PostHogObjectWithoutEmbed: Story = { +export const PostHogObjectWithoutPage: Story = { parameters: { msw: { mocks: objectMocks() } }, - render: () => , + render: () => , } export const Versions: Story = { @@ -642,14 +631,13 @@ function livingMocks(): ReturnType { return { get: { ...mocks.get, - ...OBJECT_MOCKS.get, [`/api/projects/:team_id/tasks/${TASK_ID}/runs/:run_id/living_artifacts/`]: { artifacts: LIVING_DOCUMENTS, }, [`/api/projects/:team_id/tasks/${TASK_ID}/runs/:run_id/living_artifacts/doc-trial-chart/versions/:version/`]: () => new HttpResponse(CHART_SVG, { headers: { 'Content-Type': 'image/svg+xml' } }), }, - post: { ...mocks.post, ...OBJECT_MOCKS.post }, + post: mocks.post, } } @@ -851,3 +839,39 @@ export const ImageCommentPins: Story = { parameters: { msw: { mocks: commentMocks() } }, render: () => , } + +export const MarkdownCommentThread: Story = { + parameters: { msw: { mocks: commentMocks() } }, + render: () => , + play: async ({ canvasElement }) => { + const highlight = await waitFor( + () => { + const element = canvasElement.querySelector( + '[data-attr="task-artifact-comment-highlight"]' + ) + expect(element).not.toBeNull() + return element! + }, + { timeout: 10_000 } + ) + await userEvent.click(highlight) + }, +} + +export const ImageCommentThread: Story = { + parameters: { msw: { mocks: commentMocks() } }, + render: () => , + play: async ({ canvasElement }) => { + const pin = await waitFor( + () => { + const element = canvasElement.querySelector( + '[data-attr="task-artifact-comment-pin-marker"]' + ) + expect(element).not.toBeNull() + return element! + }, + { timeout: 10_000 } + ) + await userEvent.click(pin) + }, +} diff --git a/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactCommentActions.tsx b/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactCommentActions.tsx index f6149b2b859f..a5b7c4d95971 100644 --- a/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactCommentActions.tsx +++ b/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactCommentActions.tsx @@ -1,18 +1,15 @@ import { useActions, useValues } from 'kea' -import { IconComment, IconPin } from '@posthog/icons' -import { Button, Text, Tooltip, TooltipContent, TooltipTrigger, cn } from '@posthog/quill-primitives' +import { IconPin } from '@posthog/icons' +import { Button, Tooltip, TooltipContent, TooltipTrigger, cn } from '@posthog/quill-primitives' import { TaskArtifactCommentsLogicProps, taskArtifactCommentsLogic } from '../taskArtifactCommentsLogic' -import { taskRunArtifactsLogic } from '../taskRunArtifactsLogic' +import { ArtifactCommentsMenu } from './ArtifactCommentsMenu' -/** The toolbar controls for comments: show the panel, and on an image, pin a comment to a spot. */ +/** The toolbar controls for comments: the comments menu, and on an image, pin a comment to a spot. */ export function ArtifactCommentActions({ logicProps }: { logicProps: TaskArtifactCommentsLogicProps }): JSX.Element { - const { openCount, pinMode } = useValues(taskArtifactCommentsLogic(logicProps)) + const { pinMode } = useValues(taskArtifactCommentsLogic(logicProps)) const { setPinMode } = useActions(taskArtifactCommentsLogic(logicProps)) - const { commentsOpen } = useValues(taskRunArtifactsLogic({ taskId: logicProps.taskId })) - const { setCommentsOpen } = useActions(taskRunArtifactsLogic({ taskId: logicProps.taskId })) - const commentsLabel = commentsOpen ? 'Hide comments' : 'Show comments' const pinLabel = pinMode ? 'Stop pinning' : 'Pin a comment to a spot on the image' return ( <> @@ -37,28 +34,7 @@ export function ArtifactCommentActions({ logicProps }: { logicProps: TaskArtifac {pinLabel} )} - - 0 ? `${commentsLabel}, ${openCount} open` : commentsLabel} - aria-pressed={commentsOpen} - className={cn(commentsOpen && 'bg-fill-selected')} - onClick={() => setCommentsOpen(!commentsOpen)} - data-attr="task-artifact-comments-toggle" - /> - } - > - - {openCount > 0 && ( - } className="tabular-nums"> - {openCount} - - )} - - {commentsLabel} - + ) } diff --git a/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactCommentThreadCard.tsx b/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactCommentThreadCard.tsx index 5f25931448b3..83301b266850 100644 --- a/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactCommentThreadCard.tsx +++ b/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactCommentThreadCard.tsx @@ -1,7 +1,7 @@ import { useActions, useValues } from 'kea' import { useEffect, useRef } from 'react' -import { IconCheck, IconPin, IconRefresh } from '@posthog/icons' +import { IconCheck, IconPin, IconRefresh, IconX } from '@posthog/icons' import { Badge, Button, Text, ThreadItemAction, ThreadItemGroup, cn } from '@posthog/quill-primitives' import { ArtifactCommentThread } from '../artifactComments' @@ -13,9 +13,11 @@ import { ArtifactCommentEntry } from './ArtifactCommentEntry' export function ArtifactCommentThreadCard({ logicProps, thread, + inline = false, }: { logicProps: TaskArtifactCommentsLogicProps thread: ArtifactCommentThread + inline?: boolean }): JSX.Element { const { activeThreadId, writing, drafts } = useValues(taskArtifactCommentsLogic(logicProps)) const { activateThread, setThreadResolved, replyToThread, setDraft } = useActions( @@ -28,10 +30,10 @@ export function ArtifactCommentThreadCard({ // A click on a highlight or a pin in the preview brings its thread into view. useEffect(() => { - if (active) { + if (active && !inline) { ref.current?.scrollIntoView({ block: 'nearest', behavior: 'smooth' }) } - }, [active]) + }, [active, inline]) const resolveAction = ( +
+
+ {anchor?.kind === 'text' && ( + } + className="line-clamp-2 w-full border-l-2 border-warning pl-2 italic" + > + {anchor.quote} + + )} + {thread.pinNumber && ( + + + {`Pin ${thread.pinNumber}`} + + )} + {thread.resolved && Resolved} +
+ +
+
+ + + {thread.replies.map((reply) => ( + + ))} + +
+ {!thread.resolved && ( +
+ setDraft(rootId, value)} + onSubmit={() => replyToThread(rootId)} + saving={writing === rootId} + busy={!!writing && writing !== rootId} + label="Reply" + placeholder="Reply" + submitLabel="Reply" + rows={2} + dataAttr="task-artifact-comment-reply" + /> +
+ )} + + ) + } + return (
void }): JSX.Element { + const name = thread.root.created_by ? fullNameOrEmail(thread.root.created_by) : 'Deleted user' + const { anchor } = thread + return ( + + ) +} + +function ThreadList({ logicProps }: { logicProps: TaskArtifactCommentsLogicProps }): JSX.Element { + const { visibleThreads, threads, commentsLoadFailed, commentsLoading } = useValues( + taskArtifactCommentsLogic(logicProps) + ) + const { loadComments, activateThread } = useActions(taskArtifactCommentsLogic(logicProps)) + const { setCommentsOpen } = useActions(taskRunArtifactsLogic({ taskId: logicProps.taskId })) + if (!threads) { + if (commentsLoadFailed) { + return ( + + + + + + Comments didn't load + Check your connection and try again. + + + + + + ) + } + return ( +
+ {[0, 1].map((index) => ( +
+ + +
+ ))} +
+ ) + } + if (!visibleThreads || visibleThreads.length === 0) { + return ( + + + + + + {threads.length > 0 ? 'No open comments' : 'No comments yet'} + {emptyHint(logicProps.kind)} + + + ) + } + return ( +
+ {visibleThreads.map((thread, index) => ( + + {index > 0 && } + {thread.anchor && thread.anchor.kind !== 'document' ? ( + { + setCommentsOpen(false) + activateThread(thread.root.id, 'menu') + }} + /> + ) : ( + + )} + + ))} +
+ ) +} + +export function ArtifactCommentsMenu({ logicProps }: { logicProps: TaskArtifactCommentsLogicProps }): JSX.Element { + const { openCount, resolvedCount, showResolved, drafts, writing } = useValues(taskArtifactCommentsLogic(logicProps)) + const { setShowResolved, setDraft, submitComment } = useActions(taskArtifactCommentsLogic(logicProps)) + const { commentsOpen } = useValues(taskRunArtifactsLogic({ taskId: logicProps.taskId })) + const { setCommentsOpen } = useActions(taskRunArtifactsLogic({ taskId: logicProps.taskId })) + const label = openCount > 0 ? `Comments, ${openCount} open` : 'Comments' + const switchId = `task-artifact-comments-show-resolved-${logicProps.artifactId}` + return ( + setCommentsOpen(open)}> + + }> + + } + > + + {openCount > 0 && ( + } className="tabular-nums"> + {openCount} + + )} + + + Comments + + +
+ }> + Comments + + {resolvedCount > 0 && ( +
+ setShowResolved(checked)} + data-attr="task-artifact-comments-show-resolved" + /> + +
+ )} +
+
+ setDraft('document', value)} + onSubmit={() => submitComment('document')} + saving={writing === 'document'} + busy={!!writing && writing !== 'document'} + label="Comment on this file" + placeholder="Comment on this file" + dataAttr="task-artifact-comment-document" + /> +
+ +
+
+ ) +} diff --git a/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactCommentsPanel.tsx b/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactCommentsPanel.tsx deleted file mode 100644 index 86f3ce84ffe5..000000000000 --- a/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactCommentsPanel.tsx +++ /dev/null @@ -1,170 +0,0 @@ -import { useActions, useValues } from 'kea' -import { Fragment } from 'react' - -import { IconComment, IconWarning, IconX } from '@posthog/icons' -import { - Button, - Empty, - EmptyContent, - EmptyDescription, - EmptyHeader, - EmptyMedia, - EmptyTitle, - Label, - Separator, - Skeleton, - SkeletonText, - Switch, - Text, - Tooltip, - TooltipContent, - TooltipTrigger, -} from '@posthog/quill-primitives' - -import { supportsSelectionComments } from '../artifactComments' -import { TaskArtifactCommentsLogicProps, taskArtifactCommentsLogic } from '../taskArtifactCommentsLogic' -import { taskRunArtifactsLogic } from '../taskRunArtifactsLogic' -import { ArtifactCommentComposer } from './ArtifactCommentComposer' -import { ArtifactCommentThreadCard } from './ArtifactCommentThreadCard' - -function emptyHint(kind: TaskArtifactCommentsLogicProps['kind']): string { - if (kind === 'image') { - return 'Comment on the whole image above, or pin a comment to a spot on it.' - } - if (supportsSelectionComments(kind)) { - return 'Comment on the whole file above, or select text in the preview to comment on it.' - } - return 'Comment on the whole file above.' -} - -function ThreadList({ logicProps }: { logicProps: TaskArtifactCommentsLogicProps }): JSX.Element { - const { visibleThreads, threads, commentsLoadFailed, commentsLoading } = useValues( - taskArtifactCommentsLogic(logicProps) - ) - const { loadComments } = useActions(taskArtifactCommentsLogic(logicProps)) - if (!threads) { - if (commentsLoadFailed) { - return ( - - - - - - Comments didn't load - Check your connection and try again. - - - - - - ) - } - return ( -
- {[0, 1].map((index) => ( -
- - -
- ))} -
- ) - } - if (!visibleThreads || visibleThreads.length === 0) { - return ( - - - - - - {threads.length > 0 ? 'No open comments' : 'No comments yet'} - {emptyHint(logicProps.kind)} - - - ) - } - return ( -
- {visibleThreads.map((thread, index) => ( - - {index > 0 && } - - - ))} -
- ) -} - -/** The comment threads on the open artifact version, with a box to comment on the whole file. */ -export function ArtifactCommentsPanel({ logicProps }: { logicProps: TaskArtifactCommentsLogicProps }): JSX.Element { - const { resolvedCount, showResolved, drafts, writing } = useValues(taskArtifactCommentsLogic(logicProps)) - const { setShowResolved, setDraft, submitComment } = useActions(taskArtifactCommentsLogic(logicProps)) - const { setCommentsOpen } = useActions(taskRunArtifactsLogic({ taskId: logicProps.taskId })) - const switchId = `task-artifact-comments-show-resolved-${logicProps.artifactId}` - return ( - - ) -} diff --git a/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactImagePins.tsx b/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactImagePins.tsx index 46cdd30f01f7..2b62c302aea2 100644 --- a/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactImagePins.tsx +++ b/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactImagePins.tsx @@ -1,5 +1,5 @@ import { useActions, useValues } from 'kea' -import { useEffect, useRef } from 'react' +import { CSSProperties, useEffect, useRef } from 'react' import { cn } from '@posthog/quill-primitives' @@ -7,6 +7,7 @@ import { fullNameOrEmail } from 'lib/utils/strings' import type { RegionCommentAnchor } from '../artifactComments' import { TaskArtifactCommentsLogicProps, taskArtifactCommentsLogic } from '../taskArtifactCommentsLogic' +import { ArtifactInlineThread } from './ArtifactInlineThread' import { ArtifactPendingComment } from './ArtifactPendingComment' /** The point of a pin sits on its region's bottom left corner, the same spot Desktop draws it on. */ @@ -14,6 +15,19 @@ function pinStyle(anchor: RegionCommentAnchor): { left: string; top: string } { return { left: `${anchor.x * 100}%`, top: `${(anchor.y + anchor.height) * 100}%` } } +function besidePinStyle(region: RegionCommentAnchor): CSSProperties { + return { + ...(region.y < 0.5 + ? { top: `${(region.y + region.height) * 100}%` } + : { bottom: `${(1 - region.y - region.height) * 100}%` }), + ...(region.x < 0.5 ? { left: `${region.x * 100}%` } : { right: `${(1 - region.x - region.width) * 100}%` }), + } +} + +function besidePinClassName(region: RegionCommentAnchor): string { + return cn('pointer-events-auto', region.y < 0.5 ? 'mt-1' : 'mb-8') +} + function PinMarker({ label, number, @@ -57,7 +71,7 @@ export function ArtifactImagePins({ logicProps }: { logicProps: TaskArtifactComm const { activateThread } = useActions(taskArtifactCommentsLogic(logicProps)) const rootRef = useRef(null) - // A pick in the comments panel scrolls its pin into view when the image is zoomed in. + // A pick in the comments menu scrolls its pin into view when the image is zoomed in. useEffect(() => { if (activeThreadId) { rootRef.current @@ -67,6 +81,8 @@ export function ArtifactImagePins({ logicProps }: { logicProps: TaskArtifactComm }, [activeThreadId]) const pendingRegion = pendingAnchor?.kind === 'region' ? pendingAnchor : null + const activeAnchor = anchoredThreads.find((thread) => thread.root.id === activeThreadId)?.anchor + const activeRegion = activeAnchor?.kind === 'region' ? activeAnchor : null return (
{anchoredThreads.map((thread) => { @@ -91,19 +107,19 @@ export function ArtifactImagePins({ logicProps }: { logicProps: TaskArtifactComm )} + {activeRegion && !pendingRegion && ( + + )}
) } diff --git a/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactInlineThread.tsx b/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactInlineThread.tsx new file mode 100644 index 000000000000..27ef144174b2 --- /dev/null +++ b/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactInlineThread.tsx @@ -0,0 +1,74 @@ +import { useActions, useValues } from 'kea' +import { CSSProperties, useEffect, useRef } from 'react' + +import { cn } from '@posthog/quill-primitives' + +import { TaskArtifactCommentsLogicProps, taskArtifactCommentsLogic } from '../taskArtifactCommentsLogic' +import { ArtifactCommentThreadCard } from './ArtifactCommentThreadCard' + +export const INLINE_THREAD_WIDTH_PX = 320 + +const KEEP_OPEN_SELECTOR = + '[data-quill-portal], [data-attr="task-artifact-comment-highlight"], [data-attr="task-artifact-comment-pin-marker"], [data-attr="task-artifact-comments-toggle"]' + +export function ArtifactInlineThread({ + logicProps, + style, + className, +}: { + logicProps: TaskArtifactCommentsLogicProps + style: CSSProperties + className?: string +}): JSX.Element | null { + const { activeThreadId, threads } = useValues(taskArtifactCommentsLogic(logicProps)) + const { activateThread } = useActions(taskArtifactCommentsLogic(logicProps)) + const ref = useRef(null) + const thread = threads?.find((candidate) => candidate.root.id === activeThreadId) ?? null + const open = !!thread + + useEffect(() => { + if (!open) { + return + } + const onKeyDown = (event: KeyboardEvent): void => { + if (event.key === 'Escape') { + activateThread(null) + } + } + const onPointerDown = (event: PointerEvent): void => { + const target = event.target + if (target instanceof Element && (ref.current?.contains(target) || target.closest(KEEP_OPEN_SELECTOR))) { + return + } + activateThread(null) + } + window.addEventListener('keydown', onKeyDown) + document.addEventListener('pointerdown', onPointerDown) + return () => { + window.removeEventListener('keydown', onKeyDown) + document.removeEventListener('pointerdown', onPointerDown) + } + }, [open, activateThread]) + + if (!thread) { + return null + } + return ( +
event.stopPropagation()} + onClick={(event) => event.stopPropagation()} + data-attr="task-artifact-comment-inline-thread" + > + +
+ ) +} diff --git a/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactObjectEmbed.tsx b/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactObjectEmbed.tsx index 32ad021d927b..0f17d0ea01fc 100644 --- a/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactObjectEmbed.tsx +++ b/products/posthog_ai/frontend/scenes/TaskTracker/components/ArtifactObjectEmbed.tsx @@ -1,73 +1,32 @@ -import { Dashboard } from 'scenes/dashboard/Dashboard' -import { SessionRecordingPlayer } from 'scenes/session-recordings/player/SessionRecordingPlayer' -import { SessionRecordingPlayerMode } from 'scenes/session-recordings/player/sessionRecordingPlayerLogic' +import { useState } from 'react' -import { Query } from '~/queries/Query/Query' -import { NodeKind } from '~/queries/schema/schema-general' -import { DashboardPlacement, InsightShortId } from '~/types' +import { Spinner, cn } from '@posthog/quill-primitives' -import type { PostHogObjectRef } from '../taskRunArtifacts' - -function EmbedBody({ objectKind, objectId }: PostHogObjectRef): JSX.Element | null { - if (objectKind === 'insight') { - return ( -
-
- -
-
- ) - } - if (objectKind === 'hogql') { - // The SQL itself is the object id for this kind. - return ( -
- -
- ) - } - if (objectKind === 'dashboard') { - return ( -
- -
- ) - } - if (objectKind === 'replay') { - return ( -
- -
- ) - } - return null -} +import { EMBEDDED_PAGE_FRAME_NAME } from 'lib/utils/embeddedPageFrame' /** - * The live object a reference points at, rendered with the same components its own page uses. - * Kept in its own module so the insight, dashboard and replay code loads only when one opens. + * A cited object's own page, the same page its URL opens. The frame keeps that page's URL and navigation + * apart from the task page, and the frame name makes the app show the page without its navigation. */ -export function ArtifactObjectEmbed(ref: PostHogObjectRef): JSX.Element { - // These are LemonUI page components inside the quill artifacts pane, so they need PostHog's own color tokens back. +export function ArtifactObjectEmbed({ url, title }: { url: string; title: string }): JSX.Element { + const [loaded, setLoaded] = useState(false) return ( -
- +
+ {!loaded && ( +
+ +
+ )} + {/* The page is the app's own, so it keeps its origin, scripts, forms and new tabs. The sandbox + still stops it navigating the task page away. */} +