Skip to content
This repository has been archived by the owner on Jun 25, 2024. It is now read-only.

Commit

Permalink
fix: increase advanced config consistency (#81)
Browse files Browse the repository at this point in the history
  • Loading branch information
@melanahammel
melanahammel authored Dec 18, 2020
1 parent 836018c commit 756fbdc
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 19 deletions.
38 changes: 31 additions & 7 deletions packages/client/src/components/config/AbstractMetricModal.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ const initialState = {
metricName: false,
aggregationMethod: false,
dimensions: false,
criticalIncrease: false,
criticalDecrease: false,
allowedIncrease: false,
allowedDecrease: false,
outlierStrategy: false
},
isValid: true,
Expand All @@ -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'
Expand Down Expand Up @@ -304,6 +316,10 @@ export abstract class AbstractMetricModal<T extends CanaryMetricSetQueryConfig>
if (criticality === 'critical') {
delete canaryAnalysisConfigurationCopy['mustHaveData'];
canaryAnalysisConfigurationCopy['critical'] = true;
canaryAnalysisConfigurationCopy['effectSize'] = {
criticalIncrease: 1,
criticalDecrease: 1
};
}

if (criticality === 'mustHaveData') {
Expand All @@ -312,8 +328,12 @@ export abstract class AbstractMetricModal<T extends CanaryMetricSetQueryConfig>
}

if (criticality === 'normal') {
delete canaryAnalysisConfigurationCopy['critical'];
canaryAnalysisConfigurationCopy['critical'] = false;
delete canaryAnalysisConfigurationCopy['mustHaveData'];
canaryAnalysisConfigurationCopy['effectSize'] = {
allowedIncrease: 1,
allowedDecrease: 1
};
}

this.setState(
Expand Down Expand Up @@ -631,7 +651,7 @@ export abstract class AbstractMetricModal<T extends CanaryMetricSetQueryConfig>
<InlineTextGroup
id="criticalIncrease"
label="Critical Increase"
touched={this.state.touched['analysisConfigurations.canary.effectSize.criticalIncrease']}
touched={this.state.touched.criticalIncrease}
error={this.state.errors['analysisConfigurations.canary.effectSize.criticalIncrease']}
value={this.getEffectSizeObject('criticalIncrease')}
onChange={e => this.handleEffectSizeObjectChange('criticalIncrease', e.target.value)}
Expand All @@ -645,7 +665,7 @@ export abstract class AbstractMetricModal<T extends CanaryMetricSetQueryConfig>
<InlineTextGroup
id="criticalDecrease"
label="Critical Decrease"
touched={this.state.touched['analysisConfigurations.canary.effectSize.criticalDecrease']}
touched={this.state.touched.criticalDecrease}
error={this.state.errors['analysisConfigurations.canary.effectSize.criticalDecrease']}
value={this.getEffectSizeObject('criticalDecrease')}
onChange={e => this.handleEffectSizeObjectChange('criticalDecrease', e.target.value)}
Expand All @@ -662,7 +682,7 @@ export abstract class AbstractMetricModal<T extends CanaryMetricSetQueryConfig>
<InlineTextGroup
id="allowedIncrease"
label="Allowed Increase"
touched={this.state.touched['analysisConfigurations.canary.effectSize.allowedIncrease']}
touched={this.state.touched.allowedIncrease}
error={this.state.errors['analysisConfigurations.canary.effectSize.allowedIncrease']}
value={this.getEffectSizeObject('allowedIncrease')}
onChange={e => this.handleEffectSizeObjectChange('allowedIncrease', e.target.value)}
Expand All @@ -676,7 +696,7 @@ export abstract class AbstractMetricModal<T extends CanaryMetricSetQueryConfig>
<InlineTextGroup
id="allowedDecrease"
label="Allowed Decrease"
touched={this.state.touched['analysisConfigurations.canary.effectSize.allowedDecrease']}
touched={this.state.touched.allowedDecrease}
error={this.state.errors['analysisConfigurations.canary.effectSize.allowedDecrease']}
value={this.getEffectSizeObject('allowedDecrease')}
onChange={e => this.handleEffectSizeObjectChange('allowedDecrease', e.target.value)}
Expand Down Expand Up @@ -773,7 +793,11 @@ export abstract class AbstractMetricModal<T extends CanaryMetricSetQueryConfig>
scopeName: true,
metricName: true,
aggregationMethod: true,
dimensions: true
dimensions: true,
criticalIncrease: true,
criticalDecrease: true,
allowedIncrease: true,
allowedDecrease: true
}
});
return;
Expand Down
6 changes: 1 addition & 5 deletions packages/client/src/domain/Kayenta.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ export interface CanaryAnalysisConfiguration {
nanStrategy?: string;
critical?: boolean;
mustHaveData?: boolean;
effectSize?: CanaryMetricEffectSizeConfigs;
effectSize?: CanaryMetricEffectSizeConfig;
outliers?: CanaryMetricOutliersConfig;
}

Expand Down Expand Up @@ -176,10 +176,6 @@ export interface CanaryMetricEffectSizeConfig {
criticalDecrease?: number;
}

export interface CanaryMetricEffectSizeConfigs {
[name: string]: CanaryMetricEffectSizeConfig;
}

export interface CanaryJudgeConfig {
name: string;
judgeConfigurations: KvMap<any>;
Expand Down
35 changes: 33 additions & 2 deletions packages/client/src/stores/ConfigEditorStore.ts
View file
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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
Expand All @@ -112,6 +140,9 @@ export default class ConfigEditorStore {
metric.groups.forEach(group => {
uniqueGroups.add(group);
});
metric.analysisConfigurations['canary'] = this.checkCanaryAnalysisConfiguration(
metric.analysisConfigurations['canary']
);
});
}

Expand Down
15 changes: 10 additions & 5 deletions packages/client/src/validation/configValidators.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,16 @@ const getCanaryMetricConfigSchema = (metricQueryObjectSchema: KvMap<Schema<any>>
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']),
Expand Down

0 comments on commit 756fbdc

Please sign in to comment.