Skip to content
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.

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

Use groupBy when groupings is not populated correctly #189672

Merged
merged 10 commits into from
Sep 19, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@
* 2.0.
*/
import * as t from 'io-ts';
import { allOrAnyStringOrArray } from '../../schema';

const getSLOInstancesParamsSchema = t.type({
path: t.type({ id: t.string }),
});

const getSLOInstancesResponseSchema = t.type({
groupBy: t.union([t.string, t.array(t.string)]),
groupBy: allOrAnyStringOrArray,
instances: t.array(t.string),
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/
import * as t from 'io-ts';
import { indicatorSchema, objectiveSchema } from '../../schema';
import { dateType } from '../../schema/common';
import { allOrAnyStringOrArray, dateType } from '../../schema/common';

const getPreviewDataParamsSchema = t.type({
body: t.intersection([
Expand All @@ -20,7 +20,7 @@ const getPreviewDataParamsSchema = t.type({
t.partial({
objective: objectiveSchema,
instanceId: t.string,
groupBy: t.string,
groupBy: allOrAnyStringOrArray,
remoteName: t.string,
groupings: t.record(t.string, t.unknown),
}),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export function useGetPreviewData({
remoteName,
}: {
isValid: boolean;
groupBy?: string;
groupBy?: string | string[];
instanceId?: string;
remoteName?: string;
groupings?: Record<string, unknown>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ export function EventsChartPanel({ slo, range, selectedTabId, onBrushed }: Props
const { isLoading, data } = useGetPreviewData({
range,
isValid: true,
groupBy: slo.groupBy,
indicator: slo.indicator,
groupings: slo.groupings,
instanceId: slo.instanceId,
Expand Down
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 {
getSLOPipelineId,
SLO_INGEST_PIPELINE_INDEX_NAME_PREFIX,
Expand Down Expand Up @@ -43,47 +44,24 @@ export const getSLOPipelineTemplate = (slo: SLODefinition) => ({
},
},
{
script: {
description: 'Generated the instanceId field for SLO rollup data',
source: `
// This function will recursively collect all the values of a HashMap of HashMaps
Collection collectValues(HashMap subject) {
Collection values = new ArrayList();
// Iterate through the values
for(Object value: subject.values()) {
// If the value is a HashMap, recurse
if (value instanceof HashMap) {
values.addAll(collectValues((HashMap) value));
} else {
values.add(String.valueOf(value));
}
}
return values;
}

// Create the string builder
StringBuilder instanceId = new StringBuilder();

if (ctx["slo"]["groupings"] == null) {
ctx["slo"]["instanceId"] = "*";
} else {
// Get the values as a collection
Collection values = collectValues(ctx["slo"]["groupings"]);

// Convert to a list and sort
List sortedValues = new ArrayList(values);
Collections.sort(sortedValues);

// Create comma delimited string
for(String instanceValue: sortedValues) {
instanceId.append(instanceValue);
instanceId.append(",");
}

// Assign the slo.instanceId
ctx["slo"]["instanceId"] = instanceId.length() > 0 ? instanceId.substring(0, instanceId.length() - 1) : "*";
}
`,
dot_expander: {
path: 'slo.groupings',
field: '*',
ignore_failure: true,
if: 'ctx.slo.groupings != null',
},
},
{
set: {
description: 'Generated the instanceId field based on the groupings field',
field: 'slo.instanceId',
value:
[slo.groupBy].flat().includes(ALL_VALUE) || [slo.groupBy].flat().length === 0
? ALL_VALUE
: [slo.groupBy]
.flat()
.map((field) => `{{{slo.groupings.${field}}}}`)
.join(','),
},
},
],
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ interface Options {
interval: string;
instanceId?: string;
remoteName?: string;
groupBy?: string;
groupBy?: string | string[];
groupings?: Record<string, unknown>;
}
export class GetPreviewData {
Expand Down Expand Up @@ -516,15 +516,20 @@ export class GetPreviewData {

private getGroupingsFilter(options: Options, filter: estypes.QueryDslQueryContainer[]) {
const groupingsKeys = Object.keys(options.groupings || []);

if (groupingsKeys.length) {
groupingsKeys.forEach((key) => {
filter.push({
term: { [key]: options.groupings?.[key] },
});
});
} else if (options.instanceId !== ALL_VALUE && options.groupBy) {
filter.push({
term: { [options.groupBy]: options.instanceId },
} else if (options.instanceId && options.instanceId !== ALL_VALUE && options.groupBy) {
const instanceIdPart = options.instanceId.split(',');
const groupByPart = Array.isArray(options.groupBy) ? options.groupBy : [options.groupBy];
groupByPart.forEach((groupBy, index) => {
filter.push({
term: { [groupBy]: instanceIdPart[index] },
});
Comment on lines +526 to +532
Copy link
Member

@simianhacker simianhacker Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum... when the slo.instanceId is created, we sort the values to keep them stable. It's not guaranteed that the first value will match the first field.

I wonder if we shouldn't change the ingest pipeline to ensure the first value matches first field, the second value matches the second field, and so on... To do this, we could simplify the ingest pipeline by getting rid of the painless script and replace it with:

   {
     dot_expander: {
       path: 'slo.groupings',
       field: '*',
     },
   },
   {
     set: {
       field: 'entity.id',
       value: [slo.groupBy]
         .flat()
         .map((field) => `{{{slo.groupings.${field}}}}`)
         .join(','),
     },
   },

This would ensure that first value in the slo.instanceId would map back to the first field in the group by list. Since this quirk only affects SLOs who's data stopped reporting and were multi-group-by-values (probably a small subset of our users), it would work for most everyone else. Anyone who had a multi-group-by slo.instanceId that had an issue could use the _reset command to get the new ingest pipeline and things would start working as expected.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a good approach. I like the simplification. We probably need to handle the case where groupBy is * but otherwise looks good to me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I came up with this 2 processors to handle no groupings. Local testing seems to work fine without group, with 1 and many groups

 {
      dot_expander: {
        path: 'slo.groupings',
        field: '*',
        ignore_failure: true,
        if: 'ctx.slo.groupings != null',
      },
    },
    {
      set: {
        description: 'Generated the instanceId field based on the groupings field',
        field: 'slo.instanceId',
        value: [slo.groupBy].flat().includes(ALL_VALUE)
          ? ALL_VALUE
          : [slo.groupBy]
              .flat()
              .map((field) => `{{{slo.groupings.${field}}}}`)
              .join(','),
      },
    },

});
}
}
Expand Down