Skip to content

Commit

Permalink
PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@ogupte
ogupte committed Oct 28, 2022
1 parent 9a91b16 commit 0940bf3
Show file tree
Hide file tree
Showing 12 changed files with 88 additions and 69 deletions.
24 changes: 14 additions & 10 deletions x-pack/plugins/apm/common/service_groups.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,22 @@ import {
SERVICE_FRAMEWORK_VERSION,
} from './elasticsearch_fieldnames';

const unsupportedFields = [
TRANSACTION_TYPE,
TRANSACTION_DURATION,
SERVICE_FRAMEWORK_VERSION,
];
const mockFields = [...SERVICE_GROUP_SUPPORTED_FIELDS, ...unsupportedFields];

describe('service_groups common utils', () => {
describe('isSupportedField', () => {
it('should only filter supported supported fields for service groups', () => {
const supportedFields = mockFields.filter(isSupportedField);
expect(supportedFields).toEqual(SERVICE_GROUP_SUPPORTED_FIELDS);
it('should allow supported fields', () => {
SERVICE_GROUP_SUPPORTED_FIELDS.map((field) => {
expect(isSupportedField(field)).toBe(true);
});
});
it('should reject unsupported fields', () => {
const unsupportedFields = [
TRANSACTION_TYPE,
TRANSACTION_DURATION,
SERVICE_FRAMEWORK_VERSION,
];
unsupportedFields.map((field) => {
expect(isSupportedField(field)).toBe(false);
});
});
});
describe('validateServiceGroupKuery', () => {
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/apm/common/utils/environment_query.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
import { SERVICE_NODE_NAME_MISSING } from '../service_nodes';

export function environmentQuery(
environment: string
environment: string | undefined
): QueryDslQueryContainer[] {
if (!environment || environment === ENVIRONMENT_ALL.value) {
return [];
Expand Down
6 changes: 3 additions & 3 deletions x-pack/plugins/apm/public/components/alerting/utils/fields.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export function ServiceField({
)}
defaultValue={currentValue}
fieldName={SERVICE_NAME}
onChange={onChange}
onChange={(value) => onChange(value || undefined)}
placeholder={i18n.translate('xpack.apm.serviceNamesSelectPlaceholder', {
defaultMessage: 'Select service name',
})}
Expand Down Expand Up @@ -82,7 +82,7 @@ export function EnvironmentField({
)}
defaultValue={getEnvironmentLabel(currentValue)}
fieldName={SERVICE_ENVIRONMENT}
onChange={onChange}
onChange={(value) => onChange(value || undefined)}
placeholder={i18n.translate('xpack.apm.environmentsSelectPlaceholder', {
defaultMessage: 'Select environment',
})}
Expand Down Expand Up @@ -115,7 +115,7 @@ export function TransactionTypeField({
)}
defaultValue={currentValue}
fieldName={TRANSACTION_TYPE}
onChange={onChange}
onChange={(value) => onChange(value || undefined)}
placeholder={i18n.translate(
'xpack.apm.transactionTypesSelectPlaceholder',
{
Expand Down
6 changes: 4 additions & 2 deletions ...k/plugins/apm/public/components/app/service_groups/service_group_save/select_services.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@ export function SelectServices({
}: Props) {
const [kuery, setKuery] = useState(serviceGroup?.kuery || '');
const [stagedKuery, setStagedKuery] = useState(serviceGroup?.kuery || '');
const [kueryValidationMessage, setKueryValidationMessage] = useState('');
const [kueryValidationMessage, setKueryValidationMessage] = useState<
string | undefined
>();

useEffect(() => {
if (isEdit) {
Expand All @@ -83,7 +85,7 @@ export function SelectServices({
const { isValid, isParsingError, message } =
validateServiceGroupKuery(stagedKuery);
if (isValid || isParsingError) {
setKueryValidationMessage('');
setKueryValidationMessage(undefined);
} else {
setKueryValidationMessage(message);
}
Expand Down
32 changes: 25 additions & 7 deletions ...maly/get_anomalous_event_source_fields.ts → ...y/get_service_group_fields_for_anomaly.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,33 +4,51 @@
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/
import { IScopedClusterClient } from '@kbn/core/server';
import { firstValueFrom } from 'rxjs';
import {
IScopedClusterClient,
SavedObjectsClientContract,
} from '@kbn/core/server';
import {
SERVICE_ENVIRONMENT,
SERVICE_NAME,
TRANSACTION_TYPE,
TRANSACTION_DURATION,
} from '../../../../../common/elasticsearch_fieldnames';
import { alertingEsClient } from '../../alerting_es_client';
import { getSourceFields, getSourceFieldsAgg } from '../get_source_fields';
import {
getServiceGroupFields,
getServiceGroupFieldsAgg,
} from '../get_service_group_fields';
import { getApmIndices } from '../../../settings/apm_indices/get_apm_indices';
import { RegisterRuleDependencies } from '../../register_apm_rule_types';

export async function getAnomalousEventSourceFields({
export async function getServiceGroupFieldsForAnomaly({
config$,
scopedClusterClient,
index,
savedObjectsClient,
serviceName,
environment,
transactionType,
timestamp,
bucketSpan,
}: {
config$: RegisterRuleDependencies['config$'];
scopedClusterClient: IScopedClusterClient;
index: string;
savedObjectsClient: SavedObjectsClientContract;
serviceName: string;
environment: string;
transactionType: string;
timestamp: number;
bucketSpan: number;
}) {
const config = await firstValueFrom(config$);
const indices = await getApmIndices({
config,
savedObjectsClient,
});
const { transaction: index } = indices;

const params = {
index,
body: {
Expand All @@ -55,7 +73,7 @@ export async function getAnomalousEventSourceFields({
},
},
aggs: {
...getSourceFieldsAgg({
...getServiceGroupFieldsAgg({
sort: [{ [TRANSACTION_DURATION]: { order: 'desc' as const } }],
}),
},
Expand All @@ -69,5 +87,5 @@ export async function getAnomalousEventSourceFields({
if (!response.aggregations) {
return {};
}
return getSourceFields(response.aggregations);
return getServiceGroupFields(response.aggregations);
}
28 changes: 7 additions & 21 deletions x-pack/plugins/apm/server/routes/alerts/rule_types/anomaly/register_anomaly_rule_type.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import { KibanaRequest } from '@kbn/core/server';
import { termQuery } from '@kbn/observability-plugin/server';
import { createLifecycleRuleTypeFactory } from '@kbn/rule-registry-plugin/server';
import { ProcessorEvent } from '@kbn/observability-plugin/common';
import { firstValueFrom } from 'rxjs';
import {
ApmRuleType,
RULE_TYPES_CONFIG,
Expand Down Expand Up @@ -47,15 +46,14 @@ import { getAlertUrlTransaction } from '../../../../../common/utils/formatters';
import { getMLJobs } from '../../../service_map/get_service_anomalies';
import { apmActionVariables } from '../../action_variables';
import { RegisterRuleDependencies } from '../../register_apm_rule_types';
import { getAnomalousEventSourceFields } from './get_anomalous_event_source_fields';
import { getApmIndices } from '../../../settings/apm_indices/get_apm_indices';
import { getServiceGroupFieldsForAnomaly } from './get_service_group_fields_for_anomaly';

const paramsSchema = schema.object({
serviceName: schema.maybe(schema.string()),
transactionType: schema.maybe(schema.string()),
windowSize: schema.number(),
windowUnit: schema.string(),
environment: schema.string(),
environment: schema.maybe(schema.string()),
anomalySeverityType: schema.oneOf([
schema.literal(ANOMALY_SEVERITY.CRITICAL),
schema.literal(ANOMALY_SEVERITY.MAJOR),
Expand Down Expand Up @@ -107,13 +105,6 @@ export function registerAnomalyRuleType({
return {};
}

const config = await firstValueFrom(config$);
const indices = await getApmIndices({
config,
savedObjectsClient: services.savedObjectsClient,
});
const { transaction: transactionsIndex } = indices;

const ruleParams = params;
const request = {} as KibanaRequest;
const { mlAnomalySearch } = ml.mlSystemProvider(
Expand Down Expand Up @@ -173,14 +164,8 @@ export function registerAnomalyRuleType({
},
},
},
...termQuery(
'partition_field_value',
ruleParams.serviceName || null
),
...termQuery(
'by_field_value',
ruleParams.transactionType || null
),
...termQuery('partition_field_value', ruleParams.serviceName),
...termQuery('by_field_value', ruleParams.transactionType),
...termQuery(
'detector_index',
getApmMlDetectorIndex(ApmMlDetectorType.txLatency)
Expand Down Expand Up @@ -261,9 +246,10 @@ export function registerAnomalyRuleType({
bucketSpan,
} = anomaly;

const eventSourceFields = await getAnomalousEventSourceFields({
const eventSourceFields = await getServiceGroupFieldsForAnomaly({
config$,
scopedClusterClient: services.scopedClusterClient,
index: transactionsIndex,
savedObjectsClient: services.savedObjectsClient,
serviceName,
environment,
transactionType,
Expand Down
11 changes: 7 additions & 4 deletions ...plugins/apm/server/routes/alerts/rule_types/error_count/register_error_count_rule_type.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,17 @@ import { getApmIndices } from '../../../settings/apm_indices/get_apm_indices';
import { apmActionVariables } from '../../action_variables';
import { alertingEsClient } from '../../alerting_es_client';
import { RegisterRuleDependencies } from '../../register_apm_rule_types';
import { getSourceFieldsAgg, getSourceFields } from '../get_source_fields';
import {
getServiceGroupFieldsAgg,
getServiceGroupFields,
} from '../get_service_group_fields';

const paramsSchema = schema.object({
windowSize: schema.number(),
windowUnit: schema.string(),
threshold: schema.number(),
serviceName: schema.maybe(schema.string()),
environment: schema.string(),
environment: schema.maybe(schema.string()),
});

const ruleTypeConfig = RULE_TYPES_CONFIG[ApmRuleType.ErrorCount];
Expand Down Expand Up @@ -126,7 +129,7 @@ export function registerErrorCountRuleType({
size: 1000,
order: { _count: 'desc' as const },
},
aggs: getSourceFieldsAgg(),
aggs: getServiceGroupFieldsAgg(),
},
},
},
Expand All @@ -144,7 +147,7 @@ export function registerErrorCountRuleType({
serviceName,
environment,
errorCount: bucket.doc_count,
sourceFields: getSourceFields(bucket),
sourceFields: getServiceGroupFields(bucket),
};
}) ?? [];

Expand Down
12 changes: 6 additions & 6 deletions ...erts/rule_types/get_source_fields.test.ts → ...le_types/get_service_group_fields.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@
*/

import {
getSourceFields,
getSourceFieldsAgg,
getServiceGroupFields,
getServiceGroupFieldsAgg,
flattenSourceDoc,
} from './get_source_fields';
} from './get_service_group_fields';

const mockSourceObj = {
service: {
Expand Down Expand Up @@ -37,7 +37,7 @@ const mockBucket = {

describe('getSourceFields', () => {
it('should return a flattened record of fields and values for a given bucket', () => {
const result = getSourceFields(mockBucket);
const result = getServiceGroupFields(mockBucket);
expect(result).toMatchInlineSnapshot(`
Object {
"agent.name": "nodejs",
Expand All @@ -52,7 +52,7 @@ describe('getSourceFields', () => {

describe('getSourceFieldsAgg', () => {
it('should create a agg for specific source fields', () => {
const agg = getSourceFieldsAgg();
const agg = getServiceGroupFieldsAgg();
expect(agg).toMatchInlineSnapshot(`
Object {
"source_fields": Object {
Expand All @@ -74,7 +74,7 @@ describe('getSourceFieldsAgg', () => {
});

it('should accept options for top_hits options', () => {
const agg = getSourceFieldsAgg({
const agg = getServiceGroupFieldsAgg({
sort: [{ 'transaction.duration.us': { order: 'desc' } }],
});
expect(agg).toMatchInlineSnapshot(`
Expand Down
4 changes: 2 additions & 2 deletions ...es/alerts/rule_types/get_source_fields.ts → ...ts/rule_types/get_service_group_fields.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export interface SourceDoc {
[key: string]: string | SourceDoc;
}

export function getSourceFieldsAgg(
export function getServiceGroupFieldsAgg(
topHitsOpts: AggregationsTopHitsAggregation = {}
) {
return {
Expand All @@ -36,7 +36,7 @@ interface AggResultBucket {
};
}

export function getSourceFields(bucket?: AggResultBucket) {
export function getServiceGroupFields(bucket?: AggResultBucket) {
if (!bucket) {
return {};
}
Expand Down
15 changes: 9 additions & 6 deletions .../routes/alerts/rule_types/transaction_duration/register_transaction_duration_rule_type.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,14 @@ import {
averageOrPercentileAgg,
getMultiTermsSortOrder,
} from './average_or_percentile_agg';
import { getSourceFields, getSourceFieldsAgg } from '../get_source_fields';
import {
getServiceGroupFields,
getServiceGroupFieldsAgg,
} from '../get_service_group_fields';

const paramsSchema = schema.object({
serviceName: schema.string(),
transactionType: schema.string(),
serviceName: schema.maybe(schema.string()),
transactionType: schema.maybe(schema.string()),
windowSize: schema.number(),
windowUnit: schema.string(),
threshold: schema.number(),
Expand All @@ -64,7 +67,7 @@ const paramsSchema = schema.object({
schema.literal(AggregationType.P95),
schema.literal(AggregationType.P99),
]),
environment: schema.string(),
environment: schema.maybe(schema.string()),
});

const ruleTypeConfig = RULE_TYPES_CONFIG[ApmRuleType.TransactionDuration];
Expand Down Expand Up @@ -170,7 +173,7 @@ export function registerTransactionDurationRuleType({
aggregationType: ruleParams.aggregationType,
transactionDurationField: field,
}),
...getSourceFieldsAgg(),
...getServiceGroupFieldsAgg(),
},
},
},
Expand Down Expand Up @@ -205,7 +208,7 @@ export function registerTransactionDurationRuleType({
environment,
transactionType,
transactionDuration,
sourceFields: getSourceFields(bucket),
sourceFields: getServiceGroupFields(bucket),
});
}
}
Expand Down
Loading

0 comments on commit 0940bf3

Please sign in to comment.