From 4afdb85224f6db05cfe033247cbec145bd777fc3 Mon Sep 17 00:00:00 2001 From: Ashu <11219262+ashutosh16@users.noreply.github.com> Date: Wed, 9 Oct 2024 11:32:09 -0700 Subject: [PATCH] fix(dashboard): analysis modal crashed when value not valid (#3881) * handle the parse issue Signed-off-by: ashutosh16 <11219262+ashutosh16@users.noreply.github.com> * handle the parse error Signed-off-by: ashutosh16 <11219262+ashutosh16@users.noreply.github.com> * handle the parse error Signed-off-by: ashutosh16 <11219262+ashutosh16@users.noreply.github.com> * handle the parse error Signed-off-by: ashutosh16 <11219262+ashutosh16@users.noreply.github.com> --------- Signed-off-by: ashutosh16 <11219262+ashutosh16@users.noreply.github.com> --- .../analysis-modal/transforms.test.ts | 72 +++++++++++++++++-- .../components/analysis-modal/transforms.ts | 27 ++++--- ui/src/app/components/pods/pods.tsx | 3 + 3 files changed, 87 insertions(+), 15 deletions(-) diff --git a/ui/src/app/components/analysis-modal/transforms.test.ts b/ui/src/app/components/analysis-modal/transforms.test.ts index 5c2c2a667e..60f7b37a7e 100644 --- a/ui/src/app/components/analysis-modal/transforms.test.ts +++ b/ui/src/app/components/analysis-modal/transforms.test.ts @@ -5,6 +5,7 @@ import { GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1CloudWatchMetric, GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1MetricProvider, GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1MetricResult, + GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1Measurement, } from '../../../models/rollout/generated'; import { analysisEndTime, @@ -25,6 +26,7 @@ import { metricSubstatus, printableCloudWatchQuery, printableDatadogQuery, + transformMeasurements, } from './transforms'; import {AnalysisStatus, FunctionalStatus} from './types'; @@ -376,12 +378,12 @@ describe('analysis modal transforms', () => { }); test('interpolateQuery() for prometheus query and args', () => { expect(interpolateQuery(MOCK_QUERY_PROMETHEUS, MOCK_ARGS_PROMETHEUS)).toBe( - 'sum(irate(istio_requests_total{reporter="source",destination_service=~"istio-host-split-canary",response_code!~"5.*"}[5m])) / sum(irate(istio_requests_total{reporter="source",destination_service=~"istio-host-split-canary"}[5m]))' + 'sum(irate(istio_requests_total{reporter="source",destination_service=~"istio-host-split-canary",response_code!~"5.*"}[5m])) / sum(irate(istio_requests_total{reporter="source",destination_service=~"istio-host-split-canary"}[5m]))', ); }); test('interpolateQuery() for newrelic query and args', () => { expect(interpolateQuery(MOCK_QUERY_NEWRELIC, MOCK_ARGS_NEWRELIC)).toBe( - "FROM Transaction SELECT percentage(count(*), WHERE httpResponseCode != 500) as successRate where appName = 'myApp'" + "FROM Transaction SELECT percentage(count(*), WHERE httpResponseCode != 500) as successRate where appName = 'myApp'", ); }); test('interpolateQuery() for simple datadog query and args', () => { @@ -389,22 +391,22 @@ describe('analysis modal transforms', () => { }); test('interpolateQuery() for wavefront query and args', () => { expect(interpolateQuery(MOCK_QUERY_WAVEFRONT, MOCK_ARGS_WAVEFRONT)).toBe( - 'sum(rate(5m, ts("istio.requestcount.count", response_code!=500 and destination_service="istio-host-split-canary"))) / sum(rate(5m, ts("istio.requestcount.count", reporter=client and destination_service="istio-host-split-canary")))' + 'sum(rate(5m, ts("istio.requestcount.count", response_code!=500 and destination_service="istio-host-split-canary"))) / sum(rate(5m, ts("istio.requestcount.count", reporter=client and destination_service="istio-host-split-canary")))', ); }); test('interpolateQuery() for graphite query and args', () => { expect(interpolateQuery(MOCK_QUERY_GRAPHITE, MOCK_ARGS_GRAPHITE)).toBe( - "target=summarize(asPercent(sumSeries(stats.timers.httpServerRequests.app.istio-host-split-canary.exception.*.method.*.outcome.{CLIENT_ERROR,INFORMATIONAL,REDIRECTION,SUCCESS}.status.*.uri.*.count), sumSeries(stats.timers.httpServerRequests.app.istio-host-split-canary.exception.*.method.*.outcome.*.status.*.uri.*.count)),'5min','avg')" + "target=summarize(asPercent(sumSeries(stats.timers.httpServerRequests.app.istio-host-split-canary.exception.*.method.*.outcome.{CLIENT_ERROR,INFORMATIONAL,REDIRECTION,SUCCESS}.status.*.uri.*.count), sumSeries(stats.timers.httpServerRequests.app.istio-host-split-canary.exception.*.method.*.outcome.*.status.*.uri.*.count)),'5min','avg')", ); }); test('interpolateQuery() for influxdb query and args', () => { expect(interpolateQuery(MOCK_QUERY_INFLUXDB, MOCK_ARGS_INFLUXDB)).toBe( - 'from(bucket: "app_istio") range(start: -15m) filter(fn: (r) => r["destination_workload"] == "myApp")|> filter(fn: (r) => r["_measurement"] == "istio:istio_requests_errors_percentage:rate1m:5xx")' + 'from(bucket: "app_istio") range(start: -15m) filter(fn: (r) => r["destination_workload"] == "myApp")|> filter(fn: (r) => r["_measurement"] == "istio:istio_requests_errors_percentage:rate1m:5xx")', ); }); test('interpolateQuery() for skywalking query and args', () => { expect(interpolateQuery(MOCK_QUERY_SKYWALKING, MOCK_ARGS_SKYWALKING)).toBe( - 'query queryData($duration: Duration!) { service_apdex: readMetricsValues(condition: { name: "service_apdex", entity: { scope: Service, serviceName: "istio-host-split-canary", normal: true } }, duration: $duration) { label values { values { value } } } }' + 'query queryData($duration: Duration!) { service_apdex: readMetricsValues(condition: { name: "service_apdex", entity: { scope: Service, serviceName: "istio-host-split-canary", normal: true } }, duration: $duration) { label values { values { value } } } }', ); }); @@ -545,4 +547,62 @@ describe('analysis modal transforms', () => { tableValue: {latency: null, cpuUsage: null}, }); }); + const MOCK_MEASUREMENTS: GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1Measurement[] = [{value: '[5]'}, {value: '[10]'}, {value: '[15]'}]; + const MOCK_MEASUREMENTS_WITH_NAN: GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1Measurement[] = [{value: '[NaN]'}, {value: '[10]'}, {value: '[15]'}]; + const MOCK_MEASUREMENTS_WITH_STRING: GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1Measurement[] = [{value: 'NaN'}, {value: '10'}, {value: '15'}]; + + it('transforms measurements correctly with valid numbers', () => { + expect(transformMeasurements(['0'], MOCK_MEASUREMENTS)).toEqual({ + chartable: true, + min: 0, + max: null, + measurements: [ + {value: '[5]', chartValue: {'0': 5}, tableValue: {'0': 5}}, + {value: '[10]', chartValue: {'0': 10}, tableValue: {'0': 10}}, + {value: '[15]', chartValue: {'0': 15}, tableValue: {'0': 15}}, + ], + }); + }); + it('transforms measurements correctly with NaN numbers in list', () => { + expect(transformMeasurements(['0'], MOCK_MEASUREMENTS_WITH_NAN)).toEqual({ + chartable: true, + min: 0, + max: null, + measurements: [ + {value: '[NaN]', chartValue: null, tableValue: null}, + {value: '[10]', chartValue: {'0': 10}, tableValue: {'0': 10}}, + {value: '[15]', chartValue: {'0': 15}, tableValue: {'0': 15}}, + ], + }); + }); + it('returns default values when measurements are undefined', () => { + expect(transformMeasurements(['0'])).toEqual({ + chartable: false, + min: 0, + max: null, + measurements: [], + }); + }); + + it('returns default values when measurements are empty', () => { + expect(transformMeasurements(['0'], [])).toEqual({ + chartable: false, + min: 0, + max: null, + measurements: [], + }); + }); + + it('handles measurements with string values', () => { + expect(transformMeasurements(['0'], MOCK_MEASUREMENTS_WITH_STRING)).toEqual({ + chartable: true, + min: 0, + max: 15, + measurements: [ + {value: 'NaN', chartValue: null, tableValue: null}, + {value: '10', chartValue: 10, tableValue: 10}, + {value: '15', chartValue: 15, tableValue: 15}, + ], + }); + }); }); diff --git a/ui/src/app/components/analysis-modal/transforms.ts b/ui/src/app/components/analysis-modal/transforms.ts index 6d869328ae..63459e4e5c 100644 --- a/ui/src/app/components/analysis-modal/transforms.ts +++ b/ui/src/app/components/analysis-modal/transforms.ts @@ -1,5 +1,4 @@ // eslint-disable-file @typescript-eslint/ban-ts-comment -import JSON5 from 'json5' import * as moment from 'moment'; import { @@ -132,7 +131,7 @@ const PROVIDER_CONDITION_SUPPORT: { export const conditionDetails = ( condition?: string, args: GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1Argument[] = [], - provider?: GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1MetricProvider + provider?: GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1MetricProvider, ): { label: string | null; thresholds: number[]; @@ -209,7 +208,7 @@ export const chartMax = (valueMax: number, failThresholds: number[] | null, succ * @param phase analysis phase * @returns analysis phase adjusted to render the UI status with a more accurate functional status */ -export const getAdjustedMetricPhase = (phase?: AnalysisStatus): AnalysisStatus => (phase === AnalysisStatus.Error ? AnalysisStatus.Failed : phase ?? AnalysisStatus.Unknown); +export const getAdjustedMetricPhase = (phase?: AnalysisStatus): AnalysisStatus => (phase === AnalysisStatus.Error ? AnalysisStatus.Failed : (phase ?? AnalysisStatus.Unknown)); /** * @@ -379,7 +378,7 @@ export const interpolateQuery = (query?: string, args?: GithubComArgoprojArgoRol */ export const printableDatadogQuery = ( datadog: GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1DatadogMetric, - args: GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1Argument[] + args: GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1Argument[], ): string[] | undefined => { if ((datadog.apiVersion ?? '').toLowerCase() === 'v1' && 'query' in datadog) { return [interpolateQuery(datadog.query, args)]; @@ -418,7 +417,7 @@ export const printableCloudWatchQuery = (cloudWatch: GithubComArgoprojArgoRollou */ export const metricQueries = ( provider?: GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1MetricProvider | null, - args: GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1Argument[] = [] + args: GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1Argument[] = [], ): string[] | undefined => { if (provider === undefined || provider === null) { return undefined; @@ -468,7 +467,7 @@ export const transformMeasurements = (conditionKeys: string[], measurements?: Gi return measurements.reduce( ( acc: {chartable: boolean; min: number; max: number | null; measurements: TransformedMeasurement[]}, - currMeasurement: GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1Measurement + currMeasurement: GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1Measurement, ) => { const transformedValue = transformMeasurementValue(conditionKeys, currMeasurement.value); const {canChart, tableValue} = transformedValue; @@ -488,7 +487,7 @@ export const transformMeasurements = (conditionKeys: string[], measurements?: Gi ], }; }, - {chartable: true, min: 0, max: null, measurements: [] as TransformedMeasurement[]} + {chartable: true, min: 0, max: null, measurements: [] as TransformedMeasurement[]}, ); }; @@ -508,7 +507,7 @@ type FormattedMeasurementValue = number | string | null; */ export const formattedValue = (value: any): FormattedMeasurementValue => { const isNum = isFiniteNumber(value); - return isNum ? roundNumber(Number(value)) : value?.toString() ?? null; + return isNum ? roundNumber(Number(value)) : (value?.toString() ?? null); }; /** @@ -534,6 +533,7 @@ const formatNumberMeasurement = (value: number): MeasurementValueInfo => { export const formatSingleItemArrayMeasurement = (value: FormattedMeasurementValue[], accessor: number): MeasurementValueInfo => { if (isFiniteNumber(accessor)) { const measurementValue = value?.[accessor] ?? null; + // if it's a number or null, chart it if (isFiniteNumber(measurementValue) || measurementValue === null) { const displayValue = formattedValue(measurementValue); @@ -618,7 +618,16 @@ const transformMeasurementValue = (conditionKeys: string[], value?: string): Mea }; } - const parsedValue = JSON5.parse(value); + let parsedValue; + try { + parsedValue = JSON.parse(value); + } catch { + return { + canChart: true, + chartValue: null, + tableValue: null, + }; + } // single number measurement value if (isFiniteNumber(parsedValue)) { diff --git a/ui/src/app/components/pods/pods.tsx b/ui/src/app/components/pods/pods.tsx index 5e7005fcbd..6b44aa3d41 100644 --- a/ui/src/app/components/pods/pods.tsx +++ b/ui/src/app/components/pods/pods.tsx @@ -21,6 +21,9 @@ export enum PodStatus { } const isPodReady = (ready: string) => { + if (!ready) { + return false; + } // Ready is a string in the format "0/1", "1/1", etc. const [current, total] = ready.split('/'); return current === total;