Skip to content

remove show_all_group_by_instances from schema and change UI to selec… #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mgiota wants to merge 1 commit into
base: slo_alerts_schema_registration
Choose a base branch
from
Open

remove show_all_group_by_instances from schema and change UI to selec… #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 25 additions & 31 deletions ...servability/plugins/slo/common/embeddables/alerts/transforms/transform_alerts_out.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,14 @@ describe('transformAlertsOut', () => {
} as unknown as AlertsEmbeddableState)
).toMatchInlineSnapshot(`
Object {
"show_all_group_by_instances": true,
"slos": Array [
Object {
"group_by": Array [
"agent.id",
],
"name": "Legacy SLO",
"slo_id": "legacy-slo-id",
"slo_instance_id": "legacy-instance-id",
"slo_instance_id": "*",
},
],
}
Expand All @@ -42,7 +41,6 @@ describe('transformAlertsOut', () => {
it('should return state unchanged when already in snake_case', () => {
expect(
transformAlertsOut({
show_all_group_by_instances: false,
slos: [
{
slo_id: 'new-slo-id',
Expand All @@ -54,7 +52,6 @@ describe('transformAlertsOut', () => {
})
).toMatchInlineSnapshot(`
Object {
"show_all_group_by_instances": false,
"slos": Array [
Object {
"group_by": Array [
Expand Down Expand Up @@ -88,7 +85,6 @@ describe('transformAlertsOut', () => {
} as unknown as AlertsEmbeddableState)
).toMatchInlineSnapshot(`
Object {
"show_all_group_by_instances": true,
"slos": Array [
Object {
"group_by": Array [
Expand All @@ -103,52 +99,53 @@ describe('transformAlertsOut', () => {
`);
});

it('should migrate legacy showAllGroupByInstances to show_all_group_by_instances', () => {
it('should migrate legacy showAllGroupByInstances by expanding instance to * for grouped SLOs', () => {
expect(
transformAlertsOut({
showAllGroupByInstances: true,
slos: [],
slos: [
{
slo_id: 'slo-1',
slo_instance_id: 'instance-1',
name: 'SLO',
group_by: ['host.name'],
},
],
} as unknown as AlertsEmbeddableState)
).toMatchInlineSnapshot(`
Object {
"show_all_group_by_instances": true,
"slos": Array [],
"slos": Array [
Object {
"group_by": Array [
"host.name",
],
"name": "SLO",
"slo_id": "slo-1",
"slo_instance_id": "*",
},
],
}
`);
});

it('should not include legacy showAllGroupByInstances in output', () => {
it('should not include show_all_group_by_instances in output', () => {
const result = transformAlertsOut({
showAllGroupByInstances: false,
show_all_group_by_instances: true,
slos: [],
} as unknown as AlertsEmbeddableState);
expect(result).not.toHaveProperty('showAllGroupByInstances');
expect(result).toHaveProperty('show_all_group_by_instances', false);
});

it('should default show_all_group_by_instances to false when missing', () => {
expect(transformAlertsOut({ slos: [] } as unknown as AlertsEmbeddableState)).toMatchObject({
show_all_group_by_instances: false,
slos: [],
});
expect(result).not.toHaveProperty('show_all_group_by_instances');
expect(result).toMatchObject({ slos: [] });
});

it('should handle empty slos array', () => {
expect(
transformAlertsOut({
show_all_group_by_instances: false,
slos: [],
})
).toEqual({
show_all_group_by_instances: false,
slos: [],
});
expect(transformAlertsOut({ slos: [] })).toEqual({ slos: [] });
});

it('should handle mixed legacy and snake_case slo items', () => {
expect(
transformAlertsOut({
show_all_group_by_instances: false,
slos: [
{
slo_id: 'snake-slo',
Expand All @@ -166,7 +163,6 @@ describe('transformAlertsOut', () => {
} as unknown as AlertsEmbeddableState)
).toMatchInlineSnapshot(`
Object {
"show_all_group_by_instances": false,
"slos": Array [
Object {
"group_by": Array [],
Expand All @@ -191,12 +187,10 @@ describe('transformAlertsOut', () => {
expect(
transformAlertsOut({
title: 'My Alerts Panel',
show_all_group_by_instances: false,
slos: [],
} as unknown as AlertsEmbeddableState)
).toMatchObject({
title: 'My Alerts Panel',
show_all_group_by_instances: false,
slos: [],
});
});
Expand Down
34 changes: 26 additions & 8 deletions ...ns/observability/plugins/slo/common/embeddables/alerts/transforms/transform_alerts_out.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import { ALL_VALUE } from '@kbn/slo-schema';
import type {
AlertsEmbeddableState,
SloItem,
Expand All @@ -18,18 +19,25 @@ export interface LegacyAlertsSloItem {
}

/** Maps legacy camelCase slo item to current snake_case schema. */
function mapLegacySloItem(slo: Record<string, unknown>): SloItem {
function mapLegacySloItem(
slo: Record<string, unknown>,
legacyShowAllGroupByInstances: boolean
): SloItem {
const rawGroupBy = (slo.group_by ?? slo.groupBy) as string[] | undefined;
let sloInstanceId = (slo.slo_instance_id as string) ?? (slo.instanceId as string) ?? '';
if (legacyShowAllGroupByInstances && sloInstanceId && sloInstanceId !== ALL_VALUE) {
sloInstanceId = ALL_VALUE;
}
return {
slo_id: (slo.slo_id as string) ?? (slo.id as string) ?? '',
slo_instance_id: (slo.slo_instance_id as string) ?? (slo.instanceId as string) ?? '',
slo_instance_id: sloInstanceId,
name: (slo.name as string) ?? '',
group_by: Array.isArray(rawGroupBy) ? rawGroupBy : [],
};
}

export interface LegacyAlertsState {
showAllGroupByInstances: boolean;
showAllGroupByInstances?: boolean;
slos: LegacyAlertsSloItem[];
}

Expand All @@ -38,14 +46,24 @@ export function transformAlertsOut(storedState: AlertsEmbeddableState): AlertsEm
const state = storedState as AlertsEmbeddableState & {
slos?: Array<Record<string, unknown>>;
showAllGroupByInstances?: boolean;
show_all_group_by_instances?: boolean;
};
const { showAllGroupByInstances: _legacy, ...rest } = state;
const legacyShowAll =
state.show_all_group_by_instances ?? state.showAllGroupByInstances ?? false;
const { showAllGroupByInstances: _legacy1, show_all_group_by_instances: _legacy2, ...rest } =
state;
const slos =
state.slos?.map((slo) => {
const hasLegacy = 'id' in slo || 'instanceId' in slo || 'groupBy' in slo;
return hasLegacy ? mapLegacySloItem(slo) : (slo as SloItem);
return hasLegacy
? mapLegacySloItem(slo, legacyShowAll)
: (() => {
const item = slo as SloItem;
if (legacyShowAll && item.slo_instance_id && item.slo_instance_id !== ALL_VALUE) {
return { ...item, slo_instance_id: ALL_VALUE };
}
return item;
})();
}) ?? [];
const showAllGroupByInstances =
state.show_all_group_by_instances ?? state.showAllGroupByInstances ?? false;
return { ...rest, slos, show_all_group_by_instances: showAllGroupByInstances };
return { ...rest, slos };
}
4 changes: 1 addition & 3 deletions .../observability/plugins/slo/public/embeddable/slo/alerts/components/slo_alerts_summary.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,21 +23,19 @@ interface Props {
slos: SloItem[];
timeRange: TimeRange;
onLoaded?: () => void;
showAllGroupByInstances?: boolean;
}

export function SloAlertsSummary({
slos,
deps,
timeRange,
onLoaded,
showAllGroupByInstances,
}: Props) {
const {
triggersActionsUi: { getAlertSummaryWidget: AlertSummaryWidget },
} = deps;

const esQuery = useSloAlertsQuery(slos, timeRange, showAllGroupByInstances);
const esQuery = useSloAlertsQuery(slos, timeRange);
const timeBuckets = useTimeBuckets();
const bucketSize = useMemo(
() =>
Expand Down
22 changes: 6 additions & 16 deletions ...ns/observability/plugins/slo/public/embeddable/slo/alerts/components/slo_alerts_table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,14 +78,9 @@ interface Props {
timeRange: TimeRange;
onLoaded?: () => void;
lastReloadRequestTime: number | undefined;
showAllGroupByInstances?: boolean;
}

export const getSloInstanceFilter = (
sloId: string,
sloInstanceId: string,
showAllGroupByInstances = false
) => {
export const getSloInstanceFilter = (sloId: string, sloInstanceId: string) => {
return {
bool: {
must: [
Expand All @@ -94,7 +89,7 @@ export const getSloInstanceFilter = (
'slo.id': sloId,
},
},
...(sloInstanceId !== ALL_VALUE && !showAllGroupByInstances
...(sloInstanceId !== ALL_VALUE
? [
{
term: {
Expand All @@ -108,11 +103,7 @@ export const getSloInstanceFilter = (
};
};

export const useSloAlertsQuery = (
slos: SloItem[],
timeRange: TimeRange,
showAllGroupByInstances?: boolean
) => {
export const useSloAlertsQuery = (slos: SloItem[], timeRange: TimeRange) => {
return useMemo(() => {
if (slos.length === 0) {
return {
Expand Down Expand Up @@ -140,7 +131,7 @@ export const useSloAlertsQuery = (
{
bool: {
should: slos.map((slo) =>
getSloInstanceFilter(slo.slo_id, slo.slo_instance_id, showAllGroupByInstances)
getSloInstanceFilter(slo.slo_id, slo.slo_instance_id)
),
},
},
Expand All @@ -149,7 +140,7 @@ export const useSloAlertsQuery = (
};

return query;
}, [showAllGroupByInstances, slos, timeRange.from]);
}, [slos, timeRange.from]);
};

export function SloAlertsTable({
Expand All @@ -158,7 +149,6 @@ export function SloAlertsTable({
timeRange,
onLoaded,
lastReloadRequestTime,
showAllGroupByInstances,
}: Props) {
const ref = useRef<AlertsTableImperativeApi>(null);

Expand All @@ -172,7 +162,7 @@ export function SloAlertsTable({
id={ALERTS_TABLE_ID}
ruleTypeIds={[SLO_BURN_RATE_RULE_TYPE_ID]}
consumers={[AlertConsumers.SLO, AlertConsumers.ALERTS, AlertConsumers.OBSERVABILITY]}
query={useSloAlertsQuery(slos, timeRange, showAllGroupByInstances)}
query={useSloAlertsQuery(slos, timeRange)}
columns={columns}
hideLazyLoader
pageSize={ALERTS_PER_PAGE}
Expand Down
42 changes: 8 additions & 34 deletions .../observability/plugins/slo/public/embeddable/slo/alerts/slo_alerts_embeddable_factory.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import type { CoreStart } from '@kbn/core-lifecycle-browser';
import type { EmbeddableFactory } from '@kbn/embeddable-plugin/public';
import { i18n } from '@kbn/i18n';
import { ALL_VALUE } from '@kbn/slo-schema';
import { KibanaContextProvider } from '@kbn/kibana-react-plugin/public';
import { Storage } from '@kbn/kibana-utils-plugin/public';
import type { FetchContext } from '@kbn/presentation-publishing';
Expand Down Expand Up @@ -77,22 +76,9 @@ export function getAlertsEmbeddableFactory({
}

const titleManager = initializeTitleManager(initialState);
const hasSlosWithAllInstances = initialState?.slos?.some(
(slo) => slo.slo_instance_id === ALL_VALUE
);
const normalizedInitialState: AlertsCustomState = {
...initialState,
show_all_group_by_instances: hasSlosWithAllInstances
? true
: initialState?.show_all_group_by_instances ?? false,
};
const sloAlertsStateManager = initializeStateManager<AlertsCustomState>(
normalizedInitialState,
{
slos: [],
show_all_group_by_instances: false,
}
);
const sloAlertsStateManager = initializeStateManager<AlertsCustomState>(initialState, {
slos: [],
});
const defaultTitle$ = new BehaviorSubject<string | undefined>(getAlertsPanelTitle());
const reload$ = new Subject<FetchContext>();

Expand All @@ -117,7 +103,6 @@ export function getAlertsEmbeddableFactory({
...titleComparators,
...drilldownsManager.comparators,
slos: 'referenceEquality',
show_all_group_by_instances: 'referenceEquality',
}),
onReset: (lastSaved) => {
drilldownsManager.reinitializeState(lastSaved ?? {});
Expand All @@ -141,17 +126,10 @@ export function getAlertsEmbeddableFactory({
onEdit();
},
serializeState,
getSloAlertsConfig: () => {
return {
slos: sloAlertsStateManager.api.slos$.getValue(),
show_all_group_by_instances:
sloAlertsStateManager.api.showAllGroupByInstances$.getValue(),
};
},
updateSloAlertsConfig: (update) => {
sloAlertsStateManager.api.setSlos(update.slos);
sloAlertsStateManager.api.setShowAllGroupByInstances(update.show_all_group_by_instances);
},
getSloAlertsConfig: () => ({
slos: sloAlertsStateManager.api.slos$.getValue(),
}),
updateSloAlertsConfig: (update) => sloAlertsStateManager.api.setSlos(update.slos),
});

const fetchSubscription = fetch$(api)
Expand All @@ -163,10 +141,7 @@ export function getAlertsEmbeddableFactory({
return {
api,
Component: () => {
const [slos, showAllGroupByInstances] = useBatchedPublishingSubjects(
sloAlertsStateManager.api.slos$,
sloAlertsStateManager.api.showAllGroupByInstances$
);
const [slos] = useBatchedPublishingSubjects(sloAlertsStateManager.api.slos$);
const fetchContext = useFetchContext(api);
const I18nContext = deps.i18n.Context;

Expand Down Expand Up @@ -202,7 +177,6 @@ export function getAlertsEmbeddableFactory({
slos={slos}
timeRange={fetchContext.timeRange ?? { from: 'now-15m/m', to: 'now' }}
reloadSubject={reload$}
showAllGroupByInstances={showAllGroupByInstances}
/>
</QueryClientProvider>
</PluginContext.Provider>
Expand Down
Loading