From 4b519ead4f053c3dbe844cf6e269c026f9c5e8f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Mon, 19 Dec 2022 17:06:59 +0100 Subject: [PATCH] perf: Simplify queries to prometheus (#2706) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## About the changes This PR improves our queries to Prometheus (instead of making multiple queries do only one) and improves the UI and the code. The reports aggregate all HTTP methods (GET, POST, PUT, DELETE, OPTIONS, HEAD and PATCH) without distinction under the same "endpoint" (a relative path inside unleash up to a certain depth) Co-authored-by: Nuno Góis --- .../NetworkOverview/NetworkOverview.tsx | 51 ++-- .../network/NetworkTraffic/NetworkTraffic.tsx | 227 +++++++++--------- .../src/component/common/Mermaid/Mermaid.tsx | 21 +- .../useInstanceMetrics/useInstanceMetrics.ts | 6 +- ...stsPerSecondSchemaDataResultInnerMetric.ts | 6 + frontend/src/utils/unknownify.ts | 2 + .../spec/requests-per-second-schema.ts | 3 + src/lib/routes/admin-api/metrics.test.ts | 7 +- src/lib/routes/admin-api/metrics.ts | 22 +- .../client-metrics/instance-service.ts | 11 +- .../__snapshots__/openapi.e2e.test.ts.snap | 3 + 11 files changed, 180 insertions(+), 179 deletions(-) create mode 100644 frontend/src/utils/unknownify.ts diff --git a/frontend/src/component/admin/network/NetworkOverview/NetworkOverview.tsx b/frontend/src/component/admin/network/NetworkOverview/NetworkOverview.tsx index d655567abb..96de9d7657 100644 --- a/frontend/src/component/admin/network/NetworkOverview/NetworkOverview.tsx +++ b/frontend/src/component/admin/network/NetworkOverview/NetworkOverview.tsx @@ -3,6 +3,7 @@ import { Mermaid } from 'component/common/Mermaid/Mermaid'; import { useInstanceMetrics } from 'hooks/api/getters/useInstanceMetrics/useInstanceMetrics'; import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; import { Alert, styled } from '@mui/material'; +import { unknownify } from 'utils/unknownify'; const StyledMermaid = styled(Mermaid)(({ theme }) => ({ '#mermaid .node rect': { @@ -11,6 +12,13 @@ const StyledMermaid = styled(Mermaid)(({ theme }) => ({ }, })); +const isRecent = (value: ResultValue) => { + const threshold = 60000; // ten minutes + return value[0] * 1000 > new Date().getTime() - threshold; +}; + +type ResultValue = [number, string]; + interface INetworkApp { label?: string; reqs: string; @@ -20,26 +28,33 @@ interface INetworkApp { export const NetworkOverview = () => { usePageTitle('Network - Overview'); const { metrics } = useInstanceMetrics(); + const results = metrics?.data?.result; const apps: INetworkApp[] = []; - if (Boolean(metrics)) { - Object.keys(metrics).forEach(metric => { - apps.push( - ...( - metrics[metric].data?.result - ?.map(result => ({ - label: result.metric?.appName, - reqs: parseFloat( - result.values?.[ - result.values?.length - 1 - ][1].toString() || '0' - ).toFixed(2), - type: metric.split('Metrics')[0], - })) - .filter(app => app.label !== 'undefined') || [] - ).filter(app => app.reqs !== '0.00') - ); - }); + + if (results) { + apps.push( + ...( + results + ?.map(result => { + const values = (result.values || []) as ResultValue[]; + const data = + values.filter(value => isRecent(value)) || []; + let reqs = 0; + if (data.length) { + reqs = parseFloat(data[data.length - 1][1]); + } + return { + label: unknownify(result.metric?.appName), + reqs: reqs.toFixed(2), + type: unknownify( + result.metric?.endpoint?.split('/')[2] + ), + }; + }) + .filter(app => app.label !== 'unknown') || [] + ).filter(app => app.reqs !== '0.00') + ); } const graph = ` diff --git a/frontend/src/component/admin/network/NetworkTraffic/NetworkTraffic.tsx b/frontend/src/component/admin/network/NetworkTraffic/NetworkTraffic.tsx index 3712e340a8..51fca04982 100644 --- a/frontend/src/component/admin/network/NetworkTraffic/NetworkTraffic.tsx +++ b/frontend/src/component/admin/network/NetworkTraffic/NetworkTraffic.tsx @@ -1,13 +1,9 @@ -import { - InstanceMetrics, - useInstanceMetrics, -} from 'hooks/api/getters/useInstanceMetrics/useInstanceMetrics'; +import { useInstanceMetrics } from 'hooks/api/getters/useInstanceMetrics/useInstanceMetrics'; import { useMemo, VFC } from 'react'; import { Line } from 'react-chartjs-2'; import { CategoryScale, Chart as ChartJS, - ChartData, ChartDataset, ChartOptions, Legend, @@ -26,11 +22,12 @@ import theme from 'themes/theme'; import { formatDateHM } from 'utils/formatDate'; import { RequestsPerSecondSchema } from 'openapi'; import 'chartjs-adapter-date-fns'; -import { Alert, PaletteColor } from '@mui/material'; +import { Alert } from '@mui/material'; import { Box } from '@mui/system'; import { CyclicIterator } from 'utils/cyclicIterator'; import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; import { usePageTitle } from 'hooks/usePageTitle'; +import { unknownify } from 'utils/unknownify'; interface IPoint { x: number; @@ -38,150 +35,150 @@ interface IPoint { } type ChartDatasetType = ChartDataset<'line', IPoint[]>; -type ChartDataType = ChartData<'line', IPoint[], string>; + +type ResultValue = [number, string]; const createChartPoints = ( - values: Array>, + values: ResultValue[], y: (m: string) => number ): IPoint[] => { return values.map(row => ({ - x: row[0] as number, - y: y(row[1] as string), + x: row[0], + y: y(row[1]), })); }; + const createInstanceChartOptions = ( - metrics: InstanceMetrics, locationSettings: ILocationSettings -): ChartOptions<'line'> => { - return { - locale: locationSettings.locale, - responsive: true, - maintainAspectRatio: false, - interaction: { - mode: 'index', - intersect: false, +): ChartOptions<'line'> => ({ + locale: locationSettings.locale, + responsive: true, + maintainAspectRatio: false, + interaction: { + mode: 'index', + intersect: false, + }, + plugins: { + tooltip: { + backgroundColor: theme.palette.background.paper, + bodyColor: theme.palette.text.primary, + titleColor: theme.palette.grey[700], + borderColor: theme.palette.primary.main, + borderWidth: 1, + padding: 10, + boxPadding: 5, + usePointStyle: true, + callbacks: { + title: items => + formatDateHM( + 1000 * items[0].parsed.x, + locationSettings.locale + ), + }, + itemSort: (a, b) => b.parsed.y - a.parsed.y, }, - plugins: { - tooltip: { - backgroundColor: 'white', - bodyColor: theme.palette.text.primary, - titleColor: theme.palette.grey[700], - borderColor: theme.palette.primary.main, - borderWidth: 1, - padding: 10, - boxPadding: 5, + legend: { + position: 'top', + align: 'end', + labels: { + boxWidth: 10, + boxHeight: 10, usePointStyle: true, - callbacks: { - title: items => - formatDateHM( - 1000 * items[0].parsed.x, - locationSettings.locale - ), - }, - itemSort: (a, b) => b.parsed.y - a.parsed.y, }, - legend: { - position: 'top', - align: 'end', - labels: { - boxWidth: 10, - boxHeight: 10, - usePointStyle: true, - }, + }, + title: { + text: 'Requests per second in the last 6 hours', + position: 'top', + align: 'start', + display: true, + font: { + size: 16, + weight: '400', }, + }, + }, + scales: { + y: { + type: 'linear', title: { - text: 'Requests per second in the last 6 hours', - position: 'top', - align: 'start', display: true, - font: { - size: 16, - weight: '400', - }, + text: 'Requests per second', + }, + // min: 0, + suggestedMin: 0, + ticks: { precision: 0 }, + }, + x: { + type: 'time', + time: { unit: 'minute' }, + grid: { display: false }, + ticks: { + callback: (_, i, data) => + formatDateHM(data[i].value, locationSettings.locale), }, }, - scales: { - y: { - type: 'linear', - title: { - display: true, - text: 'Requests per second', - }, - // min: 0, - suggestedMin: 0, - ticks: { precision: 0 }, - }, - x: { - type: 'time', - time: { unit: 'minute' }, - grid: { display: false }, - ticks: { - callback: (_, i, data) => - formatDateHM(data[i].value, locationSettings.locale), - }, - }, - }, - }; -}; + }, +}); -const toChartData = ( - rps: RequestsPerSecondSchema, - color: PaletteColor, - label: (name: string) => string -): ChartDatasetType[] => { - if (rps.data?.result) { - return rps.data.result.map(dataset => ({ - label: label(dataset.metric?.appName || 'unknown'), - borderColor: color.main, - backgroundColor: color.main, - data: createChartPoints(dataset.values || [], y => parseFloat(y)), - elements: { - point: { - radius: 4, - pointStyle: 'circle', - }, - line: { - borderDash: [8, 4], - }, - }, - })); +class ItemPicker { + private items: CyclicIterator; + private picked: Map = new Map(); + constructor(items: T[]) { + this.items = new CyclicIterator(items); } - return []; -}; -const createInstanceChartData = (metrics?: InstanceMetrics): ChartDataType => { - if (metrics) { - const colors = new CyclicIterator([ + public pick(key: string): T { + if (!this.picked.has(key)) { + this.picked.set(key, this.items.next()); + } + return this.picked.get(key)!; + } +} + +const toChartData = (rps?: RequestsPerSecondSchema): ChartDatasetType[] => { + if (rps?.data?.result) { + const colorPicker = new ItemPicker([ theme.palette.success, theme.palette.error, theme.palette.primary, + theme.palette.warning, ]); - let datasets: ChartDatasetType[] = []; - for (let key in metrics) { - datasets = datasets.concat( - toChartData( - metrics[key], - colors.next(), - metricName => `${metricName}: ${key}` - ) - ); - } - return { datasets }; + return rps.data.result.map(dataset => { + const endpoint = unknownify(dataset.metric?.endpoint); + const appName = unknownify(dataset.metric?.appName); + const color = colorPicker.pick(endpoint); + const values = (dataset.values || []) as ResultValue[]; + return { + label: `${endpoint}: ${appName}`, + borderColor: color.main, + backgroundColor: color.main, + data: createChartPoints(values, y => parseFloat(y)), + elements: { + point: { + radius: 4, + pointStyle: 'circle', + }, + line: { + borderDash: [8, 4], + }, + }, + }; + }); } - return { datasets: [] }; + return []; }; export const NetworkTraffic: VFC = () => { const { locationSettings } = useLocationSettings(); const { metrics } = useInstanceMetrics(); const options = useMemo(() => { - return createInstanceChartOptions(metrics, locationSettings); - }, [metrics, locationSettings]); + return createInstanceChartOptions(locationSettings); + }, [locationSettings]); usePageTitle('Network - Traffic'); const data = useMemo(() => { - return createInstanceChartData(metrics); + return { datasets: toChartData(metrics) }; }, [metrics, locationSettings]); return ( diff --git a/frontend/src/component/common/Mermaid/Mermaid.tsx b/frontend/src/component/common/Mermaid/Mermaid.tsx index 51a861c0a6..c156a2fa74 100644 --- a/frontend/src/component/common/Mermaid/Mermaid.tsx +++ b/frontend/src/component/common/Mermaid/Mermaid.tsx @@ -1,6 +1,6 @@ import { styled } from '@mui/material'; import mermaid from 'mermaid'; -import { useEffect } from 'react'; +import { useRef, useEffect } from 'react'; const StyledMermaid = styled('div')(({ theme }) => ({ display: 'flex', @@ -11,6 +11,7 @@ const StyledMermaid = styled('div')(({ theme }) => ({ })); mermaid.initialize({ + startOnLoad: false, theme: 'default', themeCSS: ` .clusters #_ rect { @@ -21,23 +22,19 @@ mermaid.initialize({ }); interface IMermaidProps { - className?: string; children: string; } -export const Mermaid = ({ className = '', children }: IMermaidProps) => { +export const Mermaid = ({ children, ...props }: IMermaidProps) => { + const mermaidRef = useRef(null); + useEffect(() => { - mermaid.render('mermaid', children, (svgCode: string) => { - const mermaidDiv = document.querySelector('.mermaid'); - if (mermaidDiv) { - mermaidDiv.innerHTML = svgCode; + mermaid.render('mermaid', children, svgCode => { + if (mermaidRef.current) { + mermaidRef.current.innerHTML = svgCode; } }); }, [children]); - return ( - - {children} - - ); + return ; }; diff --git a/frontend/src/hooks/api/getters/useInstanceMetrics/useInstanceMetrics.ts b/frontend/src/hooks/api/getters/useInstanceMetrics/useInstanceMetrics.ts index 1f13d39248..873051a02a 100644 --- a/frontend/src/hooks/api/getters/useInstanceMetrics/useInstanceMetrics.ts +++ b/frontend/src/hooks/api/getters/useInstanceMetrics/useInstanceMetrics.ts @@ -4,12 +4,8 @@ import { formatApiPath } from 'utils/formatPath'; import handleErrorResponses from '../httpErrorResponseHandler'; import { RequestsPerSecondSchema } from 'openapi'; -export interface InstanceMetrics { - [key: string]: RequestsPerSecondSchema; -} - export interface IInstanceMetricsResponse { - metrics: InstanceMetrics; + metrics: RequestsPerSecondSchema; refetch: () => void; diff --git a/frontend/src/openapi/models/RequestsPerSecondSchemaDataResultInnerMetric.ts b/frontend/src/openapi/models/RequestsPerSecondSchemaDataResultInnerMetric.ts index 2c55d2b303..6c53655117 100644 --- a/frontend/src/openapi/models/RequestsPerSecondSchemaDataResultInnerMetric.ts +++ b/frontend/src/openapi/models/RequestsPerSecondSchemaDataResultInnerMetric.ts @@ -25,6 +25,12 @@ export interface RequestsPerSecondSchemaDataResultInnerMetric { * @memberof RequestsPerSecondSchemaDataResultInnerMetric */ appName?: string; + /** + * + * @type {string} + * @memberof RequestsPerSecondSchemaDataResultInnerMetric + */ + endpoint?: string; } /** diff --git a/frontend/src/utils/unknownify.ts b/frontend/src/utils/unknownify.ts new file mode 100644 index 0000000000..21bfc6b01d --- /dev/null +++ b/frontend/src/utils/unknownify.ts @@ -0,0 +1,2 @@ +export const unknownify = (value?: string) => + !value || value === 'undefined' ? 'unknown' : value; diff --git a/src/lib/openapi/spec/requests-per-second-schema.ts b/src/lib/openapi/spec/requests-per-second-schema.ts index 0c54f263bb..66ad039f92 100644 --- a/src/lib/openapi/spec/requests-per-second-schema.ts +++ b/src/lib/openapi/spec/requests-per-second-schema.ts @@ -28,6 +28,9 @@ export const requestsPerSecondSchema = { appName: { type: 'string', }, + endpoint: { + type: 'string', + }, }, }, values: { diff --git a/src/lib/routes/admin-api/metrics.test.ts b/src/lib/routes/admin-api/metrics.test.ts index 4a84027df3..91e52e1c73 100644 --- a/src/lib/routes/admin-api/metrics.test.ts +++ b/src/lib/routes/admin-api/metrics.test.ts @@ -12,10 +12,9 @@ async function getSetup() { preRouterHook: perms.hook, }); const services = createServices(stores, config); - jest.spyOn( - services.clientInstanceService, - 'getRPSForPath', - ).mockImplementation(async () => jest.fn()); + jest.spyOn(services.clientInstanceService, 'getRPS').mockImplementation( + async () => {}, + ); const app = await getApp(config, stores, services); return { diff --git a/src/lib/routes/admin-api/metrics.ts b/src/lib/routes/admin-api/metrics.ts index 7f0ae2fb9e..8de178078f 100644 --- a/src/lib/routes/admin-api/metrics.ts +++ b/src/lib/routes/admin-api/metrics.ts @@ -187,26 +187,8 @@ class MetricsController extends Controller { } try { const hoursToQuery = 6; - const [clientMetrics, frontendMetrics, adminMetrics] = - await Promise.all([ - this.clientInstanceService.getRPSForPath( - '/api/client/.*', - hoursToQuery, - ), - this.clientInstanceService.getRPSForPath( - '/api/frontend.*', - hoursToQuery, - ), - this.clientInstanceService.getRPSForPath( - '/api/admin/.*', - hoursToQuery, - ), - ]); - res.json({ - clientMetrics, - frontendMetrics, - adminMetrics, - }); + const rps = await this.clientInstanceService.getRPS(hoursToQuery); + res.json(rps || {}); } catch (err) { this.logger.error('Failed to fetch RPS metrics', err); res.status(500).send('Error fetching RPS metrics'); diff --git a/src/lib/services/client-metrics/instance-service.ts b/src/lib/services/client-metrics/instance-service.ts index 45092eb368..70200cc500 100644 --- a/src/lib/services/client-metrics/instance-service.ts +++ b/src/lib/services/client-metrics/instance-service.ts @@ -224,16 +224,17 @@ export default class ClientInstanceService { return (d.getTime() - d.getMilliseconds()) / 1000; } - async getRPSForPath(path: string, hoursToQuery: number): Promise { + async getRPS(hoursToQuery: number): Promise { if (!this.prometheusApi) { this.logger.warn('Prometheus not configured'); return; } const timeoutSeconds = 5; - const basePath = this.serverOption.baseUriPath; - const compositePath = `${basePath}/${path}`.replaceAll('//', '/'); - const step = '5m'; // validate: I'm using the step both for step in query_range and for irate - const query = `sum by(appName) (irate (http_request_duration_milliseconds_count{path=~"${compositePath}"} [${step}]))`; + const basePath = this.serverOption.baseUriPath.replace(/\/$/, ''); + const pathQuery = `${basePath}/api/.*`; + const step = '5m'; + const rpsQuery = `irate (http_request_duration_milliseconds_count{path=~"${pathQuery}"} [${step}])`; + const query = `sum by(appName, endpoint) (label_replace(${rpsQuery}, "endpoint", "$1", "path", "${basePath}(/api/(?:client/)?[^/\*]*).*"))`; const end = new Date(); const start = new Date(); start.setHours(end.getHours() - hoursToQuery); diff --git a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap b/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap index cb33cb1875..181558d6e6 100644 --- a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap +++ b/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap @@ -2739,6 +2739,9 @@ exports[`should serve the OpenAPI spec 1`] = ` "appName": { "type": "string", }, + "endpoint": { + "type": "string", + }, }, "type": "object", },