From 3c28a3d494bc78f2ac29eff023f256f428ca8918 Mon Sep 17 00:00:00 2001 From: Brendan O'Handley Date: Thu, 28 Mar 2024 12:22:54 -0500 Subject: [PATCH] Graphite: second function as another function argument parsing error fix (#85224) * update language around query editor parsing issues * add special handling for second function arguments in divideSeriesLists * additional check for first argument as series(string) and not a function --- .../graphite/query-editor/index.md | 4 + .../datasource/graphite/graphite_query.ts | 66 +++++++++ .../graphite/specs/graphite_query.test.ts | 136 +++++++++++++++--- 3 files changed, 183 insertions(+), 23 deletions(-) diff --git a/docs/sources/datasources/graphite/query-editor/index.md b/docs/sources/datasources/graphite/query-editor/index.md index 74e0dd6bbda..098a98b2b94 100644 --- a/docs/sources/datasources/graphite/query-editor/index.md +++ b/docs/sources/datasources/graphite/query-editor/index.md @@ -55,6 +55,10 @@ Some functions like aliasByNode support an optional second argument. To add an a To learn more, refer to [Graphite's documentation on functions](https://graphite.readthedocs.io/en/latest/functions.html). +{{% admonition type="warning" %}} +Some functions take a second argument that may be a function that returns a series. If you are adding a second argument that is a function, it is suggested to use a series reference from a second query instead of the function itself. The query editor does not currently support parsing of a second argument that is a function when switching between the query editor and the code editor. +{{% /admonition %}} + ### Sort labels If you have the same labels on multiple graphs, they are both sorted differently and use different colors. diff --git a/public/app/plugins/datasource/graphite/graphite_query.ts b/public/app/plugins/datasource/graphite/graphite_query.ts index fbf5f9bf32f..94e2d7db4ef 100644 --- a/public/app/plugins/datasource/graphite/graphite_query.ts +++ b/public/app/plugins/datasource/graphite/graphite_query.ts @@ -114,6 +114,8 @@ export default class GraphiteQuery { // bug fix for parsing multiple functions as params handleMultipleSeriesByTagsParams(astNode); + handleDivideSeriesListsNestedFunctions(astNode); + each(astNode.params, (param) => { this.parseTargetRecursive(param, innerFunc); }); @@ -367,3 +369,67 @@ function handleMultipleSeriesByTagsParams(astNode: AstNode) { }); } } + +/** + * Converts all nested functions as parametors (recursively) to strings + */ +function handleDivideSeriesListsNestedFunctions(astNode: AstNode) { + // if divideSeriesLists function, the second parameters should be strings + if (astNode.name === 'divideSeriesLists' && astNode.params && astNode.params.length >= 2) { + astNode.params = astNode.params.map((p: AstNode, idx: number) => { + if (idx === 1 && p.type === 'function') { + // convert nested 2nd functions as parametors to a strings + // all nested functions should be strings + // if the node is a function it will have params + // if these params are functions, they will have params + // at some point we will have to add the params as strings + // then wrap them in the function + let functionString = ''; + let s = p.name + '(' + nestedFunctionsToString(p, functionString); + + p = { + type: 'string', + value: s, + }; + } + + return p; + }); + } + + return astNode; +} + +function nestedFunctionsToString(node: AstNode, functionString: string): string | undefined { + let count = 0; + if (node.params) { + count++; + + const paramsLength = node.params?.length ?? 0; + + node.params.forEach((innerNode: AstNode, idx: number) => { + if (idx < paramsLength - 1) { + functionString += switchCase(innerNode, functionString) + ','; + } else { + functionString += switchCase(innerNode, functionString); + } + }); + + return functionString + ')'; + } else { + return (functionString += switchCase(node, functionString)); + } +} + +function switchCase(node: AstNode, functionString: string) { + switch (node.type) { + case 'function': + functionString += node.name + '('; + return nestedFunctionsToString(node, functionString); + case 'metric': + const segmentString = join(map(node.segments, 'value'), '.'); + return segmentString; + default: + return node.value; + } +} diff --git a/public/app/plugins/datasource/graphite/specs/graphite_query.test.ts b/public/app/plugins/datasource/graphite/specs/graphite_query.test.ts index 014a2189bea..a02456b64a9 100644 --- a/public/app/plugins/datasource/graphite/specs/graphite_query.test.ts +++ b/public/app/plugins/datasource/graphite/specs/graphite_query.test.ts @@ -111,11 +111,11 @@ describe('Graphite query model', () => { }); }); - describe('when query has multiple seriesByTags functions as parameters it updates the model target correctly', () => { + describe('When the second parameter of a function is a function, the graphite parser breaks', () => { /* all functions that take parameters as functions can have a bug where writing a query - in code with two seriesByTags funcs as params and then - switching from code to builder parsers the second function in a way that + in code where the second parameter of the function IS A FUNCTION, + then switching from code to builder parsers the second function in a way that changes the order of the params and wraps the first param in the second param. asPercent(seriesByTag('namespace=asd'), (seriesByTag('namespace=fgh')) @@ -126,33 +126,123 @@ describe('Graphite query model', () => { where each function is wrapped in another function https://github.com/grafana/grafana/blob/main/public/app/plugins/datasource/graphite/graphite_query.ts#LL187C8-L187C8 - Parsing the second seriesByTag function as param as a string fixes this issue + Parsing the second "function as param" as a string fixes this issue This is one of the edge cases that could be a reason for either refactoring or rebuilding the Graphite query builder */ - beforeEach(() => { - ctx.target = { refId: 'A', target: `asPercent(seriesByTag('namespace=asd'), seriesByTag('namespace=fgh'))` }; - ctx.targets = [ctx.target]; - ctx.queryModel = new GraphiteQuery(ctx.datasource, ctx.target, ctx.templateSrv); + describe('when query has multiple seriesByTags functions as parameters it updates the model target correctly', () => { + beforeEach(() => { + ctx.target = { refId: 'A', target: `asPercent(seriesByTag('namespace=asd'), seriesByTag('namespace=fgh'))` }; + ctx.targets = [ctx.target]; + ctx.queryModel = new GraphiteQuery(ctx.datasource, ctx.target, ctx.templateSrv); + }); + + it('should parse the second function param as a string and not a second function', () => { + const targets = [ + { + refId: 'A', + datasource: { + type: 'graphite', + uid: 'zzz', + }, + target: "asPercent(seriesByTag('namespace=jkl'), seriesByTag('namespace=fgh'))", + textEditor: false, + key: '123', + }, + ]; + expect(ctx.queryModel.segments.length).toBe(0); + expect(ctx.queryModel.functions.length).toBe(2); + ctx.queryModel.updateModelTarget(targets); + expect(ctx.queryModel.target.target).not.toContain('seriesByTag(seriesByTag('); + }); }); - it('should parse the second function param as a string and not a second function', () => { - const targets = [ - { + describe('when query has divideSeriesLists function where second parameter is a function is parses correctly', () => { + it('should parse the second function param as a string and not parse it as a second function', () => { + const functionAsParam = 'scaleToSeconds(carbon.agents.0df7e0ba2701-a.cache.queries,1)'; + + ctx.target = { refId: 'A', - datasource: { - type: 'graphite', - uid: 'zzz', + target: `divideSeriesLists(scaleToSeconds(nonNegativeDerivative(carbon.agents.0df7e0ba2701-a.cache.queries), 1), ${functionAsParam})`, + }; + ctx.targets = [ctx.target]; + ctx.queryModel = new GraphiteQuery(ctx.datasource, ctx.target, ctx.templateSrv); + + const targets = [ + { + refId: 'A', + datasource: { + type: 'graphite', + uid: 'zzz', + }, + target: `divideSeriesLists(scaleToSeconds(nonNegativeDerivative(carbon.agents.0df7e0ba2701-a.cache.queries), 1), ${functionAsParam})`, + textEditor: false, + key: '123', }, - target: "asPercent(seriesByTag('namespace=jkl'), seriesByTag('namespace=fgh'))", - textEditor: false, - key: '123', - }, - ]; - expect(ctx.queryModel.segments.length).toBe(0); - expect(ctx.queryModel.functions.length).toBe(2); - ctx.queryModel.updateModelTarget(targets); - expect(ctx.queryModel.target.target).not.toContain('seriesByTag(seriesByTag('); + ]; + expect(ctx.queryModel.segments.length).toBe(5); + expect(ctx.queryModel.functions.length).toBe(3); + ctx.queryModel.updateModelTarget(targets); + expect(ctx.queryModel.target.target).toContain(functionAsParam); + }); + + it('should recursively parse a second function argument that contains another function as a string', () => { + const nestedFunctionAsParam = + 'scaleToSeconds(nonNegativeDerivative(carbon.agents.0df7e0ba2701-a.cache.queries,1))'; + + ctx.target = { + refId: 'A', + target: `divideSeriesLists(scaleToSeconds(nonNegativeDerivative(carbon.agents.0df7e0ba2701-a.cache.queries), 1), ${nestedFunctionAsParam})`, + }; + ctx.targets = [ctx.target]; + ctx.queryModel = new GraphiteQuery(ctx.datasource, ctx.target, ctx.templateSrv); + + const targets = [ + { + refId: 'A', + datasource: { + type: 'graphite', + uid: 'zzz', + }, + target: `divideSeriesLists(scaleToSeconds(nonNegativeDerivative(carbon.agents.0df7e0ba2701-a.cache.queries), 1), ${nestedFunctionAsParam})`, + textEditor: false, + key: '123', + }, + ]; + expect(ctx.queryModel.segments.length).toBe(5); + expect(ctx.queryModel.functions.length).toBe(3); + ctx.queryModel.updateModelTarget(targets); + expect(ctx.queryModel.target.target).toContain(nestedFunctionAsParam); + }); + + it('should recursively parse a second function argument where the first argument is a series', () => { + const nestedFunctionAsParam = + 'scaleToSeconds(nonNegativeDerivative(carbon.agents.0df7e0ba2701-a.cache.queries,1))'; + + ctx.target = { + refId: 'A', + target: `divideSeriesLists(carbon.agents.0df7e0ba2701-a.cache.queries, ${nestedFunctionAsParam})`, + }; + ctx.targets = [ctx.target]; + ctx.queryModel = new GraphiteQuery(ctx.datasource, ctx.target, ctx.templateSrv); + + const targets = [ + { + refId: 'A', + datasource: { + type: 'graphite', + uid: 'zzz', + }, + target: `divideSeriesLists(scaleToSeconds(nonNegativeDerivative(carbon.agents.0df7e0ba2701-a.cache.queries), 1), ${nestedFunctionAsParam})`, + textEditor: false, + key: '123', + }, + ]; + expect(ctx.queryModel.segments.length).toBe(5); + expect(ctx.queryModel.functions.length).toBe(1); + ctx.queryModel.updateModelTarget(targets); + expect(ctx.queryModel.target.target).toContain(nestedFunctionAsParam); + }); }); }); });