From 5d8ec2756e3d10b75f0ab3d18a64839eecbcd4d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A1bor=20Farkas?= Date: Thu, 16 Mar 2023 08:54:32 +0100 Subject: [PATCH] loki: query chunking: better error handling (#64811) * loki: query chunking: better error handling * better comment text Co-authored-by: Matias Chomicki * better comment text Co-authored-by: Matias Chomicki * changed merge approach * simplified code * fixed test * removed forgotten code --------- Co-authored-by: Matias Chomicki --- .../datasource/loki/querySplitting.test.ts | 2 +- .../plugins/datasource/loki/querySplitting.ts | 6 +- .../datasource/loki/queryUtils.test.ts | 69 +++++++++++++++++++ .../app/plugins/datasource/loki/queryUtils.ts | 16 +++++ 4 files changed, 89 insertions(+), 4 deletions(-) diff --git a/public/app/plugins/datasource/loki/querySplitting.test.ts b/public/app/plugins/datasource/loki/querySplitting.test.ts index 717024dca61..0a0366928eb 100644 --- a/public/app/plugins/datasource/loki/querySplitting.test.ts +++ b/public/app/plugins/datasource/loki/querySplitting.test.ts @@ -42,7 +42,7 @@ describe('runPartitionedQueries()', () => { .spyOn(datasource, 'runQuery') .mockReturnValue(of({ state: LoadingState.Error, error: { refId: 'A', message: 'Error' }, data: [] })); await expect(runPartitionedQueries(datasource, request)).toEmitValuesWith((values) => { - expect(values).toEqual([{ refId: 'A', message: 'Error' }]); + expect(values).toEqual([{ error: { refId: 'A', message: 'Error' }, data: [], state: LoadingState.Streaming }]); }); }); diff --git a/public/app/plugins/datasource/loki/querySplitting.ts b/public/app/plugins/datasource/loki/querySplitting.ts index 3f0df310c25..c22fc08fcb6 100644 --- a/public/app/plugins/datasource/loki/querySplitting.ts +++ b/public/app/plugins/datasource/loki/querySplitting.ts @@ -131,10 +131,10 @@ export function runGroupedQueries(datasource: LokiDatasource, requests: LokiGrou .runQuery({ ...requests[requestGroup].request, range, requestId, targets }) .subscribe({ next: (partialResponse) => { - if (partialResponse.error) { - subscriber.error(partialResponse.error); - } mergedResponse = combineResponses(mergedResponse, partialResponse); + if ((mergedResponse.errors ?? []).length > 0 || mergedResponse.error != null) { + shouldStop = true; + } }, complete: () => { subscriber.next(mergedResponse); diff --git a/public/app/plugins/datasource/loki/queryUtils.test.ts b/public/app/plugins/datasource/loki/queryUtils.test.ts index 047e1c97a6b..c450329ae00 100644 --- a/public/app/plugins/datasource/loki/queryUtils.test.ts +++ b/public/app/plugins/datasource/loki/queryUtils.test.ts @@ -476,6 +476,75 @@ describe('combineResponses', () => { expect(combineResponses(null, responseB)).not.toBe(responseB); }); + it('combine when first param has errors', () => { + const { metricFrameA, metricFrameB } = getMockFrames(); + const errorA = { + message: 'errorA', + }; + const responseA: DataQueryResponse = { + data: [metricFrameA], + error: errorA, + errors: [errorA], + }; + const responseB: DataQueryResponse = { + data: [metricFrameB], + }; + + const combined = combineResponses(responseA, responseB); + expect(combined.data[0].length).toBe(4); + expect(combined.error?.message).toBe('errorA'); + expect(combined.errors).toHaveLength(1); + expect(combined.errors?.[0]?.message).toBe('errorA'); + }); + + it('combine when second param has errors', () => { + const { metricFrameA, metricFrameB } = getMockFrames(); + const responseA: DataQueryResponse = { + data: [metricFrameA], + }; + const errorB = { + message: 'errorB', + }; + const responseB: DataQueryResponse = { + data: [metricFrameB], + error: errorB, + errors: [errorB], + }; + + const combined = combineResponses(responseA, responseB); + expect(combined.data[0].length).toBe(4); + expect(combined.error?.message).toBe('errorB'); + expect(combined.errors).toHaveLength(1); + expect(combined.errors?.[0]?.message).toBe('errorB'); + }); + + it('combine when both params have errors', () => { + const { metricFrameA, metricFrameB } = getMockFrames(); + const errorA = { + message: 'errorA', + }; + const errorB = { + message: 'errorB', + }; + const responseA: DataQueryResponse = { + data: [metricFrameA], + error: errorA, + errors: [errorA], + }; + const responseB: DataQueryResponse = { + data: [metricFrameB], + error: errorB, + errors: [errorB], + }; + + const combined = combineResponses(responseA, responseB); + expect(combined.data[0].length).toBe(4); + expect(combined.error?.message).toBe('errorA'); + expect(combined.errors).toHaveLength(2); + expect(combined.errors?.[0]?.message).toBe('errorA'); + expect(combined.errors?.[1]?.message).toBe('errorB'); + }); + describe('combine stats', () => { const { metricFrameA } = getMockFrames(); const makeResponse = (stats?: QueryResultMetaStat[]): DataQueryResponse => ({ diff --git a/public/app/plugins/datasource/loki/queryUtils.ts b/public/app/plugins/datasource/loki/queryUtils.ts index c63183f0329..d68953cb06e 100644 --- a/public/app/plugins/datasource/loki/queryUtils.ts +++ b/public/app/plugins/datasource/loki/queryUtils.ts @@ -335,6 +335,22 @@ export function combineResponses(currentResult: DataQueryResponse | null, newRes combineFrames(currentFrame, newFrame); }); + const mergedErrors = [...(currentResult.errors ?? []), ...(newResult.errors ?? [])]; + + // we make sure to have `.errors` as undefined, instead of empty-array + // when no errors. + + if (mergedErrors.length > 0) { + currentResult.errors = mergedErrors; + } + + // the `.error` attribute is obsolete now, + // but we have to maintain it, otherwise + // some grafana parts do not behave well. + // we just choose the old error, if it exists, + // otherwise the new error, if it exists. + currentResult.error = currentResult.error ?? newResult.error; + return currentResult; }