diff --git a/apps/opik-frontend/src/constants/colorVariants.ts b/apps/opik-frontend/src/constants/colorVariants.ts index ec8f5ca08fc..83e58d7ccdc 100644 --- a/apps/opik-frontend/src/constants/colorVariants.ts +++ b/apps/opik-frontend/src/constants/colorVariants.ts @@ -14,7 +14,11 @@ export const COLOR_VARIANTS = [ ] as const; export type ColorVariant = (typeof COLOR_VARIANTS)[number]; -export type ExtendedColorVariant = ColorVariant | "primary" | "default"; +export type ExtendedColorVariant = + | ColorVariant + | "primary" + | "purpleDark" + | "default"; export const COLOR_VARIANTS_MAP: Record< ExtendedColorVariant, @@ -31,6 +35,7 @@ export const COLOR_VARIANTS_MAP: Record< turquoise: { css: "var(--color-turquoise)", hex: "#06b6d4" }, blue: { css: "var(--color-blue)", hex: "#3b82f6" }, primary: { css: "var(--color-primary)", hex: "#6366f1" }, + purpleDark: { css: "var(--color-purple-dark)", hex: "#491b7e" }, default: { css: "var(--color-gray)", hex: "#64748b" }, }; @@ -40,8 +45,10 @@ export const PRESET_HEX_COLORS = COLOR_VARIANTS.map( export const DEFAULT_HEX_COLOR = COLOR_VARIANTS_MAP.blue.hex; +// `primary` and `purpleDark` are not in COLOR_VARIANTS (they are not picker presets) but are still +// reachable as resolved colors, so their css vars must round-trip to hex for the color picker. export const CSS_VAR_TO_HEX: Record = Object.fromEntries( - [...COLOR_VARIANTS, "primary" as const].map((v) => [ + [...COLOR_VARIANTS, "primary" as const, "purpleDark" as const].map((v) => [ COLOR_VARIANTS_MAP[v].css, COLOR_VARIANTS_MAP[v].hex, ]), diff --git a/apps/opik-frontend/src/lib/colorVariants.test.ts b/apps/opik-frontend/src/lib/colorVariants.test.ts new file mode 100644 index 00000000000..27bf2bf19ca --- /dev/null +++ b/apps/opik-frontend/src/lib/colorVariants.test.ts @@ -0,0 +1,261 @@ +import { describe, expect, it } from "vitest"; + +import { + resolveChartColorMap, + resolveColor, + resolveHexColor, +} from "@/lib/colorVariants"; +import { TAG_VARIANTS, TAG_VARIANTS_COLOR_MAP } from "@/ui/tag"; +import { COLOR_VARIANTS_MAP } from "@/constants/colorVariants"; + +/** + * Perceptual helpers live here rather than in shipped code: only these invariants need them, and + * shipping an unused utility invites a second, divergent implementation. + */ +const linearize = (c: number) => + c <= 0.04045 ? c / 12.92 : Math.pow((c + 0.055) / 1.055, 2.4); + +const linearChannels = (hex: string) => + [1, 3, 5].map((offset) => + linearize(parseInt(hex.slice(offset, offset + 2), 16) / 255), + ) as [number, number, number]; + +const toLab = (hex: string): [number, number, number] => { + const [r, g, b] = linearChannels(hex); + + const x = (r * 0.4124564 + g * 0.3575761 + b * 0.1804375) / 0.95047; + const y = r * 0.2126729 + g * 0.7151522 + b * 0.072175; + const z = (r * 0.0193339 + g * 0.119192 + b * 0.9503041) / 1.08883; + + const f = (t: number) => (t > 0.008856 ? Math.cbrt(t) : 7.787 * t + 16 / 116); + + return [116 * f(y) - 16, 500 * (f(x) - f(y)), 200 * (f(y) - f(z))]; +}; + +/** CIE76 distance. Below ~25 two thin chart lines are hard to tell apart without hovering. */ +const perceptualDistance = (a: string, b: string) => { + const [l1, a1, b1] = toLab(a); + const [l2, a2, b2] = toLab(b); + return Math.hypot(l1 - l2, a1 - a2, b1 - b2); +}; + +const relativeLuminance = (hex: string) => { + const [r, g, b] = linearChannels(hex); + return 0.2126 * r + 0.7152 * g + 0.0722 * b; +}; + +const contrastRatio = (a: string, b: string) => { + const la = relativeLuminance(a); + const lb = relativeLuminance(b); + return (Math.max(la, lb) + 0.05) / (Math.min(la, lb) + 0.05); +}; + +// The grounds the product actually paints: `--background` is 0 0% 100% in the light theme and +// 0 0% 7% in the dark one (main.scss). +const LIGHT_GROUND = "#ffffff"; +const DARK_GROUND = "#121212"; + +const autoPaletteHexes = () => + TAG_VARIANTS.map((variant) => ({ + variant: variant as string, + hex: resolveHexColor(TAG_VARIANTS_COLOR_MAP[variant!]), + })); + +describe("automatic palette invariants", () => { + it("resolves every entry to a hex value", () => { + for (const { variant, hex } of autoPaletteHexes()) { + expect(hex, `${variant} must round-trip to hex`).toMatch( + /^#[0-9a-fA-F]{6}$/, + ); + } + }); + + it("keeps every pair of entries perceptually distinguishable", () => { + // The automatic color is `hash % TAG_VARIANTS.length`, so any two labels can land on any two + // entries. Regression guard for OPIK-7839, where `primary` sat at ΔE 14.5 from `purple` and + // made distinct grouped series look identical. + const MINIMUM_DISTANCE = 30; + const palette = autoPaletteHexes(); + const tooClose: string[] = []; + + for (let i = 0; i < palette.length; i += 1) { + for (let j = i + 1; j < palette.length; j += 1) { + const distance = perceptualDistance(palette[i].hex, palette[j].hex); + if (distance < MINIMUM_DISTANCE) { + tooClose.push( + `${palette[i].variant}/${palette[j].variant} ΔE=${distance.toFixed( + 1, + )}`, + ); + } + } + } + + expect(tooClose).toEqual([]); + }); + + it("keeps the palette at ten entries", () => { + // The automatic color is `hash % TAG_VARIANTS.length`. The golden test below samples four + // labels, which would not notice a length change that only moves labels it does not sample, + // so the length is pinned separately. Changing it re-colors every label in the product. + expect(TAG_VARIANTS).toHaveLength(10); + }); + + it("pins the design-approved hex for the entry excused from the contrast check", () => { + // `purpleDark` is skipped by the contrast assertion below, so without this the approved value + // could drift to something the check would otherwise have rejected. + expect(COLOR_VARIANTS_MAP.purpleDark.hex.toLowerCase()).toBe("#491b7e"); + expect(resolveHexColor(COLOR_VARIANTS_MAP.purpleDark.css)).toBe( + COLOR_VARIANTS_MAP.purpleDark.hex, + ); + }); + + it("never assigns the indigo that was confusable with purple and blue", () => { + const indigo = COLOR_VARIANTS_MAP.primary.hex.toLowerCase(); + + expect( + autoPaletteHexes().map(({ hex }) => hex.toLowerCase()), + ).not.toContain(indigo); + }); + + it("keeps known labels on the colors they resolve to today", () => { + // Guards the property that makes a palette change safe to ship: the automatic color depends on + // the palette length, so adding or removing an entry silently re-colors every label in the + // product. Substituting one entry only moves labels that resolved to that slot. If this fails, + // a change shifted labels it did not intend to. + expect( + resolveChartColorMap([ + "domain-agent", + "supervisor-agent", + "planner-agent", + "retriever-agent", + ]), + ).toEqual({ + "domain-agent": "var(--color-purple-dark)", + "supervisor-agent": "var(--color-blue)", + "planner-agent": "var(--color-orange)", + "retriever-agent": "var(--color-yellow)", + }); + }); + + it("still lets two labels share a color once the palette is exhausted", () => { + // Documented limitation, not a defect to fix here: with ten colors, identical colors remain + // possible as the number of series approaches the palette size. The answer is the manual + // per-label override, not automatic reassignment — reassigning would break the guarantee that + // a label keeps one color across widgets. + const map = resolveChartColorMap(["id_filter_enabled", "domain-agent"]); + + expect(map["id_filter_enabled"]).toBe(map["domain-agent"]); + }); + + it("keeps entries legible as thin lines on both grounds", () => { + // WCAG 1.4.11 asks for 3:1 and names "each line in a graph", so that is the floor here. + // Five of the ten entries do not meet it and are tracked as a palette-contrast follow-up: + // light ground — yellow 1.92, turquoise 2.43, green 2.54, orange 2.80 (all pre-existing) + // dark ground — purpleDark 1.55 (design-chosen; reads well on light, but is nearly the + // dark theme's own background) + // Half the palette failing is the finding, not an accident of this test: a single palette + // serving both themes cannot clear the floor on both, which is why the follow-up proposes two + // lightness bands. The assertion still blocks *new* entries from joining the list, and the + // list must not grow. + const KNOWN_LOW_CONTRAST = [ + "yellow", + "turquoise", + "green", + "orange", + "purpleDark", + ]; + const MINIMUM_CONTRAST = 3; + const failing: string[] = []; + + for (const { variant, hex } of autoPaletteHexes()) { + if (KNOWN_LOW_CONTRAST.includes(variant)) continue; + + const light = contrastRatio(hex, LIGHT_GROUND); + const dark = contrastRatio(hex, DARK_GROUND); + if (Math.min(light, dark) < MINIMUM_CONTRAST) { + failing.push( + `${variant} light=${light.toFixed(2)} dark=${dark.toFixed(2)}`, + ); + } + } + + expect(failing).toEqual([]); + }); +}); + +describe("resolveColor priority", () => { + const label = "domain-agent"; + + it("falls back to an automatic color when no map contains the label", () => { + expect(resolveColor(label)).toBeDefined(); + }); + + it("prefers a caller-supplied color over the automatic one", () => { + const reserved = { [label]: COLOR_VARIANTS_MAP.gray.css }; + + expect(resolveColor(label, null, reserved)).toBe( + COLOR_VARIANTS_MAP.gray.css, + ); + expect(resolveColor(label, null, reserved)).not.toBe(resolveColor(label)); + }); + + it("prefers the workspace color over both the caller-supplied and the automatic one", () => { + const workspace = { [label]: "#123456" }; + const reserved = { [label]: COLOR_VARIANTS_MAP.gray.css }; + + expect(resolveColor(label, workspace, reserved)).toBe("#123456"); + }); + + it("leaves labels absent from the maps on their automatic color", () => { + const other = "supervisor-agent"; + const reserved = { [label]: COLOR_VARIANTS_MAP.gray.css }; + + expect(resolveColor(other, null, reserved)).toBe(resolveColor(other)); + }); + + it("assigns a label the same color regardless of which other labels are present", () => { + // Cross-widget consistency is a product requirement, not an incidental property: the same tag + // must keep one color across widgets that show different sets of series. + const inOneChart = resolveChartColorMap([label, "supervisor-agent"]); + const inAnother = resolveChartColorMap([label, "planner-agent", "a", "b"]); + + expect(inOneChart[label]).toBe(inAnother[label]); + expect(resolveChartColorMap([label])[label]).toBe(inOneChart[label]); + }); +}); + +describe("resolveChartColorMap", () => { + it("returns one color per label", () => { + const labels = ["alpha", "beta", "gamma"]; + const map = resolveChartColorMap(labels); + + expect(Object.keys(map).sort()).toEqual(labels.sort()); + }); + + it("returns an empty map for an empty label list", () => { + expect(resolveChartColorMap([])).toEqual({}); + }); + + it("resolves an empty-string label to a palette color rather than undefined", () => { + // Group-by values can be empty strings; a missing color makes recharts fall back to black. + expect(resolveChartColorMap([""])[""]).toMatch(/^var\(--color-/); + }); + + it("leaves a value that is already hex untouched and passes unknown values through", () => { + expect(resolveHexColor("#123abc")).toBe("#123abc"); + expect(resolveHexColor("var(--color-not-a-real-token)")).toBe( + "var(--color-not-a-real-token)", + ); + }); + + it("pins only the labels the caller names, leaving the rest automatic", () => { + // How a chart fixes colors for specific series (metric sub-series today) without disturbing + // labels it says nothing about. + const pinned = { traces: COLOR_VARIANTS_MAP.purple.css }; + const map = resolveChartColorMap(["traces", "domain-agent"], null, pinned); + + expect(map.traces).toBe(COLOR_VARIANTS_MAP.purple.css); + expect(map["domain-agent"]).toBe(resolveColor("domain-agent")); + }); +}); diff --git a/apps/opik-frontend/src/main.scss b/apps/opik-frontend/src/main.scss index 44a6122692b..9a60e4bf4bd 100644 --- a/apps/opik-frontend/src/main.scss +++ b/apps/opik-frontend/src/main.scss @@ -205,6 +205,7 @@ --color-lime: #a3e635; --color-turquoise: #06b6d4; --color-blue: #3b82f6; + --color-purple-dark: #491b7e; --color-primary: #6366f1; --color-indigo: #6366f1; --color-violet: #8b5cf6; @@ -249,6 +250,8 @@ --tag-blue-text: #19426b; --tag-lavender-bg: #e5e5fe; --tag-lavender-text: #3b3b8e; + --tag-purple-dark-bg: #e4d7f7; + --tag-purple-dark-text: #3d1669; --click-blue: #262ab5; @@ -517,6 +520,8 @@ --tag-blue-text: #6eabe7; --tag-lavender-bg: #2a2a3d; --tag-lavender-text: #a8a8e0; + --tag-purple-dark-bg: #2a2233; + --tag-purple-dark-text: #b98ce6; /* Trace/span type colors - Dark Mode */ --type-trace: #945fcf; diff --git a/apps/opik-frontend/src/shared/ColorIndicator/ColorIndicator.test.tsx b/apps/opik-frontend/src/shared/ColorIndicator/ColorIndicator.test.tsx new file mode 100644 index 00000000000..0e214814f37 --- /dev/null +++ b/apps/opik-frontend/src/shared/ColorIndicator/ColorIndicator.test.tsx @@ -0,0 +1,68 @@ +import { describe, expect, it, vi } from "vitest"; +import { fireEvent, render, screen } from "@testing-library/react"; + +import ColorIndicator from "./ColorIndicator"; +import { TooltipProvider } from "@/ui/tooltip"; + +// The indicator's own persistence path is not what this file is about: it covers the interaction +// contract of the nested Radix triggers, which is what customers hit when they try to recolour a +// series (OPIK_7840 — the control was reported as missing because it was hard to reach). +vi.mock("@/hooks/useUpdateColorMapping", () => ({ + default: () => ({ + updateColor: vi.fn(), + previewColor: {}, + setPreviewColor: vi.fn(), + isPending: false, + }), +})); + +const renderIndicator = (onParentClick?: () => void) => { + const utils = render( + +
+ +
+ , + ); + + // The indicator renders no text, so the trigger is found by the role Radix gives it. + const trigger = utils.container.querySelector( + "[data-state]", + ) as HTMLElement | null; + + if (!trigger) throw new Error("colour indicator trigger not rendered"); + + return { ...utils, trigger }; +}; + +describe("ColorIndicator", () => { + it("opens the colour picker when the indicator is clicked", async () => { + const { trigger } = renderIndicator(); + + fireEvent.click(trigger); + + // The hex field is the picker's only stable, user-visible landmark. + expect(await screen.findByPlaceholderText("#000000")).toBeInTheDocument(); + }); + + it("does not fire a surrounding click handler when the indicator is clicked", async () => { + // In a chart legend the label around the dot navigates to filtered traces. Opening the picker + // must not also trigger that navigation. + const onParentClick = vi.fn(); + const { trigger } = renderIndicator(onParentClick); + + fireEvent.click(trigger); + + await screen.findByPlaceholderText("#000000"); + expect(onParentClick).not.toHaveBeenCalled(); + }); + + it("gives the indicator a pointer target larger than the dot itself", () => { + // The dot is 6px. Without the enlarged target it is effectively unhittable, which is why the + // feature read as missing. The dot's own size stays unchanged. + const { trigger } = renderIndicator(); + + expect(trigger.className).toContain("size-1.5"); + expect(trigger.className).toMatch(/before:-inset-/); + }); +}); diff --git a/apps/opik-frontend/src/shared/ColorIndicator/ColorIndicator.tsx b/apps/opik-frontend/src/shared/ColorIndicator/ColorIndicator.tsx index c2d5c96ef22..25b1ba83ccd 100644 --- a/apps/opik-frontend/src/shared/ColorIndicator/ColorIndicator.tsx +++ b/apps/opik-frontend/src/shared/ColorIndicator/ColorIndicator.tsx @@ -22,6 +22,15 @@ const colorIndicatorVariants = cva("bg-[var(--bg-color)]", { }, }); +/** + * The dot is 6px, which is too small to find or aim at, so customers reported the colour override as + * missing entirely (OPIK_7840). A transparent pseudo-element enlarges the pointer target without + * changing how the indicator looks. Horizontal growth is kept to 6px because in a chart legend the + * dot sits immediately left of the label, and the label has its own click action to drill down. + */ +const HIT_AREA = + "before:absolute before:-inset-y-2 before:-inset-x-1.5 before:content-['']"; + type ColorIndicatorProps = VariantProps & { label: string; colorKey?: string; @@ -83,11 +92,14 @@ const ColorIndicator: React.FC = ({ return ( - + {/* No hover delay: the affordance is small, so the hint has to arrive the moment it is found. */} +
e.stopPropagation()} /> diff --git a/apps/opik-frontend/src/ui/tag.tsx b/apps/opik-frontend/src/ui/tag.tsx index cfd703de7d6..309a4d01900 100644 --- a/apps/opik-frontend/src/ui/tag.tsx +++ b/apps/opik-frontend/src/ui/tag.tsx @@ -19,6 +19,8 @@ const tagVariants = cva("inline-block truncate rounded-sm transition-colors", { turquoise: "bg-[var(--tag-turquoise-bg)] text-[var(--tag-turquoise-text)]", blue: "bg-[var(--tag-blue-bg)] text-[var(--tag-blue-text)]", + purpleDark: + "bg-[var(--tag-purple-dark-bg)] text-[var(--tag-purple-dark-text)]", lavender: "bg-[var(--tag-lavender-bg)] text-[var(--tag-lavender-text)]", white: "border border-gray-200 bg-white text-muted-slate dark:border-gray-600 dark:bg-gray-800 dark:text-foreground", @@ -60,11 +62,25 @@ const Tag = React.forwardRef( ); Tag.displayName = "Tag"; +/** + * Palette used to derive a color automatically from a label (see `generateTagVariant`). + * + * Two invariants are enforced by `lib/colorVariants.test.ts` and must hold for any change here: + * - every pair of entries stays perceptually distinguishable (OPIK-7839: `primary` #6366f1 sat at + * ΔE 14.5 from `purple`, which made distinct series look identical in grouped charts); + * - the list keeps its length, because the automatic color is `hash % TAG_VARIANTS.length` — + * adding or removing an entry re-maps every label in the product, while substituting one + * re-maps only the labels that resolved to that slot. + * + * `primary` is deliberately absent: it is too close to both `purple` and `blue` for chart series. + * The tenth entry is design-owned (`purple-dark`, from the product palette); see colorVariants.test.ts + * for the invariants any replacement must still satisfy. + */ export const TAG_VARIANTS: Exclude< TagProps["variant"], "red" | "transparent" | "white" | "lavender" >[] = [ - "primary", + "purpleDark", "gray", "purple", "burgundy", @@ -94,6 +110,7 @@ export const TAG_VARIANTS_COLOR_MAP: Record< green: "var(--color-green)", turquoise: "var(--color-turquoise)", blue: "var(--color-blue)", + purpleDark: "var(--color-purple-dark)", }; export { Tag, tagVariants }; diff --git a/apps/opik-frontend/src/v2/pages-shared/dashboards/widgets/ProjectMetricsWidget/MetricChart/MetricChartContainer.tsx b/apps/opik-frontend/src/v2/pages-shared/dashboards/widgets/ProjectMetricsWidget/MetricChart/MetricChartContainer.tsx index edc5ba9eb15..3a2fa83ec57 100644 --- a/apps/opik-frontend/src/v2/pages-shared/dashboards/widgets/ProjectMetricsWidget/MetricChart/MetricChartContainer.tsx +++ b/apps/opik-frontend/src/v2/pages-shared/dashboards/widgets/ProjectMetricsWidget/MetricChart/MetricChartContainer.tsx @@ -74,7 +74,10 @@ interface MetricContainerChartProps { hideYAxis?: boolean; } -const customColorMap = { +// Fixed colors for metric sub-series (trace counts, cost, duration percentiles, token kinds). +// Only valid when no breakdown is applied — with a breakdown the line names are group values, +// not metric names, so these keys would hijack a group that happens to share a name. +const METRIC_COLOR_MAP = { traces: COLOR_VARIANTS_MAP.purple.css, cost: COLOR_VARIANTS_MAP.purple.css, "duration.p50": COLOR_VARIANTS_MAP.turquoise.css, @@ -198,7 +201,11 @@ const MetricContainerChart = ({ ); }, [data, lines, isPending]); - const config = useChartConfig(lines, labelsMap, colorMap ?? customColorMap); + const config = useChartConfig( + lines, + labelsMap, + colorMap ?? (breakdown ? undefined : METRIC_COLOR_MAP), + ); const labelActions = useMemo(() => { if (!getLabelAction) return undefined; diff --git a/tests_end_to_end/coverage/taxonomy.yaml b/tests_end_to_end/coverage/taxonomy.yaml index 8cf71c3b996..5d7453ef7ee 100644 --- a/tests_end_to_end/coverage/taxonomy.yaml +++ b/tests_end_to_end/coverage/taxonomy.yaml @@ -363,7 +363,9 @@ areas: label: Dashboards nav_group: Observability routes: ["/$workspaceName/dashboards", "/dashboards/$dashboardId", "/projects/$projectId/dashboards"] + spec_dir: dashboards specs: + - dashboards/chart-series-colors.spec.ts capabilities: list-dashboards: { covered: false } create-dashboard: { covered: false } @@ -371,7 +373,7 @@ areas: edit-dashboard: { covered: false } delete-dashboard: { covered: false } add-widget: { covered: false } - configure-widget: { covered: false } + configure-widget: { covered: true, tier: t2-cuj, note: "grouped vs ungrouped series colours; the widget is configured through the API, the editor UI itself is still uncovered" } widget-filters: { covered: false } metric-date-range: { covered: false } diff --git a/tests_end_to_end/e2e/core/backend/client.ts b/tests_end_to_end/e2e/core/backend/client.ts index cecfca384c2..eb3807cbe81 100644 --- a/tests_end_to_end/e2e/core/backend/client.ts +++ b/tests_end_to_end/e2e/core/backend/client.ts @@ -97,6 +97,21 @@ export interface AnnotationQueueDetail { reviewers: AnnotationQueueReviewerRef[]; } +export interface DashboardRef { + id: string; + name: string; +} + +/** + * One series of a project-metrics response — the shape a chart widget turns + * into a line. `name` is the metric sub-series (`traces`, `duration.p50`) or, + * when the request carries a breakdown, the group value (a tag, a trace name). + */ +export interface ProjectMetricSeriesRef { + name: string; + points: Array<{ time: string; value: number | null }>; +} + export interface OptimizationRef { id: string; name: string; @@ -124,6 +139,37 @@ export function makeBackendClient(apiKey: string | null = null) { apiUrl: env.apiBaseUrl, }); + /** + * A raw call to a private REST endpoint the pinned TS SDK does not model. + * Everything the SDK *does* expose goes through `opik.api.*` — this is only + * for the gaps (dashboards, the windowed/breakdown metric requests). + */ + const privateFetch = async ( + method: 'GET' | 'POST' | 'DELETE', + path: string, + body?: unknown, + ): Promise => { + const headers: Record = { + Accept: 'application/json', + 'Comet-Workspace': env.workspace, + }; + if (body !== undefined) headers['Content-Type'] = 'application/json'; + const key = apiKey ?? env.apiKey; + if (key) headers['Authorization'] = key; + + const res = await fetch(`${env.apiBaseUrl}${path}`, { + method, + headers, + ...(body === undefined ? {} : { body: JSON.stringify(body) }), + }); + if (!res.ok) { + throw new Error( + `${method} ${path} -> ${res.status}: ${(await res.text()).slice(0, 300)}`, + ); + } + return res; + }; + // Hoisted so the poll helpers (free functions) can call it without depending // on the not-yet-constructed return object. const localGetOptimization = async (id: string): Promise => { @@ -314,21 +360,7 @@ export function makeBackendClient(apiKey: string | null = null) { if (args.fromTime) params.set('from_time', args.fromTime.toISOString()); if (args.toTime) params.set('to_time', args.toTime.toISOString()); - const headers: Record = { - Accept: 'application/json', - 'Comet-Workspace': env.workspace, - }; - const key = apiKey ?? env.apiKey; - if (key) headers['Authorization'] = key; - - const res = await fetch(`${env.apiBaseUrl}/v1/private/projects/stats?${params}`, { - headers, - }); - if (!res.ok) { - throw new Error( - `GET /v1/private/projects/stats -> ${res.status}: ${(await res.text()).slice(0, 300)}`, - ); - } + const res = await privateFetch('GET', `/v1/private/projects/stats?${params}`); const body = (await res.json()) as { content?: Array<{ project_id?: string; @@ -352,6 +384,87 @@ export function makeBackendClient(apiKey: string | null = null) { })); }, + /** + * The series a project-metrics chart widget would draw — the exact call + * `useProjectMetric` makes, so a spec can check the data really groups the + * way it expects before opening a browser to look at the chart. + * + * `breakdownField` is a `BREAKDOWN_FIELD` value (`tags`, `name`, …). + * Omitting it requests the ungrouped series, where `name` is the metric + * sub-series rather than a group value. + */ + async getProjectMetricSeries(args: { + projectId: string; + metricType: string; + interval: 'HOURLY' | 'DAILY' | 'WEEKLY' | 'TOTAL'; + intervalStart: Date; + intervalEnd: Date; + breakdownField?: string; + }): Promise { + const res = await privateFetch( + 'POST', + `/v1/private/projects/${args.projectId}/metrics`, + { + metric_type: args.metricType, + interval: args.interval, + interval_start: args.intervalStart.toISOString(), + interval_end: args.intervalEnd.toISOString(), + ...(args.breakdownField ? { breakdown: { field: args.breakdownField } } : {}), + }, + ); + const body = (await res.json()) as { + results?: Array<{ name?: string; data?: Array<{ time: string; value: number | null }> }>; + }; + return (body.results ?? []).map((r) => ({ + name: String(r.name ?? ''), + points: r.data ?? [], + })); + }, + + /** + * Creates a dashboard from a full config document — the same payload the + * FE's `useDashboardCreateMutation` posts, so a widget can be seeded + * already configured instead of being assembled through the editor UI. + * + * The id only comes back in the `Location` header (the endpoint answers + * 201 with an empty body), which is what `extractIdFromLocation` reads. + */ + async createDashboard(args: { + name: string; + type: 'multi_project' | 'experiments'; + config: unknown; + description?: string; + }): Promise { + const res = await privateFetch('POST', '/v1/private/dashboards', { + name: args.name, + type: args.type, + config: args.config, + ...(args.description ? { description: args.description } : {}), + }); + const location = res.headers.get('location'); + const id = location?.split('/').pop(); + if (!id) { + throw new Error( + `POST /v1/private/dashboards returned no usable Location header: ${location}`, + ); + } + return { id, name: args.name }; + }, + + /** + * Dashboards are not swept by `global-teardown` (it only knows + * experiments, datasets and projects) and do not cascade with the project + * their widgets point at, so whatever creates one must delete it. + */ + async deleteDashboard(id: string): Promise { + try { + await privateFetch('DELETE', `/v1/private/dashboards/${id}`); + } catch (err) { + if (isNotFoundMessage(err)) return; + throw err; + } + }, + async findExperimentByName(name: string): Promise { const page = await opik.api.experiments.findExperiments({ name, size: 50 }); const content = page.content ?? []; @@ -553,6 +666,15 @@ export function makeBackendClient(apiKey: string | null = null) { }; } +/** + * The 404 check for `privateFetch`, which throws a plain Error carrying the + * status rather than the SDK's structured `statusCode` — used so a delete of + * an already-deleted entity stays a no-op, exactly as the SDK-backed deletes do. + */ +function isNotFoundMessage(err: unknown): boolean { + return err instanceof Error && / -> 404:/.test(err.message); +} + function isNotFoundError(err: unknown): boolean { return ( typeof err === 'object' && diff --git a/tests_end_to_end/e2e/core/backend/index.ts b/tests_end_to_end/e2e/core/backend/index.ts index 4b1336541d8..8241932bc32 100644 --- a/tests_end_to_end/e2e/core/backend/index.ts +++ b/tests_end_to_end/e2e/core/backend/index.ts @@ -14,5 +14,7 @@ export { type AutomationRuleRef, type AnnotationQueueDetail, type AnnotationQueueReviewerRef, + type DashboardRef, + type ProjectMetricSeriesRef, } from './client'; export { type PollFeedbackScoreOpts } from './poll-feedback-score'; diff --git a/tests_end_to_end/e2e/fixtures/index.ts b/tests_end_to_end/e2e/fixtures/index.ts index 66e1201c29b..e5858f225ad 100644 --- a/tests_end_to_end/e2e/fixtures/index.ts +++ b/tests_end_to_end/e2e/fixtures/index.ts @@ -1,4 +1,4 @@ -export { test, expect } from './filterable-traces.fixture'; +export { test, expect } from './series-colors-dashboard.fixture'; export type { ProjectFixtures } from './project.fixture'; export type { ScratchDir, ScratchDirFixtures } from './scratch-dir.fixture'; export type { @@ -50,4 +50,13 @@ export type { FilterableTraceRef, FilterableTracesFixtures, } from './filterable-traces.fixture'; +export type { + SeriesColorsDashboardRef, + SeriesColorsDashboardFixtures, +} from './series-colors-dashboard.fixture'; +export { + SERIES_COLORS_TAGS, + SERIES_COLORS_TIME_RANGE, + SERIES_COLORS_WIDGETS, +} from './series-colors-dashboard.fixture'; export type { ProjectRef } from '../core/backend'; diff --git a/tests_end_to_end/e2e/fixtures/series-colors-dashboard.fixture.ts b/tests_end_to_end/e2e/fixtures/series-colors-dashboard.fixture.ts new file mode 100644 index 00000000000..91b9a3c0fd7 --- /dev/null +++ b/tests_end_to_end/e2e/fixtures/series-colors-dashboard.fixture.ts @@ -0,0 +1,171 @@ +import { expect as baseExpect } from '@playwright/test'; +import { test as baseTest } from './filterable-traces.fixture'; +import { shouldLeaveArtifacts } from '../core/artifacts'; + +/** Widget titles, which are also how a spec addresses a widget on the page. */ +export const SERIES_COLORS_WIDGETS = { + groupedByTag: 'Traces grouped by tag', + ungrouped: 'Traces with no grouping', + duration: 'Trace duration percentiles', +} as const; + +/** + * Tag values carried by the seeded traces, one per trace, so each becomes its + * own group when the chart is grouped by Tags. + * + * `traces` and `cost` are chosen deliberately: they collide with keys of the + * chart's fixed metric colour map, which is what makes them able to detect a + * grouped chart resolving group colours through that map instead of through + * the tag palette. The rest are ordinary values that hash to distinct slots. + */ +export const SERIES_COLORS_TAGS = ['traces', 'cost', 'beta', 'zeta', 'iota'] as const; + +/** + * Ages, in days, of the traces seeded per tag. Three points spread over three + * days so a DAILY chart draws an actual line per group: recharts renders no + * line curve at all for a single-point series, only a dot. + */ +const TRACE_AGES_DAYS = [0.2, 1.2, 2.2]; + +/** Distinct durations so the p50/p90/p99 series are not the same number. */ +const TRACE_DURATIONS_SECONDS = [0.5, 2, 8]; + +/** + * The window the spec pins on the dashboard URL. Wide enough to hold every + * seeded trace, and > 3 days so the chart buckets DAILY rather than HOURLY. + */ +export const SERIES_COLORS_TIME_RANGE = 'past7days'; +const TIME_RANGE_DAYS = 7; + +export interface SeriesColorsDashboardRef { + /** The dashboard holding the three widgets, ready to open. */ + id: string; + name: string; + /** Group values present on the grouped chart, one per seeded tag. */ + groups: string[]; +} + +export interface SeriesColorsDashboardFixtures { + seriesColorsDashboard: SeriesColorsDashboardRef; +} + +/** + * A project of tagged traces plus a dashboard whose widgets read it, seeded + * entirely through the API so the browser only ever has to *look* at charts. + * + * The three widgets cover both sides of the "is a fixed metric colour map + * applied?" question on one page: one chart grouped by Tags (colours must come + * from the tag palette), and two ungrouped ones (colours must come from the + * fixed map). + * + * The widgets are configured in the created config rather than through the + * widget editor. That keeps the spec's subject the rendering, not the editor — + * and it avoids the editor's own default of switching a newly grouped widget + * to Total aggregation, which collapses every series to a single point. + */ +export const test = baseTest.extend({ + seriesColorsDashboard: async ( + { sdkClient, backendClient, project, testNamespace }, + use, + testInfo, + ) => { + for (const tag of SERIES_COLORS_TAGS) { + for (const [index, ageDays] of TRACE_AGES_DAYS.entries()) { + await sdkClient.python.createNestedTrace({ + project_name: project.name, + name: `${testNamespace}-${tag}-${index}`, + input: { query: tag }, + output: { answer: tag }, + tags: [tag], + age_days: ageDays, + duration_seconds: TRACE_DURATIONS_SECONDS[index], + spans: [], + }); + } + } + + // Prove the seed really produced one group per tag before anything opens a + // browser. Without this a chart that renders nothing — because ingestion + // had not landed, or because the grouping silently returned a single + // series — would make the UI assertions unreachable rather than failing. + const window = { + intervalStart: new Date(Date.now() - TIME_RANGE_DAYS * 24 * 60 * 60 * 1000), + intervalEnd: new Date(), + }; + await baseExpect + .poll( + async () => { + const series = await backendClient.getProjectMetricSeries({ + projectId: project.id, + metricType: 'TRACE_COUNT', + interval: 'DAILY', + breakdownField: 'tags', + ...window, + }); + return series.map((s) => s.name).sort(); + }, + { timeout: 60_000 }, + ) + .toEqual([...SERIES_COLORS_TAGS].sort()); + + const widget = (id: string, title: string, config: Record) => ({ + id, + title, + type: 'project_metrics', + config: { projectId: project.id, chartType: 'line', traceFilters: [], ...config }, + }); + + const created = await backendClient.createDashboard({ + name: `${testNamespace}-dash`, + type: 'multi_project', + config: { + version: 4, + sections: [ + { + id: 'series-colors', + title: 'Series colours', + widgets: [ + widget('w-grouped', SERIES_COLORS_WIDGETS.groupedByTag, { + metricType: 'TRACE_COUNT', + breakdown: { field: 'tags' }, + }), + widget('w-ungrouped', SERIES_COLORS_WIDGETS.ungrouped, { + metricType: 'TRACE_COUNT', + breakdown: { field: 'none' }, + }), + widget('w-duration', SERIES_COLORS_WIDGETS.duration, { + metricType: 'DURATION', + }), + ], + layout: [ + { i: 'w-grouped', x: 0, y: 0, w: 3, h: 5 }, + { i: 'w-ungrouped', x: 3, y: 0, w: 3, h: 5 }, + { i: 'w-duration', x: 0, y: 5, w: 3, h: 5 }, + ], + }, + ], + lastModified: 0, + }, + }); + + await testInfo.attach('opik.seriesColorsDashboard', { + body: JSON.stringify({ id: created.id, name: created.name }, null, 2), + contentType: 'application/json', + }); + + await use({ id: created.id, name: created.name, groups: [...SERIES_COLORS_TAGS] }); + + // The traces cascade with the project fixture's own teardown; the + // dashboard does not belong to the project and is outside the run-prefix + // sweep, so it has to be deleted here. + if (!shouldLeaveArtifacts(testInfo)) { + try { + await backendClient.deleteDashboard(created.id); + } catch (err) { + console.warn(`[seriesColorsDashboard fixture] delete warning for ${created.name}:`, err); + } + } + }, +}); + +export { expect } from './filterable-traces.fixture'; diff --git a/tests_end_to_end/e2e/pom/dashboard.page.ts b/tests_end_to_end/e2e/pom/dashboard.page.ts new file mode 100644 index 00000000000..a302815850b --- /dev/null +++ b/tests_end_to_end/e2e/pom/dashboard.page.ts @@ -0,0 +1,139 @@ +import { test, expect } from '@playwright/test'; +import type { Page, Locator } from '@playwright/test'; +import { loadEnvConfig } from '../config/env.config'; + +/** + * A workspace dashboard — the grid of widgets at + * `//dashboards/`. + * + * Everything here is about the *colours* a chart widget draws, because that is + * the part of a chart a user reads without any text: two series painted the + * same colour is a chart that is silently wrong. Two independent renderings of + * the same resolved colour are exposed, and a spec should assert both — the + * legend swatch (what the user matches a name against) and the SVG series + * stroke (what the user actually traces with their eye). They come from the + * same chart config, so agreeing is not interesting; disagreeing would be. + * + * Colours are read as *computed* values and normalised to hex rather than read + * off the `stroke`/`--bg-color` attribute. The attribute holds a CSS variable + * (`var(--color-purple)`), so asserting on it would pin the variable name and + * pass happily if the variable itself were redefined to another colour. + * + * Selector note: neither the widget card nor the legend carries a + * `data-testid`, so widgets are addressed by their (spec-controlled) title and + * legend entries by their label. `.react-grid-item` and the recharts curve + * classes are library-owned class names. A `data-testid` on the widget card + * and on the legend colour indicator would let this drop the CSS entirely. + */ +export class DashboardPage { + constructor(private readonly page: Page) {} + + /** + * Opens a dashboard with its date range pinned on the URL. The range is + * otherwise remembered in localStorage, so leaving it unset would let one + * run's choice decide another run's chart. + */ + async goto(dashboardId: string, timeRange: string): Promise { + return test.step(`Open dashboard ${dashboardId}`, async () => { + const env = loadEnvConfig(); + await this.page.goto( + `${env.baseUrl}/${env.workspace}/dashboards/${dashboardId}?dashboard_time_range=${timeRange}`, + ); + }); + } + + /** + * Waits until a widget has drawn `seriesCount` series, and asserts it drew + * exactly that many. Charts render their axes before the data arrives, so + * waiting on the chart alone would let a spec read an empty legend. + */ + async waitForChart(widgetTitle: string, seriesCount: number): Promise { + return test.step(`Wait for "${widgetTitle}" to draw ${seriesCount} series`, async () => { + const widget = this.widget(widgetTitle); + await expect(widget).toHaveCount(1); + await expect(widget.locator('.recharts-surface')).toBeVisible(); + await expect(this.legendSwatches(widgetTitle)).toHaveCount(seriesCount); + await expect(this.seriesCurves(widgetTitle)).toHaveCount(seriesCount); + }); + } + + /** The card of the widget whose header reads exactly `title`. */ + widget(title: string): Locator { + return this.page + .locator('.react-grid-item') + .filter({ has: this.page.getByText(title, { exact: true }) }); + } + + /** + * The colour of a legend entry's swatch, as `#rrggbb`. + * + * The entry is addressed by its label, anchored and exact: a substring match + * on `cost` would also match a group named `cost-usd`. + */ + async legendColor(widgetTitle: string, label: string): Promise { + return test.step(`Read the "${label}" legend colour of "${widgetTitle}"`, async () => { + const swatch = this.legendItem(widgetTitle, label).locator(LEGEND_SWATCH); + await expect(swatch).toHaveCount(1); + return toHex(await swatch.evaluate((el) => getComputedStyle(el).backgroundColor)); + }); + } + + /** + * The stroke colour of every series line the widget drew, as `#rrggbb`, in + * DOM order. + * + * Deliberately the whole set rather than a lookup per series: recharts does + * not label a rendered curve with the series it belongs to, and the property + * this is here to protect is a property of the set — that no two series share + * a colour. A per-series read would not be able to see a collision at all. + */ + async seriesColors(widgetTitle: string): Promise { + return test.step(`Read the series colours of "${widgetTitle}"`, async () => { + const strokes = await this.seriesCurves(widgetTitle).evaluateAll((els) => + els.map((el) => getComputedStyle(el).stroke), + ); + return strokes.map(toHex); + }); + } + + private legendItem(widgetTitle: string, label: string): Locator { + return this.widget(widgetTitle) + .locator(`div:has(> ${LEGEND_SWATCH})`) + .filter({ hasText: new RegExp(`^\\s*${escapeRegExp(label)}\\s*$`) }); + } + + private legendSwatches(widgetTitle: string): Locator { + return this.widget(widgetTitle).locator(LEGEND_SWATCH); + } + + /** + * A single-series chart is drawn as a filled area and a multi-series one as + * lines, so both curve classes count as "a series". + */ + private seriesCurves(widgetTitle: string): Locator { + return this.widget(widgetTitle).locator( + '.recharts-line-curve, .recharts-area-curve', + ); + } +} + +/** + * The chart legend's colour dot. It carries no testid and no text; what marks + * it out is the inline custom property the resolved series colour is written + * to, which is also what its background renders from. + */ +const LEGEND_SWATCH = 'div[style*="--bg-color"]'; + +/** `rgb(16, 185, 129)` -> `#10b981`. Anything else is returned unchanged. */ +function toHex(color: string): string { + const match = color.match(/^rgba?\((\d+),\s*(\d+),\s*(\d+)/); + if (!match) return color; + return `#${match + .slice(1, 4) + .map((v) => Number(v).toString(16).padStart(2, '0')) + .join('')}`; +} + +function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} diff --git a/tests_end_to_end/e2e/tests/dashboards/chart-series-colors.spec.ts b/tests_end_to_end/e2e/tests/dashboards/chart-series-colors.spec.ts new file mode 100644 index 00000000000..b8b668e57b4 --- /dev/null +++ b/tests_end_to_end/e2e/tests/dashboards/chart-series-colors.spec.ts @@ -0,0 +1,159 @@ +import { + test, + expect, + SERIES_COLORS_TIME_RANGE, + SERIES_COLORS_WIDGETS, +} from '@e2e/fixtures'; +import { DashboardPage } from '@e2e/pom/dashboard.page'; + +/** + * How a project-metrics chart resolves the colour of each series. + * + * There are two sources, and which one applies depends on whether the widget + * is grouped: + * + * - **Ungrouped**, the series are the metric's own sub-series (`traces`, + * `duration.p50`, …), and each has a fixed colour so the same metric looks + * the same everywhere in the product. + * - **Grouped**, the series are *group values* — a tag, a trace name — and + * their colours come from the tag palette, `hash(label) % palette.length`, + * the same way a tag chip is coloured elsewhere. + * + * Applying the fixed map to a grouped chart is the bug these tests exist to + * catch: a group whose value happens to equal a metric key (`traces`, `cost`) + * takes that metric's colour, so several unrelated groups end up painted + * identically. Nothing errors and no value is wrong — the chart is simply + * unreadable, which is why it needs a test rather than a bug report. + * + * Both directions are asserted, because the guard can fail either way: not + * applying it where it belongs would silently drop the fixed colours from + * every ungrouped chart in the product. + * + * The expected colours are the product's palette, pinned as literal hex. That + * is the point: a spec that recomputed the hash from the palette constants + * would agree with any palette, including a broken one. + */ +const PALETTE = { + gray: '#64748b', + purple: '#8b5cf6', + purpleDark: '#491b7e', + burgundy: '#bf399e', + green: '#10b981', + turquoise: '#06b6d4', +} as const; + +/** Where each seeded tag lands in the tag palette, by its own hash. */ +const EXPECTED_GROUP_COLORS: Record = { + traces: PALETTE.green, + cost: PALETTE.purpleDark, + beta: PALETTE.purple, + zeta: PALETTE.turquoise, + iota: PALETTE.gray, +}; + +/** The fixed colours an *ungrouped* chart must keep using. */ +const METRIC_COLORS = { + traces: PALETTE.purple, + 'duration.p50': PALETTE.turquoise, + 'duration.p90': PALETTE.burgundy, + 'duration.p99': PALETTE.purple, +} as const; + +test.describe('Dashboards — chart series colours', { tag: ['@area:dashboards'] }, () => { + // Widgets are laid out on a grid; a narrow viewport stacks them and shrinks + // each chart until recharts drops the legend. + test.use({ viewport: { width: 1600, height: 1200 } }); + + test( + 'A chart grouped by tags colours every group from the tag palette, never from the fixed metric colours', + { tag: ['@t2-cuj', '@cap:dashboards.configure-widget'] }, + async ({ seriesColorsDashboard, page }) => { + const dashboard = new DashboardPage(page); + const { groupedByTag } = SERIES_COLORS_WIDGETS; + const groups = seriesColorsDashboard.groups; + + await test.step('Open the dashboard and wait for the grouped chart', async () => { + await dashboard.goto(seriesColorsDashboard.id, SERIES_COLORS_TIME_RANGE); + await dashboard.waitForChart(groupedByTag, groups.length); + }); + + await test.step('Each group takes the palette colour its own label hashes to', async () => { + for (const group of groups) { + expect(await dashboard.legendColor(groupedByTag, group), group).toBe( + EXPECTED_GROUP_COLORS[group], + ); + } + }); + + await test.step('The drawn lines carry those same colours, and no two share one', async () => { + const drawn = await dashboard.seriesColors(groupedByTag); + // Compared as sets: recharts does not say which curve is which series, + // and it is the set that has to be right — as many distinct colours as + // there are groups, and exactly the ones the legend advertises. + expect(drawn).toHaveLength(groups.length); + expect(new Set(drawn).size).toBe(groups.length); + expect([...drawn].sort()).toEqual( + groups.map((g) => EXPECTED_GROUP_COLORS[g]).sort(), + ); + }); + + await test.step('The groups named after metric keys are not painted the metric colour', async () => { + // The regression itself: with the fixed map applied, `traces` and + // `cost` would both be METRIC_COLORS.traces, and so would `beta`, + // which hashes there — three identical lines in a five-line chart. + expect(await dashboard.legendColor(groupedByTag, 'traces')).not.toBe( + METRIC_COLORS.traces, + ); + expect(await dashboard.legendColor(groupedByTag, 'cost')).not.toBe( + METRIC_COLORS.traces, + ); + }); + }, + ); + + test( + 'A chart with no grouping keeps the fixed metric colours', + { tag: ['@t2-cuj', '@cap:dashboards.configure-widget'] }, + async ({ seriesColorsDashboard, page }) => { + const dashboard = new DashboardPage(page); + const { ungrouped, duration } = SERIES_COLORS_WIDGETS; + + await test.step('Open the dashboard and wait for both ungrouped charts', async () => { + await dashboard.goto(seriesColorsDashboard.id, SERIES_COLORS_TIME_RANGE); + await dashboard.waitForChart(ungrouped, 1); + await dashboard.waitForChart(duration, 3); + }); + + await test.step('The trace count series keeps its fixed colour, not its hashed one', async () => { + expect(await dashboard.legendColor(ungrouped, 'traces')).toBe(METRIC_COLORS.traces); + expect(await dashboard.seriesColors(ungrouped)).toEqual([METRIC_COLORS.traces]); + // `traces` is seeded as a tag too, so its hashed colour is a real + // alternative here: seeing it would mean the fixed map had been + // dropped from ungrouped charts as well as from grouped ones. + expect(EXPECTED_GROUP_COLORS.traces).not.toBe(METRIC_COLORS.traces); + expect(await dashboard.legendColor(ungrouped, 'traces')).not.toBe( + EXPECTED_GROUP_COLORS.traces, + ); + }); + + await test.step('Each duration percentile keeps its own fixed colour', async () => { + for (const [series, color] of [ + ['duration.p50', METRIC_COLORS['duration.p50']], + ['duration.p90', METRIC_COLORS['duration.p90']], + ['duration.p99', METRIC_COLORS['duration.p99']], + ] as const) { + expect(await dashboard.legendColor(duration, series), series).toBe(color); + } + + const drawn = await dashboard.seriesColors(duration); + expect([...drawn].sort()).toEqual( + [ + METRIC_COLORS['duration.p50'], + METRIC_COLORS['duration.p90'], + METRIC_COLORS['duration.p99'], + ].sort(), + ); + }); + }, + ); +});