Skip to content

Commit

Permalink
[SLOS] fix APM group by cardinality count (#183171)
Browse files Browse the repository at this point in the history
## Summary
Resolves #179046
Summarize your PR. If it involves visual changes include a screenshot or
gif.

Applies filters based on the selected APM indicator params to the
cardinality count query.

<img width="845" alt="Screenshot 2024-05-10 at 1 07 27 PM"
src="https://github.com/elastic/kibana/assets/11356435/7283f7cc-b141-47e2-883e-e463556e4aec">

### Testing.
1. Create mock api data via `node scripts/synthtrace continuous_rollups
--live`
2. Navigate to create SLI and choose APM latency
3. Select a service
4. Select group by as `service.name`. Cardinality count should be 1
5. Select environment all and change the group by to
`service.environment`. Cardinality count should be 2.
6. Now select a specific environment. Notice cardinality count changes
to 1.
7. Repeat this testing strategy with transaction name and transaction
type, changing the group by field to `transaction.name` or
`transaction.type`, respectively and playing around with the ALL_VALUE
versus a specific value.
8. Also make sure to test the global filter for regressions

### Release note
The cardinality count for SLOs generated from a single SLI definition
was previously incorrect for APM latency and APM availability SLIs. The
cardinality count is now accurate.
  • Loading branch information
dominiqueclarke committed May 14, 2024
1 parent 3ecd73c commit d3945a3
Show file tree
Hide file tree
Showing 11 changed files with 625 additions and 301 deletions.
3 changes: 3 additions & 0 deletions x-pack/packages/kbn-slo-schema/src/rest_specs/indicators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
kqlWithFiltersSchema,
metricCustomIndicatorSchema,
querySchema,
filtersSchema,
groupingsSchema,
syntheticsAvailabilityIndicatorSchema,
timesliceMetricBasicMetricWithField,
Expand Down Expand Up @@ -42,6 +43,7 @@ type HistogramIndicator = t.OutputOf<typeof histogramIndicatorSchema>;
type KQLCustomIndicator = t.OutputOf<typeof kqlCustomIndicatorSchema>;
type KqlWithFiltersSchema = t.TypeOf<typeof kqlWithFiltersSchema>;
type QuerySchema = t.TypeOf<typeof querySchema>;
type FiltersSchema = t.TypeOf<typeof filtersSchema>;
type GroupingsSchema = t.TypeOf<typeof groupingsSchema>;

export type {
Expand All @@ -59,5 +61,6 @@ export type {
KQLCustomIndicator,
KqlWithFiltersSchema,
QuerySchema,
FiltersSchema,
GroupingsSchema,
};
45 changes: 24 additions & 21 deletions x-pack/packages/kbn-slo-schema/src/schema/indicators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,29 +10,31 @@ import { allOrAnyString } from './common';

const kqlQuerySchema = t.string;

const filtersSchema = t.array(
t.type({
meta: t.partial({
alias: t.union([t.string, t.null]),
disabled: t.boolean,
negate: t.boolean,
// controlledBy is there to identify who owns the filter
controlledBy: t.string,
// allows grouping of filters
group: t.string,
// index and type are optional only because when you create a new filter, there are no defaults
index: t.string,
isMultiIndex: t.boolean,
type: t.string,
key: t.string,
params: t.any,
value: t.string,
}),
query: t.record(t.string, t.any),
})
);

const kqlWithFiltersSchema = t.type({
kqlQuery: t.string,
filters: t.array(
t.type({
meta: t.partial({
alias: t.union([t.string, t.null]),
disabled: t.boolean,
negate: t.boolean,
// controlledBy is there to identify who owns the filter
controlledBy: t.string,
// allows grouping of filters
group: t.string,
// index and type are optional only because when you create a new filter, there are no defaults
index: t.string,
isMultiIndex: t.boolean,
type: t.string,
key: t.string,
params: t.any,
value: t.string,
}),
query: t.record(t.string, t.any),
})
),
filters: filtersSchema,
});

const querySchema = t.union([kqlQuerySchema, kqlWithFiltersSchema]);
Expand Down Expand Up @@ -314,6 +316,7 @@ export {
kqlQuerySchema,
kqlWithFiltersSchema,
querySchema,
filtersSchema,
apmTransactionDurationIndicatorSchema,
apmTransactionDurationIndicatorTypeSchema,
apmTransactionErrorRateIndicatorSchema,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import { EuiFlexGroup, EuiFlexItem, EuiIconTip } from '@elastic/eui';
import { APMTransactionErrorRateIndicator } from '@kbn/slo-schema';
import { i18n } from '@kbn/i18n';
import React, { useEffect } from 'react';
import { useFormContext } from 'react-hook-form';
Expand All @@ -16,11 +17,34 @@ import { CreateSLOForm } from '../../types';
import { FieldSelector } from '../apm_common/field_selector';
import { DataPreviewChart } from '../common/data_preview_chart';
import { QueryBuilder } from '../common/query_builder';
import { formatAllFilters } from '../../helpers/format_filters';
import { getGroupByCardinalityFilters } from '../apm_common/get_group_by_cardinality_filters';

export function ApmAvailabilityIndicatorTypeForm() {
const { watch, setValue } = useFormContext<CreateSLOForm>();
const { watch, setValue } = useFormContext<CreateSLOForm<APMTransactionErrorRateIndicator>>();
const { data: apmIndex } = useFetchApmIndex();

const [
serviceName = '',
environment = '',
transactionType = '',
transactionName = '',
globalFilters,
] = watch([
'indicator.params.service',
'indicator.params.environment',
'indicator.params.transactionType',
'indicator.params.transactionName',
'indicator.params.filter',
]);
const indicatorParamsFilters = getGroupByCardinalityFilters({
serviceName,
environment,
transactionType,
transactionName,
});
const allFilters = formatAllFilters(globalFilters, indicatorParamsFilters);

useEffect(() => {
if (apmIndex !== '') {
setValue('indicator.params.index', apmIndex);
Expand Down Expand Up @@ -126,7 +150,7 @@ export function ApmAvailabilityIndicatorTypeForm() {
</EuiFlexItem>
</EuiFlexGroup>

<GroupByField dataView={dataView} isLoading={isIndexFieldsLoading} />
<GroupByField dataView={dataView} isLoading={isIndexFieldsLoading} filters={allFilters} />

<DataPreviewChart />
</EuiFlexGroup>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/
import { getGroupByCardinalityFilters } from './get_group_by_cardinality_filters';
import { ALL_VALUE } from '@kbn/slo-schema';

describe('get group by cardinality filters', () => {
it('formats filters correctly', () => {
const serviceName = 'testService';
const environment = 'testEnvironment';
const transactionName = 'testTransactionName';
const transactionType = 'testTransactionType';
expect(
getGroupByCardinalityFilters({ serviceName, environment, transactionName, transactionType })
).toEqual([
{
$state: { store: 'appState' },
meta: {
alias: null,
disabled: false,
key: 'service.name',
negate: false,
params: serviceName,
type: 'phrases',
},
query: {
bool: {
minimum_should_match: 1,
should: { match_phrase: { 'service.name': serviceName } },
},
},
},
{
$state: { store: 'appState' },
meta: {
alias: null,
disabled: false,
key: 'service.environment',
negate: false,
params: environment,
type: 'phrases',
},
query: {
bool: {
minimum_should_match: 1,
should: { match_phrase: { 'service.environment': environment } },
},
},
},
{
$state: { store: 'appState' },
meta: {
alias: null,
disabled: false,
key: 'transaction.type',
negate: false,
params: transactionType,
type: 'phrases',
},
query: {
bool: {
minimum_should_match: 1,
should: { match_phrase: { 'transaction.type': transactionType } },
},
},
},
{
$state: { store: 'appState' },
meta: {
alias: null,
disabled: false,
key: 'transaction.name',
negate: false,
params: transactionName,
type: 'phrases',
},
query: {
bool: {
minimum_should_match: 1,
should: { match_phrase: { 'transaction.name': transactionName } },
},
},
},
]);
});

it('does not include filters when values are undefined', () => {
expect(
getGroupByCardinalityFilters({
// @ts-ignore
serviceName: undefined,
environment: undefined,
transactionName: undefined,
transactionType: undefined,
})
).toEqual([]);
});

it('does not include filters when values are ALL_VALUE', () => {
expect(
getGroupByCardinalityFilters({
serviceName: ALL_VALUE,
environment: ALL_VALUE,
transactionName: ALL_VALUE,
transactionType: ALL_VALUE,
})
).toEqual([]);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/
import { ALL_VALUE, FiltersSchema } from '@kbn/slo-schema';
import { FilterStateStore } from '@kbn/es-query';

export const getGroupByCardinalityFilters = ({
serviceName,
environment,
transactionType,
transactionName,
}: {
serviceName: string;
environment?: string;
transactionType?: string;
transactionName?: string;
}): FiltersSchema => {
const serviceNameFilter =
serviceName && serviceName !== ALL_VALUE
? {
meta: {
disabled: false,
negate: false,
alias: null,
key: 'service.name',
params: serviceName,
type: 'phrases',
},
$state: {
store: FilterStateStore.APP_STATE,
},
query: {
bool: {
minimum_should_match: 1,
should: {
match_phrase: {
'service.name': serviceName,
},
},
},
},
}
: null;

const environmentFilter =
environment && environment !== ALL_VALUE
? {
meta: {
disabled: false,
negate: false,
alias: null,
key: 'service.environment',
params: environment,
type: 'phrases',
},
$state: {
store: FilterStateStore.APP_STATE,
},
query: {
bool: {
minimum_should_match: 1,
should: {
match_phrase: {
'service.environment': environment,
},
},
},
},
}
: null;

const transactionTypeFilter =
transactionType && transactionType !== ALL_VALUE
? {
meta: {
disabled: false,
negate: false,
alias: null,
key: 'transaction.type',
params: transactionType,
type: 'phrases',
},
$state: {
store: FilterStateStore.APP_STATE,
},
query: {
bool: {
minimum_should_match: 1,
should: {
match_phrase: {
'transaction.type': transactionType,
},
},
},
},
}
: null;

const transactionNameFilter =
transactionName && transactionName !== ALL_VALUE
? {
meta: {
disabled: false,
negate: false,
alias: null,
key: 'transaction.name',
params: transactionName,
type: 'phrases',
},
$state: {
store: FilterStateStore.APP_STATE,
},
query: {
bool: {
minimum_should_match: 1,
should: {
match_phrase: {
'transaction.name': transactionName,
},
},
},
},
}
: null;

return [
serviceNameFilter,
environmentFilter,
transactionTypeFilter,
transactionNameFilter,
].filter((value) => !!value) as FiltersSchema;
};

0 comments on commit d3945a3

Please sign in to comment.