From 6205236f255012943fb10edc3091e0fda3aab0c0 Mon Sep 17 00:00:00 2001 From: timo Date: Tue, 28 May 2024 15:34:35 +0200 Subject: [PATCH] NodeGraph: Use values from fixedX/fixedY column for layout (#86643) * NodeGraph: Use values from fixedX/fixedY column for layout * Cleanup some code, typings and naming * Add test --------- Co-authored-by: Andrej Ocenas --- .betterer.results | 6 +-- packages/grafana-data/src/utils/nodeGraph.ts | 6 +++ public/app/plugins/panel/nodeGraph/Edge.tsx | 6 +-- .../app/plugins/panel/nodeGraph/EdgeLabel.tsx | 11 ++--- .../app/plugins/panel/nodeGraph/NodeGraph.tsx | 11 ++--- public/app/plugins/panel/nodeGraph/layout.ts | 26 +++++++++-- public/app/plugins/panel/nodeGraph/types.ts | 2 +- .../panel/nodeGraph/useContextMenu.tsx | 14 +++--- .../app/plugins/panel/nodeGraph/utils.test.ts | 45 ++++++++++++++++++- public/app/plugins/panel/nodeGraph/utils.ts | 28 ++++++++++++ 10 files changed, 122 insertions(+), 33 deletions(-) diff --git a/.betterer.results b/.betterer.results index 04b2b218d76e6..648ea10524a39 100644 --- a/.betterer.results +++ b/.betterer.results @@ -4624,8 +4624,7 @@ exports[`better eslint`] = { "public/app/plugins/panel/nodeGraph/EdgeLabel.tsx:5381": [ [0, 0, 0, "Styles should be written using objects.", "0"], [0, 0, 0, "Styles should be written using objects.", "1"], - [0, 0, 0, "Styles should be written using objects.", "2"], - [0, 0, 0, "Do not use any type assertions.", "3"] + [0, 0, 0, "Styles should be written using objects.", "2"] ], "public/app/plugins/panel/nodeGraph/Legend.tsx:5381": [ [0, 0, 0, "Styles should be written using objects.", "0"], @@ -4676,8 +4675,7 @@ exports[`better eslint`] = { [0, 0, 0, "Do not re-export imported variable (\`./NodeGraph\`)", "0"] ], "public/app/plugins/panel/nodeGraph/layout.ts:5381": [ - [0, 0, 0, "Do not use any type assertions.", "0"], - [0, 0, 0, "Do not use any type assertions.", "1"] + [0, 0, 0, "Do not use any type assertions.", "0"] ], "public/app/plugins/panel/nodeGraph/types.ts:5381": [ [0, 0, 0, "Do not re-export imported variable (\`./panelcfg.gen\`)", "0"] diff --git a/packages/grafana-data/src/utils/nodeGraph.ts b/packages/grafana-data/src/utils/nodeGraph.ts index 4cb05d054c3cf..e986804e89a4e 100644 --- a/packages/grafana-data/src/utils/nodeGraph.ts +++ b/packages/grafana-data/src/utils/nodeGraph.ts @@ -38,4 +38,10 @@ export enum NodeGraphDataFrameFieldNames { // Defines the stroke dash array for the edge [edges]. See SVG strokeDasharray definition for syntax. strokeDasharray = 'strokedasharray', + + // Supplies a fixed X position for the node to have in the finished graph. + fixedX = 'fixedx', + + // Supplies a fixed Y position for the node to have in the finished graph. + fixedY = 'fixedy', } diff --git a/public/app/plugins/panel/nodeGraph/Edge.tsx b/public/app/plugins/panel/nodeGraph/Edge.tsx index b80a22d71d7bc..f7ecf818e8663 100644 --- a/public/app/plugins/panel/nodeGraph/Edge.tsx +++ b/public/app/plugins/panel/nodeGraph/Edge.tsx @@ -2,17 +2,17 @@ import React, { MouseEvent, memo } from 'react'; import { EdgeArrowMarker } from './EdgeArrowMarker'; import { computeNodeCircumferenceStrokeWidth, nodeR } from './Node'; -import { EdgeDatum, NodeDatum } from './types'; +import { EdgeDatumLayout, NodeDatum } from './types'; import { shortenLine } from './utils'; export const defaultHighlightedEdgeColor = '#a00'; export const defaultEdgeColor = '#999'; interface Props { - edge: EdgeDatum; + edge: EdgeDatumLayout; hovering: boolean; svgIdNamespace: string; - onClick: (event: MouseEvent, link: EdgeDatum) => void; + onClick: (event: MouseEvent, link: EdgeDatumLayout) => void; onMouseEnter: (id: string) => void; onMouseLeave: (id: string) => void; } diff --git a/public/app/plugins/panel/nodeGraph/EdgeLabel.tsx b/public/app/plugins/panel/nodeGraph/EdgeLabel.tsx index c9c03a70beb9a..3c2bb5fc381ac 100644 --- a/public/app/plugins/panel/nodeGraph/EdgeLabel.tsx +++ b/public/app/plugins/panel/nodeGraph/EdgeLabel.tsx @@ -5,7 +5,7 @@ import { GrafanaTheme2 } from '@grafana/data'; import { useStyles2 } from '@grafana/ui'; import { nodeR } from './Node'; -import { EdgeDatum, NodeDatum } from './types'; +import { EdgeDatumLayout } from './types'; import { shortenLine } from './utils'; const getStyles = (theme: GrafanaTheme2) => { @@ -26,17 +26,12 @@ const getStyles = (theme: GrafanaTheme2) => { }; interface Props { - edge: EdgeDatum; + edge: EdgeDatumLayout; } export const EdgeLabel = memo(function EdgeLabel(props: Props) { const { edge } = props; // Not great typing, but after we do layout these properties are full objects not just references - const { source, target, sourceNodeRadius, targetNodeRadius } = edge as { - source: NodeDatum; - target: NodeDatum; - sourceNodeRadius: number; - targetNodeRadius: number; - }; + const { source, target, sourceNodeRadius, targetNodeRadius } = edge; // As the nodes have some radius we want edges to end outside the node circle. const line = shortenLine( diff --git a/public/app/plugins/panel/nodeGraph/NodeGraph.tsx b/public/app/plugins/panel/nodeGraph/NodeGraph.tsx index d49e9edb8ebc9..e9fe37dda656f 100644 --- a/public/app/plugins/panel/nodeGraph/NodeGraph.tsx +++ b/public/app/plugins/panel/nodeGraph/NodeGraph.tsx @@ -13,7 +13,7 @@ import { Marker } from './Marker'; import { Node } from './Node'; import { ViewControls } from './ViewControls'; import { Config, defaultConfig, useLayout } from './layout'; -import { EdgeDatum, NodeDatum, NodesMarker } from './types'; +import { EdgeDatumLayout, NodeDatum, NodesMarker } from './types'; import { useCategorizeFrames } from './useCategorizeFrames'; import { useContextMenu } from './useContextMenu'; import { useFocusPositionOnLayout } from './useFocusPositionOnLayout'; @@ -164,7 +164,8 @@ export function NodeGraph({ getLinks, dataFrames, nodeLimit, panelId }: Props) { config, nodeCountLimit, width, - focusedNodeId + focusedNodeId, + processed.hasFixedPositions ); // If we move from grid to graph layout, and we have focused node lets get its position to center there. We want to @@ -338,11 +339,11 @@ const Markers = memo(function Nodes(props: MarkersProps) { }); interface EdgesProps { - edges: EdgeDatum[]; + edges: EdgeDatumLayout[]; nodeHoveringId?: string; edgeHoveringId?: string; svgIdNamespace: string; - onClick: (event: MouseEvent, link: EdgeDatum) => void; + onClick: (event: MouseEvent, link: EdgeDatumLayout) => void; onMouseEnter: (id: string) => void; onMouseLeave: (id: string) => void; } @@ -369,7 +370,7 @@ const Edges = memo(function Edges(props: EdgesProps) { }); interface EdgeLabelsProps { - edges: EdgeDatum[]; + edges: EdgeDatumLayout[]; nodeHoveringId?: string; edgeHoveringId?: string; } diff --git a/public/app/plugins/panel/nodeGraph/layout.ts b/public/app/plugins/panel/nodeGraph/layout.ts index 97ccead786d3b..2a0db90cdbb33 100644 --- a/public/app/plugins/panel/nodeGraph/layout.ts +++ b/public/app/plugins/panel/nodeGraph/layout.ts @@ -48,7 +48,8 @@ export function useLayout( config: Config = defaultConfig, nodeCountLimit: number, width: number, - rootNodeId?: string + rootNodeId?: string, + hasFixedPositions?: boolean ) { const [nodesGraph, setNodesGraph] = useState([]); const [edgesGraph, setEdgesGraph] = useState([]); @@ -84,19 +85,36 @@ export function useLayout( return; } + if (hasFixedPositions) { + setNodesGraph(rawNodes); + // The layout function turns source and target fields from string to NodeDatum, so we do that here as well. + const nodesMap = fromPairs(rawNodes.map((node) => [node.id, node])); + setEdgesGraph( + rawEdges.map( + (e): EdgeDatumLayout => ({ + ...e, + source: nodesMap[e.source], + target: nodesMap[e.target], + }) + ) + ); + setLoading(false); + return; + } + setLoading(true); // This is async but as I wanted to still run the sync grid layout, and you cannot return promise from effect so // having callback seems ok here. const cancel = layout(rawNodes, rawEdges, ({ nodes, edges }) => { if (isMounted()) { setNodesGraph(nodes); - setEdgesGraph(edges as EdgeDatumLayout[]); + setEdgesGraph(edges); setLoading(false); } }); layoutWorkerCancelRef.current = cancel; return cancel; - }, [rawNodes, rawEdges, isMounted]); + }, [hasFixedPositions, rawNodes, rawEdges, isMounted]); // Compute grid separately as it is sync and do not need to be inside effect. Also it is dependant on width while // default layout does not care and we don't want to recalculate that on panel resize. @@ -149,7 +167,7 @@ export function useLayout( function layout( nodes: NodeDatum[], edges: EdgeDatum[], - done: (data: { nodes: NodeDatum[]; edges: EdgeDatum[] }) => void + done: (data: { nodes: NodeDatum[]; edges: EdgeDatumLayout[] }) => void ) { // const worker = engine === 'default' ? createWorker() : createMsaglWorker(); // TODO: temp fix because of problem with msagl library https://github.com/grafana/grafana/issues/83318 diff --git a/public/app/plugins/panel/nodeGraph/types.ts b/public/app/plugins/panel/nodeGraph/types.ts index 1c93bea22e287..39412bbde6396 100644 --- a/public/app/plugins/panel/nodeGraph/types.ts +++ b/public/app/plugins/panel/nodeGraph/types.ts @@ -45,7 +45,7 @@ export type EdgeDatum = LinkDatum & { }; // After layout is run D3 will change the string IDs for actual references to the nodes. -export type EdgeDatumLayout = EdgeDatum & { +export type EdgeDatumLayout = Omit & { source: NodeDatum; target: NodeDatum; }; diff --git a/public/app/plugins/panel/nodeGraph/useContextMenu.tsx b/public/app/plugins/panel/nodeGraph/useContextMenu.tsx index 6e9b6b4db1c04..31ec9c96d671e 100644 --- a/public/app/plugins/panel/nodeGraph/useContextMenu.tsx +++ b/public/app/plugins/panel/nodeGraph/useContextMenu.tsx @@ -5,7 +5,7 @@ import { DataFrame, Field, GrafanaTheme2, LinkModel } from '@grafana/data'; import { ContextMenu, MenuGroup, MenuItem, useStyles2 } from '@grafana/ui'; import { Config } from './layout'; -import { EdgeDatum, NodeDatum } from './types'; +import { EdgeDatumLayout, NodeDatum } from './types'; import { getEdgeFields, getNodeFields, statToString } from './utils'; /** @@ -22,7 +22,7 @@ export function useContextMenu( setConfig: (config: Config) => void, setFocusedNodeId: (id: string) => void ): { - onEdgeOpen: (event: MouseEvent, edge: EdgeDatum) => void; + onEdgeOpen: (event: MouseEvent, edge: EdgeDatumLayout) => void; onNodeOpen: (event: MouseEvent, node: NodeDatum) => void; MenuComponent: React.ReactNode; } { @@ -53,7 +53,7 @@ export function useContextMenu( ); const onEdgeOpen = useCallback( - (event: MouseEvent, edge: EdgeDatum) => { + (event: MouseEvent, edge: EdgeDatumLayout) => { if (!edges) { // This could happen if we have only one node and no edges, in which case this is not needed as there is no edge // to click on. @@ -86,7 +86,7 @@ function makeContextMenu( ); } -function getItemsRenderer( +function getItemsRenderer( links: LinkModel[], item: T, extraItems?: Array> | undefined @@ -109,7 +109,7 @@ function getItemsRenderer( }; } -function mapMenuItem(item: T) { +function mapMenuItem(item: T) { return function NodeGraphMenuItem(link: LinkData) { return ( (item: T) { }; } -type LinkData = { +type LinkData = { label: string; ariaLabel?: string; url?: string; @@ -224,7 +224,7 @@ function NodeHeader({ node, nodes }: { node: NodeDatum; nodes?: DataFrame }) { /** * Shows some of the field values in a table on top of the context menu. */ -function EdgeHeader(props: { edge: EdgeDatum; edges: DataFrame }) { +function EdgeHeader(props: { edge: EdgeDatumLayout; edges: DataFrame }) { const index = props.edge.dataFrameRowIndex; const fields = getEdgeFields(props.edges); const valueSource = fields.source?.values[index] || ''; diff --git a/public/app/plugins/panel/nodeGraph/utils.test.ts b/public/app/plugins/panel/nodeGraph/utils.test.ts index 14a9ad63d37f1..3c563cbddd66d 100644 --- a/public/app/plugins/panel/nodeGraph/utils.test.ts +++ b/public/app/plugins/panel/nodeGraph/utils.test.ts @@ -1,4 +1,4 @@ -import { DataFrame, FieldType, createDataFrame } from '@grafana/data'; +import { DataFrame, FieldType, createDataFrame, NodeGraphDataFrameFieldNames } from '@grafana/data'; import { NodeDatum, NodeGraphOptions } from './types'; import { @@ -224,6 +224,49 @@ describe('processNodes', () => { expect(edgesFrame?.fields.find((f) => f.name === 'mainStat')?.config).toEqual({ unit: 'r/sec' }); expect(edgesFrame?.fields.find((f) => f.name === 'secondaryStat')?.config).toEqual({ unit: 'ft^2' }); }); + + it('processes nodes with fixedX/Y', async () => { + const nodesFrame = makeNodesDataFrame(3); + nodesFrame.fields.push({ + name: NodeGraphDataFrameFieldNames.fixedX, + type: FieldType.number, + values: [1, 2, 3], + config: {}, + }); + + nodesFrame.fields.push({ + name: NodeGraphDataFrameFieldNames.fixedY, + type: FieldType.number, + values: [1, 2, 3], + config: {}, + }); + const result = processNodes(nodesFrame, undefined); + expect(result.hasFixedPositions).toBe(true); + expect(result.nodes[0].x).toBe(1); + expect(result.nodes[0].y).toBe(1); + }); + + it('throws error if fixedX/Y is used incorrectly', async () => { + const nodesFrame = makeNodesDataFrame(3); + nodesFrame.fields.push({ + name: NodeGraphDataFrameFieldNames.fixedX, + type: FieldType.number, + values: [undefined, 2, 3], + config: {}, + }); + + expect(() => processNodes(nodesFrame, undefined)).toThrow(/fixedX/); + + // We still have one undefined value in fixedX field so this should still fail + nodesFrame.fields.push({ + name: NodeGraphDataFrameFieldNames.fixedY, + type: FieldType.number, + values: [1, 2, 3], + config: {}, + }); + + expect(() => processNodes(nodesFrame, undefined)).toThrow(/fixedX/); + }); }); describe('finds connections', () => { diff --git a/public/app/plugins/panel/nodeGraph/utils.ts b/public/app/plugins/panel/nodeGraph/utils.ts index cf8d6abb35111..bfeb54bd7cce4 100644 --- a/public/app/plugins/panel/nodeGraph/utils.ts +++ b/public/app/plugins/panel/nodeGraph/utils.ts @@ -45,6 +45,8 @@ export function shortenLine(line: Line, sourceNodeRadius: number, targetNodeRadi } export type NodeFields = { + fixedX?: Field; + fixedY?: Field; id?: Field; title?: Field; subTitle?: Field; @@ -76,6 +78,8 @@ export function getNodeFields(nodes: DataFrame): NodeFields { icon: fieldsCache.getFieldByName(NodeGraphDataFrameFieldNames.icon), nodeRadius: fieldsCache.getFieldByName(NodeGraphDataFrameFieldNames.nodeRadius.toLowerCase()), highlighted: fieldsCache.getFieldByName(NodeGraphDataFrameFieldNames.highlighted.toLowerCase()), + fixedX: fieldsCache.getFieldByName(NodeGraphDataFrameFieldNames.fixedX.toLowerCase()), + fixedY: fieldsCache.getFieldByName(NodeGraphDataFrameFieldNames.fixedY.toLowerCase()), }; } @@ -129,6 +133,7 @@ export function processNodes( ): { nodes: NodeDatum[]; edges: EdgeDatum[]; + hasFixedPositions?: boolean; legend?: Array<{ color: string; name: string; @@ -144,6 +149,24 @@ export function processNodes( throw new Error('id field is required for nodes data frame.'); } + const hasFixedPositions = + nodeFields.fixedX && + nodeFields.fixedX.values.every((v) => Number.isFinite(v)) && + nodeFields.fixedY && + nodeFields.fixedY.values.every((v) => Number.isFinite(v)); + + // Throw an error if somebody is using fixedX and fixedY fields incorrectly. Other option is to ignore this but we + // are not able to easily combine fixed and non-fixed position in layout so that behaviour would be undefined + // and silent. + if (!hasFixedPositions) { + const somePosFilled = + (nodeFields.fixedX && nodeFields.fixedX.values.some((v) => Number.isFinite(v))) || + (nodeFields.fixedY && nodeFields.fixedY.values.some((v) => Number.isFinite(v))); + if (somePosFilled) { + throw new Error('If fixedX and fixedY fields are present, the values have to be all filled and valid'); + } + } + // Create the nodes here const nodesMap: { [id: string]: NodeDatum } = {}; for (let i = 0; i < nodeFields.id.values.length; i++) { @@ -162,6 +185,7 @@ export function processNodes( return { nodes: Object.values(nodesMap), edges: edgeDatums, + hasFixedPositions, legend: nodeFields.arc.map((f) => { return { color: f.config.color?.fixedColor ?? '', @@ -210,6 +234,8 @@ export function processNodes( return { nodes, edges: edgeDatums, + // Edge-only datasets never have fixedX/fixedY + hasFixedPositions: false, }; } } @@ -336,6 +362,8 @@ function makeNodeDatum(id: string, nodeFields: NodeFields, index: number): NodeD icon: nodeFields.icon?.values[index] || '', nodeRadius: nodeFields.nodeRadius, highlighted: nodeFields.highlighted?.values[index] || false, + x: nodeFields.fixedX?.values[index] ?? undefined, + y: nodeFields.fixedY?.values[index] ?? undefined, }; }