From 756fbdcd42f0ad5266c1ac1f32651d1a27003270 Mon Sep 17 00:00:00 2001 From: Melana Hammel <42477256+melanahammel@users.noreply.github.com> Date: Fri, 18 Dec 2020 07:04:34 -0800 Subject: [PATCH] fix: increase advanced config consistency (#81) --- .../components/config/AbstractMetricModal.tsx | 38 +++++++++++++++---- packages/client/src/domain/Kayenta.ts | 6 +-- .../client/src/stores/ConfigEditorStore.ts | 35 ++++++++++++++++- .../client/src/validation/configValidators.ts | 15 +++++--- 4 files changed, 75 insertions(+), 19 deletions(-) diff --git a/packages/client/src/components/config/AbstractMetricModal.tsx b/packages/client/src/components/config/AbstractMetricModal.tsx index 6f4577c2..a51634a2 100644 --- a/packages/client/src/components/config/AbstractMetricModal.tsx +++ b/packages/client/src/components/config/AbstractMetricModal.tsx @@ -33,6 +33,10 @@ const initialState = { metricName: false, aggregationMethod: false, dimensions: false, + criticalIncrease: false, + criticalDecrease: false, + allowedIncrease: false, + allowedDecrease: false, outlierStrategy: false }, isValid: true, @@ -47,7 +51,15 @@ const initialMetricState = { analysisConfigurations: { canary: { direction: 'either', - nanStrategy: 'remove' + critical: false, + nanStrategy: 'remove', + effectSize: { + allowedIncrease: 1, + allowedDecrease: 1 + }, + outliers: { + strategy: 'keep' + } } }, scopeName: 'default' @@ -304,6 +316,10 @@ export abstract class AbstractMetricModal if (criticality === 'critical') { delete canaryAnalysisConfigurationCopy['mustHaveData']; canaryAnalysisConfigurationCopy['critical'] = true; + canaryAnalysisConfigurationCopy['effectSize'] = { + criticalIncrease: 1, + criticalDecrease: 1 + }; } if (criticality === 'mustHaveData') { @@ -312,8 +328,12 @@ export abstract class AbstractMetricModal } if (criticality === 'normal') { - delete canaryAnalysisConfigurationCopy['critical']; + canaryAnalysisConfigurationCopy['critical'] = false; delete canaryAnalysisConfigurationCopy['mustHaveData']; + canaryAnalysisConfigurationCopy['effectSize'] = { + allowedIncrease: 1, + allowedDecrease: 1 + }; } this.setState( @@ -631,7 +651,7 @@ export abstract class AbstractMetricModal this.handleEffectSizeObjectChange('criticalIncrease', e.target.value)} @@ -645,7 +665,7 @@ export abstract class AbstractMetricModal this.handleEffectSizeObjectChange('criticalDecrease', e.target.value)} @@ -662,7 +682,7 @@ export abstract class AbstractMetricModal this.handleEffectSizeObjectChange('allowedIncrease', e.target.value)} @@ -676,7 +696,7 @@ export abstract class AbstractMetricModal this.handleEffectSizeObjectChange('allowedDecrease', e.target.value)} @@ -773,7 +793,11 @@ export abstract class AbstractMetricModal scopeName: true, metricName: true, aggregationMethod: true, - dimensions: true + dimensions: true, + criticalIncrease: true, + criticalDecrease: true, + allowedIncrease: true, + allowedDecrease: true } }); return; diff --git a/packages/client/src/domain/Kayenta.ts b/packages/client/src/domain/Kayenta.ts index 210be4a9..225073fa 100644 --- a/packages/client/src/domain/Kayenta.ts +++ b/packages/client/src/domain/Kayenta.ts @@ -146,7 +146,7 @@ export interface CanaryAnalysisConfiguration { nanStrategy?: string; critical?: boolean; mustHaveData?: boolean; - effectSize?: CanaryMetricEffectSizeConfigs; + effectSize?: CanaryMetricEffectSizeConfig; outliers?: CanaryMetricOutliersConfig; } @@ -176,10 +176,6 @@ export interface CanaryMetricEffectSizeConfig { criticalDecrease?: number; } -export interface CanaryMetricEffectSizeConfigs { - [name: string]: CanaryMetricEffectSizeConfig; -} - export interface CanaryJudgeConfig { name: string; judgeConfigurations: KvMap; diff --git a/packages/client/src/stores/ConfigEditorStore.ts b/packages/client/src/stores/ConfigEditorStore.ts index cb1ecb49..1031c4a2 100644 --- a/packages/client/src/stores/ConfigEditorStore.ts +++ b/packages/client/src/stores/ConfigEditorStore.ts @@ -1,5 +1,5 @@ -import { observable, action, computed } from 'mobx'; -import { CanaryConfig, CanaryMetricConfig, GroupWeights } from '../domain/Kayenta'; +import { action, computed, observable } from 'mobx'; +import { CanaryAnalysisConfiguration, CanaryConfig, CanaryMetricConfig, GroupWeights } from '../domain/Kayenta'; import CanaryConfigFactory from '../util/CanaryConfigFactory'; import log from '../util/LoggerFactory'; import { validateCanaryConfig } from '../validation/configValidators'; @@ -97,6 +97,34 @@ export default class ConfigEditorStore { this.metricSourceType = type; } + checkCanaryAnalysisConfiguration(canaryAnalysisConfig: CanaryAnalysisConfiguration): CanaryAnalysisConfiguration { + const canaryAnalysisConfigCopy = Object.assign({}, canaryAnalysisConfig); + if (!canaryAnalysisConfig['direction']) { + canaryAnalysisConfigCopy['direction'] = 'either'; + } + if (!canaryAnalysisConfig['nanStrategy']) { + canaryAnalysisConfigCopy['nanStrategy'] = 'remove'; + } + if (!canaryAnalysisConfig['critical']) { + canaryAnalysisConfigCopy['critical'] = false; + canaryAnalysisConfigCopy['effectSize'] = { + allowedIncrease: 1, + allowedDecrease: 1 + }; + } else { + canaryAnalysisConfigCopy['effectSize'] = { + criticalIncrease: 1, + criticalDecrease: 1 + }; + } + if (!canaryAnalysisConfig['outliers']) { + canaryAnalysisConfigCopy['outliers'] = { + strategy: 'keep' + }; + } + return canaryAnalysisConfigCopy; + } + @action.bound setCanaryConfigObject(canaryConfigObject: CanaryConfig): void { // todo figure out if this ad hoc config is necessary @@ -112,6 +140,9 @@ export default class ConfigEditorStore { metric.groups.forEach(group => { uniqueGroups.add(group); }); + metric.analysisConfigurations['canary'] = this.checkCanaryAnalysisConfiguration( + metric.analysisConfigurations['canary'] + ); }); } diff --git a/packages/client/src/validation/configValidators.ts b/packages/client/src/validation/configValidators.ts index db6ff6ff..8f55f965 100644 --- a/packages/client/src/validation/configValidators.ts +++ b/packages/client/src/validation/configValidators.ts @@ -40,11 +40,16 @@ const getCanaryMetricConfigSchema = (metricQueryObjectSchema: KvMap> nanStrategy: mixed().oneOf(['remove', 'replace']), critical: boolean(), mustHaveData: boolean(), - effectSize: object().shape({ - allowedIncrease: number(), - allowedDecrease: number(), - criticalIncrease: number(), - criticalDecrease: number() + effectSize: object().when('critical', { + is: true, + then: object({ + criticalIncrease: number().required('Critical Increase is a required field. Defaults to 1.'), + criticalDecrease: number().required('Critical Decrease is a required field. Defaults to 1.') + }), + otherwise: object({ + allowedIncrease: number().required('Allowed Increase is a required field. Defaults to 1.'), + allowedDecrease: number().required('Allowed Decrease is a required field. Defaults to 1.') + }) }), outliers: object().shape({ strategy: string().oneOf(['keep', 'remove']),