From 721c50a304588ebd7cea76e301ec0f68a5a55d68 Mon Sep 17 00:00:00 2001 From: Nick Richmond <5732000+NWRichmond@users.noreply.github.com> Date: Wed, 18 Dec 2024 16:31:08 -0500 Subject: [PATCH] Prometheus: Improve handling of special chars in label values (#96067) * fix: handling of special chars * docs: add clarity * fix: escaping * refactor: put changes behind new feature toggle * docs: use consistent comment style * refactor: rename feature toggle for brevity * use single quotes * fix unit tests * remove redundant json entry * fix: keep all changes behind feature toggle * fix: support builder mode * fix: don't escape when using regex operators * fix: code mode label values completions with special chars * refactor: remove unneeded changes * move feature toggle up so new changes from main won't conflict with ours * fix: escape label values in metric select scene * refactor: ensure changes are behind feature toggle --------- Co-authored-by: ismail simsek --- .../feature-toggles/index.md | 1 + .../src/types/featureToggles.gen.ts | 1 + .../completions.test.ts | 164 +++++++++ .../monaco-completion-provider/completions.ts | 8 +- .../grafana-prometheus/src/datasource.test.ts | 330 +++++++++++++----- packages/grafana-prometheus/src/datasource.ts | 39 ++- .../shared/LokiAndPromQueryModellerBase.ts | 10 +- pkg/services/featuremgmt/registry.go | 7 + pkg/services/featuremgmt/toggles_gen.csv | 1 + pkg/services/featuremgmt/toggles_gen.go | 4 + pkg/services/featuremgmt/toggles_gen.json | 13 + .../app/features/trails/MetricSelect/api.ts | 14 +- public/app/features/trails/otel/util.ts | 18 +- public/app/features/trails/otel/utils.test.ts | 12 +- 14 files changed, 513 insertions(+), 109 deletions(-) diff --git a/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md b/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md index afe3cc88be5..5ce3a34a4e6 100644 --- a/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md +++ b/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md @@ -222,6 +222,7 @@ Experimental features might be changed or removed without prior notice. | `userStorageAPI` | Enables the user storage API | | `dashboardSchemaV2` | Enables the new dashboard schema version 2, implementing changes necessary for dynamic dashboards and dashboards as code. | | `playlistsWatcher` | Enables experimental watcher for playlists | +| `prometheusSpecialCharsInLabelValues` | Adds support for quotes and special characters in label values for Prometheus queries | | `enableExtensionsAdminPage` | Enables the extension admin page regardless of development mode | | `enableSCIM` | Enables SCIM support for user and group management | | `crashDetection` | Enables browser crash detection reporting to Faro. | diff --git a/packages/grafana-data/src/types/featureToggles.gen.ts b/packages/grafana-data/src/types/featureToggles.gen.ts index 169a99dfc6f..7783296f507 100644 --- a/packages/grafana-data/src/types/featureToggles.gen.ts +++ b/packages/grafana-data/src/types/featureToggles.gen.ts @@ -232,6 +232,7 @@ export interface FeatureToggles { playlistsWatcher?: boolean; passwordlessMagicLinkAuthentication?: boolean; exploreMetricsRelatedLogs?: boolean; + prometheusSpecialCharsInLabelValues?: boolean; enableExtensionsAdminPage?: boolean; zipkinBackendMigration?: boolean; enableSCIM?: boolean; diff --git a/packages/grafana-prometheus/src/components/monaco-query-field/monaco-completion-provider/completions.test.ts b/packages/grafana-prometheus/src/components/monaco-query-field/monaco-completion-provider/completions.test.ts index 32bfcb14efc..7a97ba98740 100644 --- a/packages/grafana-prometheus/src/components/monaco-query-field/monaco-completion-provider/completions.test.ts +++ b/packages/grafana-prometheus/src/components/monaco-query-field/monaco-completion-provider/completions.test.ts @@ -274,3 +274,167 @@ describe.each(metricNameCompletionSituations)('metric name completions in situat expect(completions.length).toBeLessThanOrEqual(expectedCompletionsCount); }); }); + +describe('Label value completions', () => { + let dataProvider: DataProvider; + + beforeEach(() => { + dataProvider = { + getAllMetricNames: jest.fn(), + metricNamesToMetrics: jest.fn(), + getHistory: jest.fn(), + getLabelNames: jest.fn(), + getLabelValues: jest.fn().mockResolvedValue(['value1', 'value"2', 'value\\3', "value'4"]), + getSeriesLabels: jest.fn(), + getSeriesValues: jest.fn(), + monacoSettings: { + setInputInRange: jest.fn(), + inputInRange: '', + suggestionsIncomplete: false, + enableAutocompleteSuggestionsUpdate: jest.fn(), + }, + metricNamesSuggestionLimit: 100, + } as unknown as DataProvider; + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + describe('with prometheusSpecialCharsInLabelValues disabled', () => { + beforeEach(() => { + jest.replaceProperty(config, 'featureToggles', { + prometheusSpecialCharsInLabelValues: false, + }); + }); + + it('should not escape special characters when between quotes', async () => { + const situation: Situation = { + type: 'IN_LABEL_SELECTOR_WITH_LABEL_NAME', + labelName: 'testLabel', + betweenQuotes: true, + otherLabels: [], + }; + + const completions = await getCompletions(situation, dataProvider); + + expect(completions).toHaveLength(4); + expect(completions[0].insertText).toBe('value1'); + expect(completions[1].insertText).toBe('value"2'); + expect(completions[2].insertText).toBe('value\\3'); + expect(completions[3].insertText).toBe("value'4"); + }); + + it('should wrap in quotes but not escape special characters when not between quotes', async () => { + const situation: Situation = { + type: 'IN_LABEL_SELECTOR_WITH_LABEL_NAME', + labelName: 'testLabel', + betweenQuotes: false, + otherLabels: [], + }; + + const completions = await getCompletions(situation, dataProvider); + + expect(completions).toHaveLength(4); + expect(completions[0].insertText).toBe('"value1"'); + expect(completions[1].insertText).toBe('"value"2"'); + expect(completions[2].insertText).toBe('"value\\3"'); + expect(completions[3].insertText).toBe('"value\'4"'); + }); + }); + + describe('with prometheusSpecialCharsInLabelValues enabled', () => { + beforeEach(() => { + jest.replaceProperty(config, 'featureToggles', { + prometheusSpecialCharsInLabelValues: true, + }); + }); + + it('should escape special characters when between quotes', async () => { + const situation: Situation = { + type: 'IN_LABEL_SELECTOR_WITH_LABEL_NAME', + labelName: 'testLabel', + betweenQuotes: true, + otherLabels: [], + }; + + const completions = await getCompletions(situation, dataProvider); + + expect(completions).toHaveLength(4); + expect(completions[0].insertText).toBe('value1'); + expect(completions[1].insertText).toBe('value\\"2'); + expect(completions[2].insertText).toBe('value\\\\3'); + expect(completions[3].insertText).toBe("value'4"); + }); + + it('should wrap in quotes and escape special characters when not between quotes', async () => { + const situation: Situation = { + type: 'IN_LABEL_SELECTOR_WITH_LABEL_NAME', + labelName: 'testLabel', + betweenQuotes: false, + otherLabels: [], + }; + + const completions = await getCompletions(situation, dataProvider); + + expect(completions).toHaveLength(4); + expect(completions[0].insertText).toBe('"value1"'); + expect(completions[1].insertText).toBe('"value\\"2"'); + expect(completions[2].insertText).toBe('"value\\\\3"'); + expect(completions[3].insertText).toBe('"value\'4"'); + }); + }); + + describe('label value escaping edge cases', () => { + beforeEach(() => { + jest.replaceProperty(config, 'featureToggles', { + prometheusSpecialCharsInLabelValues: true, + }); + }); + + it('should handle empty values', async () => { + jest.spyOn(dataProvider, 'getLabelValues').mockResolvedValue(['']); + + const situation: Situation = { + type: 'IN_LABEL_SELECTOR_WITH_LABEL_NAME', + labelName: 'testLabel', + betweenQuotes: false, + otherLabels: [], + }; + + const completions = await getCompletions(situation, dataProvider); + expect(completions).toHaveLength(1); + expect(completions[0].insertText).toBe('""'); + }); + + it('should handle values with multiple special characters', async () => { + jest.spyOn(dataProvider, 'getLabelValues').mockResolvedValue(['test"\\value']); + + const situation: Situation = { + type: 'IN_LABEL_SELECTOR_WITH_LABEL_NAME', + labelName: 'testLabel', + betweenQuotes: true, + otherLabels: [], + }; + + const completions = await getCompletions(situation, dataProvider); + expect(completions).toHaveLength(1); + expect(completions[0].insertText).toBe('test\\"\\\\value'); + }); + + it('should handle non-string values', async () => { + jest.spyOn(dataProvider, 'getLabelValues').mockResolvedValue([123 as unknown as string]); + + const situation: Situation = { + type: 'IN_LABEL_SELECTOR_WITH_LABEL_NAME', + labelName: 'testLabel', + betweenQuotes: false, + otherLabels: [], + }; + + const completions = await getCompletions(situation, dataProvider); + expect(completions).toHaveLength(1); + expect(completions[0].insertText).toBe('"123"'); + }); + }); +}); diff --git a/packages/grafana-prometheus/src/components/monaco-query-field/monaco-completion-provider/completions.ts b/packages/grafana-prometheus/src/components/monaco-query-field/monaco-completion-provider/completions.ts index 95c16738d55..1060c8df90a 100644 --- a/packages/grafana-prometheus/src/components/monaco-query-field/monaco-completion-provider/completions.ts +++ b/packages/grafana-prometheus/src/components/monaco-query-field/monaco-completion-provider/completions.ts @@ -3,6 +3,7 @@ import UFuzzy from '@leeoniya/ufuzzy'; import { config } from '@grafana/runtime'; +import { prometheusRegularEscape } from '../../../datasource'; import { escapeLabelValueInExactSelector } from '../../../language_utils'; import { FUNCTIONS } from '../../../promql'; @@ -208,10 +209,15 @@ async function getLabelValuesForMetricCompletions( return values.map((text) => ({ type: 'LABEL_VALUE', label: text, - insertText: betweenQuotes ? text : `"${text}"`, // FIXME: escaping strange characters? + insertText: formatLabelValueForCompletion(text, betweenQuotes), })); } +function formatLabelValueForCompletion(value: string, betweenQuotes: boolean): string { + const text = config.featureToggles.prometheusSpecialCharsInLabelValues ? prometheusRegularEscape(value) : value; + return betweenQuotes ? text : `"${text}"`; +} + export function getCompletions(situation: Situation, dataProvider: DataProvider): Promise { switch (situation.type) { case 'IN_DURATION': diff --git a/packages/grafana-prometheus/src/datasource.test.ts b/packages/grafana-prometheus/src/datasource.test.ts index a8da9fa6ec0..54cdf88f8c6 100644 --- a/packages/grafana-prometheus/src/datasource.test.ts +++ b/packages/grafana-prometheus/src/datasource.test.ts @@ -245,60 +245,118 @@ describe('PrometheusDatasource', () => { const DEFAULT_QUERY_EXPRESSION = 'metric{job="foo"} - metric'; const target: PromQuery = { expr: DEFAULT_QUERY_EXPRESSION, refId: 'A' }; - it('should not modify expression with no filters', async () => { - ds.query({ - interval: '15s', - range: getMockTimeRange(), - targets: [target], - } as DataQueryRequest); - const [result] = fetchMockCalledWith(fetchMock); - expect(result).toMatchObject({ expr: DEFAULT_QUERY_EXPRESSION }); + describe('with prometheusSpecialCharsInLabelValues disabled', () => { + beforeAll(() => { + config.featureToggles.prometheusSpecialCharsInLabelValues = false; + }); + + it('should not modify expression with no filters', async () => { + ds.query({ + interval: '15s', + range: getMockTimeRange(), + targets: [target], + } as DataQueryRequest); + const [result] = fetchMockCalledWith(fetchMock); + expect(result).toMatchObject({ expr: DEFAULT_QUERY_EXPRESSION }); + }); + + it('should add filters to expression', () => { + const filters = [ + { + key: 'k1', + operator: '=', + value: 'v1', + }, + { + key: 'k2', + operator: '!=', + value: 'v2', + }, + ]; + ds.query({ + interval: '15s', + range: getMockTimeRange(), + filters, + targets: [target], + } as DataQueryRequest); + const [result] = fetchMockCalledWith(fetchMock); + expect(result).toMatchObject({ expr: 'metric{job="foo", k1="v1", k2!="v2"} - metric{k1="v1", k2!="v2"}' }); + }); + + it('should add escaping if needed to regex filter expressions', () => { + const filters = [ + { + key: 'k1', + operator: '=~', + value: 'v.*', + }, + { + key: 'k2', + operator: '=~', + value: `v'.*`, + }, + ]; + ds.query({ + interval: '15s', + range: getMockTimeRange(), + filters, + targets: [target], + } as DataQueryRequest); + const [result] = fetchMockCalledWith(fetchMock); + expect(result).toMatchObject({ + expr: `metric{job="foo", k1=~"v.*", k2=~"v\\\\'.*"} - metric{k1=~"v.*", k2=~"v\\\\'.*"}`, + }); + }); }); - it('should add filters to expression', () => { - const filters = [ - { - key: 'k1', - operator: '=', - value: 'v1', - }, - { - key: 'k2', - operator: '!=', - value: 'v2', - }, - ]; - ds.query({ - interval: '15s', - range: getMockTimeRange(), - filters, - targets: [target], - } as DataQueryRequest); - const [result] = fetchMockCalledWith(fetchMock); - expect(result).toMatchObject({ expr: 'metric{job="foo", k1="v1", k2!="v2"} - metric{k1="v1", k2!="v2"}' }); - }); - it('should add escaping if needed to regex filter expressions', () => { - const filters = [ - { - key: 'k1', - operator: '=~', - value: 'v.*', - }, - { - key: 'k2', - operator: '=~', - value: `v'.*`, - }, - ]; - ds.query({ - interval: '15s', - range: getMockTimeRange(), - filters, - targets: [target], - } as DataQueryRequest); - const [result] = fetchMockCalledWith(fetchMock); - expect(result).toMatchObject({ - expr: `metric{job="foo", k1=~"v.*", k2=~"v\\\\'.*"} - metric{k1=~"v.*", k2=~"v\\\\'.*"}`, + describe('with prometheusSpecialCharsInLabelValues enabled', () => { + beforeAll(() => { + config.featureToggles.prometheusSpecialCharsInLabelValues = true; + }); + + it('should not modify expression with no filters', async () => { + ds.query({ + interval: '15s', + range: getMockTimeRange(), + targets: [target], + } as DataQueryRequest); + const [result] = fetchMockCalledWith(fetchMock); + expect(result).toMatchObject({ expr: DEFAULT_QUERY_EXPRESSION }); + }); + + it('should add escaping if needed to regex filter expressions', () => { + const filters = [ + { + key: 'k1', + operator: '=~', + value: 'v.*', + }, + { + key: 'k2', + operator: '=~', + value: `v'.*`, + }, + { + key: 'k3', + operator: '=~', + value: `v".*`, + }, + { + key: 'k4', + operator: '=~', + value: `\\v.*`, + }, + ]; + ds.query({ + interval: '15s', + range: getMockTimeRange(), + filters, + targets: [target], + } as DataQueryRequest); + const [result] = fetchMockCalledWith(fetchMock); + expect(result).toMatchObject({ + expr: `metric{job="foo", k1=~"v.*", k2=~"v'.*", k3=~"v\\".*", k4=~"\\\\v.*"} - metric{k1=~"v.*", k2=~"v'.*", k3=~"v\\".*", k4=~"\\\\v.*"}`, + }); }); }); }); @@ -470,56 +528,126 @@ describe('PrometheusDatasource', () => { }); describe('Prometheus regular escaping', () => { - it('should not escape non-string', () => { - expect(prometheusRegularEscape(12)).toEqual(12); + describe('with prometheusSpecialCharsInLabelValues disabled', () => { + beforeAll(() => { + config.featureToggles.prometheusSpecialCharsInLabelValues = false; + }); + + it('should not escape non-string', () => { + expect(prometheusRegularEscape(12)).toEqual(12); + }); + + it('should not escape strings without special characters', () => { + expect(prometheusRegularEscape('cryptodepression')).toEqual('cryptodepression'); + }); + + it('should escape single quotes', () => { + expect(prometheusRegularEscape("looking'glass")).toEqual("looking\\\\'glass"); + }); + + it('should escape backslashes', () => { + expect(prometheusRegularEscape('looking\\glass')).toEqual('looking\\\\glass'); + }); }); - it('should not escape simple string', () => { - expect(prometheusRegularEscape('cryptodepression')).toEqual('cryptodepression'); - }); + describe('with prometheusSpecialCharsInLabelValues enabled', () => { + beforeAll(() => { + config.featureToggles.prometheusSpecialCharsInLabelValues = true; + }); - it("should escape '", () => { - expect(prometheusRegularEscape("looking'glass")).toEqual("looking\\\\'glass"); - }); + it('should not escape non-string', () => { + expect(prometheusRegularEscape(12)).toEqual(12); + }); - it('should escape \\', () => { - expect(prometheusRegularEscape('looking\\glass')).toEqual('looking\\\\glass'); - }); + it('should not escape strings without special characters', () => { + expect(prometheusRegularEscape('cryptodepression')).toEqual('cryptodepression'); + }); - it('should escape multiple characters', () => { - expect(prometheusRegularEscape("'looking'glass'")).toEqual("\\\\'looking\\\\'glass\\\\'"); - }); + it('should not escape complete label matcher', () => { + expect(prometheusRegularEscape('job="grafana"')).toEqual('job="grafana"'); + expect(prometheusRegularEscape('job!="grafana"')).toEqual('job!="grafana"'); + expect(prometheusRegularEscape('job=~"grafana"')).toEqual('job=~"grafana"'); + expect(prometheusRegularEscape('job!~"grafana"')).toEqual('job!~"grafana"'); + }); - it('should escape multiple different characters', () => { - expect(prometheusRegularEscape("'loo\\king'glass'")).toEqual("\\\\'loo\\\\king\\\\'glass\\\\'"); + it('should not escape single quotes', () => { + expect(prometheusRegularEscape("looking'glass")).toEqual("looking'glass"); + }); + + it('should escape double quotes', () => { + expect(prometheusRegularEscape('looking"glass')).toEqual('looking\\"glass'); + }); + + it('should escape backslashes', () => { + expect(prometheusRegularEscape('looking\\glass')).toEqual('looking\\\\glass'); + }); + + it('should handle complete label matchers with escaped content', () => { + expect(prometheusRegularEscape('job="my\\"service"')).toEqual('job="my\\"service"'); + expect(prometheusRegularEscape('job="\\\\server"')).toEqual('job="\\\\server"'); + }); }); }); describe('Prometheus regexes escaping', () => { - it('should not escape simple string', () => { - expect(prometheusSpecialRegexEscape('cryptodepression')).toEqual('cryptodepression'); + describe('with prometheusSpecialCharsInLabelValues disabled', () => { + beforeAll(() => { + config.featureToggles.prometheusSpecialCharsInLabelValues = false; + }); + + it('should not escape strings without special characters', () => { + expect(prometheusSpecialRegexEscape('cryptodepression')).toEqual('cryptodepression'); + }); + + it('should escape special characters', () => { + expect(prometheusSpecialRegexEscape('looking{glass')).toEqual('looking\\\\{glass'); + expect(prometheusSpecialRegexEscape('looking$glass')).toEqual('looking\\\\$glass'); + expect(prometheusSpecialRegexEscape('looking\\glass')).toEqual('looking\\\\\\\\glass'); + expect(prometheusSpecialRegexEscape('looking|glass')).toEqual('looking\\\\|glass'); + }); + + it('should handle multiple special characters', () => { + expect(prometheusSpecialRegexEscape('+looking$glass?')).toEqual('\\\\+looking\\\\$glass\\\\?'); + }); }); - it('should escape $^*+?.()|\\', () => { - expect(prometheusSpecialRegexEscape("looking'glass")).toEqual("looking\\\\'glass"); - expect(prometheusSpecialRegexEscape('looking{glass')).toEqual('looking\\\\{glass'); - expect(prometheusSpecialRegexEscape('looking}glass')).toEqual('looking\\\\}glass'); - expect(prometheusSpecialRegexEscape('looking[glass')).toEqual('looking\\\\[glass'); - expect(prometheusSpecialRegexEscape('looking]glass')).toEqual('looking\\\\]glass'); - expect(prometheusSpecialRegexEscape('looking$glass')).toEqual('looking\\\\$glass'); - expect(prometheusSpecialRegexEscape('looking^glass')).toEqual('looking\\\\^glass'); - expect(prometheusSpecialRegexEscape('looking*glass')).toEqual('looking\\\\*glass'); - expect(prometheusSpecialRegexEscape('looking+glass')).toEqual('looking\\\\+glass'); - expect(prometheusSpecialRegexEscape('looking?glass')).toEqual('looking\\\\?glass'); - expect(prometheusSpecialRegexEscape('looking.glass')).toEqual('looking\\\\.glass'); - expect(prometheusSpecialRegexEscape('looking(glass')).toEqual('looking\\\\(glass'); - expect(prometheusSpecialRegexEscape('looking)glass')).toEqual('looking\\\\)glass'); - expect(prometheusSpecialRegexEscape('looking\\glass')).toEqual('looking\\\\\\\\glass'); - expect(prometheusSpecialRegexEscape('looking|glass')).toEqual('looking\\\\|glass'); - }); + describe('with prometheusSpecialCharsInLabelValues enabled', () => { + beforeAll(() => { + config.featureToggles.prometheusSpecialCharsInLabelValues = true; + }); - it('should escape multiple special characters', () => { - expect(prometheusSpecialRegexEscape('+looking$glass?')).toEqual('\\\\+looking\\\\$glass\\\\?'); + it('should not escape strings without special characters', () => { + expect(prometheusSpecialRegexEscape('cryptodepression')).toEqual('cryptodepression'); + }); + + it('should escape special characters', () => { + expect(prometheusSpecialRegexEscape('looking{glass')).toEqual('looking\\\\{glass'); + expect(prometheusSpecialRegexEscape('looking}glass')).toEqual('looking\\\\}glass'); + expect(prometheusSpecialRegexEscape('looking[glass')).toEqual('looking\\\\[glass'); + expect(prometheusSpecialRegexEscape('looking]glass')).toEqual('looking\\\\]glass'); + expect(prometheusSpecialRegexEscape('looking$glass')).toEqual('looking\\\\$glass'); + expect(prometheusSpecialRegexEscape('looking^glass')).toEqual('looking\\\\^glass'); + expect(prometheusSpecialRegexEscape('looking*glass')).toEqual('looking\\\\*glass'); + expect(prometheusSpecialRegexEscape('looking+glass')).toEqual('looking\\\\+glass'); + expect(prometheusSpecialRegexEscape('looking?glass')).toEqual('looking\\\\?glass'); + expect(prometheusSpecialRegexEscape('looking.glass')).toEqual('looking\\\\.glass'); + expect(prometheusSpecialRegexEscape('looking(glass')).toEqual('looking\\\\(glass'); + expect(prometheusSpecialRegexEscape('looking)glass')).toEqual('looking\\\\)glass'); + expect(prometheusSpecialRegexEscape('looking\\glass')).toEqual('looking\\\\\\\\glass'); + expect(prometheusSpecialRegexEscape('looking|glass')).toEqual('looking\\\\|glass'); + }); + + it('should escape double quotes with special regex escaping', () => { + expect(prometheusSpecialRegexEscape('looking"glass')).toEqual('looking\\\\\\"glass'); + }); + + it('should handle multiple special characters', () => { + expect(prometheusSpecialRegexEscape('+looking$glass?')).toEqual('\\\\+looking\\\\$glass\\\\?'); + }); + + it('should handle mixed quotes and special characters', () => { + expect(prometheusSpecialRegexEscape('+looking"$glass?')).toEqual('\\\\+looking\\\\\\"\\\\$glass\\\\?'); + }); }); }); @@ -548,9 +676,27 @@ describe('PrometheusDatasource', () => { }; }); - describe('and value is a string', () => { - it('should only escape single quotes', () => { - expect(ds.interpolateQueryExpr("abc'$^*{}[]+?.()|", customVariable)).toEqual("abc\\\\'$^*{}[]+?.()|"); + describe('with prometheusSpecialCharsInLabelValues disabled', () => { + beforeAll(() => { + config.featureToggles.prometheusSpecialCharsInLabelValues = false; + }); + + describe('and value is a string', () => { + it('should escape single quotes', () => { + expect(ds.interpolateQueryExpr("abc'$^*{}[]+?.()|", customVariable)).toEqual("abc\\\\'$^*{}[]+?.()|"); + }); + }); + }); + + describe('with prometheusSpecialCharsInLabelValues enabled', () => { + beforeAll(() => { + config.featureToggles.prometheusSpecialCharsInLabelValues = true; + }); + + describe('and value is a string', () => { + it('should only escape double quotes and backslashes', () => { + expect(ds.interpolateQueryExpr('abc\'"$^*{}[]+?.()|\\', customVariable)).toEqual('abc\'\\"$^*{}[]+?.()|\\\\'); + }); }); }); diff --git a/packages/grafana-prometheus/src/datasource.ts b/packages/grafana-prometheus/src/datasource.ts index c7653f5702f..8ece500ac12 100644 --- a/packages/grafana-prometheus/src/datasource.ts +++ b/packages/grafana-prometheus/src/datasource.ts @@ -1046,13 +1046,46 @@ export function extractRuleMappingFromGroups(groups: RawRecordingRules[]): RuleQ ); } -// NOTE: these two functions are very similar to the escapeLabelValueIn* functions +// NOTE: these two functions are similar to the escapeLabelValueIn* functions // in language_utils.ts, but they are not exactly the same algorithm, and we found // no way to reuse one in the another or vice versa. export function prometheusRegularEscape(value: T) { - return typeof value === 'string' ? value.replace(/\\/g, '\\\\').replace(/'/g, "\\\\'") : value; + if (typeof value !== 'string') { + return value; + } + + if (config.featureToggles.prometheusSpecialCharsInLabelValues) { + // if the string looks like a complete label matcher (e.g. 'job="grafana"' or 'job=~"grafana"'), + // don't escape the encapsulating quotes + if (/^\w+(=|!=|=~|!~)".*"$/.test(value)) { + return value; + } + + return value + .replace(/\\/g, '\\\\') // escape backslashes + .replace(/"/g, '\\"'); // escape double quotes + } + + // classic behavior + return value + .replace(/\\/g, '\\\\') // escape backslashes + .replace(/'/g, "\\\\'"); // escape single quotes } export function prometheusSpecialRegexEscape(value: T) { - return typeof value === 'string' ? value.replace(/\\/g, '\\\\\\\\').replace(/[$^*{}\[\]\'+?.()|]/g, '\\\\$&') : value; + if (typeof value !== 'string') { + return value; + } + + if (config.featureToggles.prometheusSpecialCharsInLabelValues) { + return value + .replace(/\\/g, '\\\\\\\\') // escape backslashes + .replace(/"/g, '\\\\\\"') // escape double quotes + .replace(/[$^*{}\[\]\'+?.()|]/g, '\\\\$&'); // escape regex metacharacters + } + + // classic behavior + return value + .replace(/\\/g, '\\\\\\\\') // escape backslashes + .replace(/[$^*{}\[\]+?.()|]/g, '\\\\$&'); // escape regex metacharacters } diff --git a/packages/grafana-prometheus/src/querybuilder/shared/LokiAndPromQueryModellerBase.ts b/packages/grafana-prometheus/src/querybuilder/shared/LokiAndPromQueryModellerBase.ts index 872973b4887..1d06ed7edac 100644 --- a/packages/grafana-prometheus/src/querybuilder/shared/LokiAndPromQueryModellerBase.ts +++ b/packages/grafana-prometheus/src/querybuilder/shared/LokiAndPromQueryModellerBase.ts @@ -1,6 +1,8 @@ // Core Grafana history https://github.com/grafana/grafana/blob/v11.0.0-preview/public/app/plugins/datasource/prometheus/querybuilder/shared/LokiAndPromQueryModellerBase.ts import { Registry } from '@grafana/data'; +import { config } from '@grafana/runtime'; +import { prometheusRegularEscape } from '../../datasource'; import { PromVisualQueryOperationCategory } from '../types'; import { QueryBuilderLabelFilter, QueryBuilderOperation, QueryBuilderOperationDef, VisualQueryModeller } from './types'; @@ -89,7 +91,13 @@ export abstract class LokiAndPromQueryModellerBase implements VisualQueryModelle expr += ', '; } - expr += `${filter.label}${filter.op}"${filter.value}"`; + let labelValue = filter.value; + const usingRegexOperator = filter.op === '=~' || filter.op === '!~'; + + if (config.featureToggles.prometheusSpecialCharsInLabelValues && !usingRegexOperator) { + labelValue = prometheusRegularEscape(labelValue); + } + expr += `${filter.label}${filter.op}"${labelValue}"`; } return expr + `}`; diff --git a/pkg/services/featuremgmt/registry.go b/pkg/services/featuremgmt/registry.go index e79a97df92f..ddde412a65e 100644 --- a/pkg/services/featuremgmt/registry.go +++ b/pkg/services/featuremgmt/registry.go @@ -1608,6 +1608,13 @@ var ( FrontendOnly: true, HideFromDocs: true, }, + { + Name: "prometheusSpecialCharsInLabelValues", + Description: "Adds support for quotes and special characters in label values for Prometheus queries", + FrontendOnly: true, + Stage: FeatureStageExperimental, + Owner: grafanaObservabilityMetricsSquad, + }, { Name: "enableExtensionsAdminPage", Description: "Enables the extension admin page regardless of development mode", diff --git a/pkg/services/featuremgmt/toggles_gen.csv b/pkg/services/featuremgmt/toggles_gen.csv index 84416308b00..92c6b16ce16 100644 --- a/pkg/services/featuremgmt/toggles_gen.csv +++ b/pkg/services/featuremgmt/toggles_gen.csv @@ -213,6 +213,7 @@ dashboardSchemaV2,experimental,@grafana/dashboards-squad,false,false,true playlistsWatcher,experimental,@grafana/grafana-app-platform-squad,false,true,false passwordlessMagicLinkAuthentication,experimental,@grafana/identity-access-team,false,false,false exploreMetricsRelatedLogs,experimental,@grafana/observability-metrics,false,false,true +prometheusSpecialCharsInLabelValues,experimental,@grafana/observability-metrics,false,false,true enableExtensionsAdminPage,experimental,@grafana/plugins-platform-backend,false,true,false zipkinBackendMigration,GA,@grafana/oss-big-tent,false,false,false enableSCIM,experimental,@grafana/identity-access-team,false,false,false diff --git a/pkg/services/featuremgmt/toggles_gen.go b/pkg/services/featuremgmt/toggles_gen.go index 42843a4cf75..72fb3c04379 100644 --- a/pkg/services/featuremgmt/toggles_gen.go +++ b/pkg/services/featuremgmt/toggles_gen.go @@ -863,6 +863,10 @@ const ( // Display Related Logs in Explore Metrics FlagExploreMetricsRelatedLogs = "exploreMetricsRelatedLogs" + // FlagPrometheusSpecialCharsInLabelValues + // Adds support for quotes and special characters in label values for Prometheus queries + FlagPrometheusSpecialCharsInLabelValues = "prometheusSpecialCharsInLabelValues" + // FlagEnableExtensionsAdminPage // Enables the extension admin page regardless of development mode FlagEnableExtensionsAdminPage = "enableExtensionsAdminPage" diff --git a/pkg/services/featuremgmt/toggles_gen.json b/pkg/services/featuremgmt/toggles_gen.json index 7a4396aad85..6827fab8a5f 100644 --- a/pkg/services/featuremgmt/toggles_gen.json +++ b/pkg/services/featuremgmt/toggles_gen.json @@ -2931,6 +2931,19 @@ "codeowner": "@grafana/observability-metrics" } }, + { + "metadata": { + "name": "prometheusSpecialCharsInLabelValues", + "resourceVersion": "1734047568531", + "creationTimestamp": "2024-12-12T23:52:48Z" + }, + "spec": { + "description": "Adds support for quotes and special characters in label values for Prometheus queries", + "stage": "experimental", + "codeowner": "@grafana/observability-metrics", + "frontend": true + } + }, { "metadata": { "name": "prometheusUsesCombobox", diff --git a/public/app/features/trails/MetricSelect/api.ts b/public/app/features/trails/MetricSelect/api.ts index c6d77c6958f..2d499efc3c0 100644 --- a/public/app/features/trails/MetricSelect/api.ts +++ b/public/app/features/trails/MetricSelect/api.ts @@ -1,5 +1,6 @@ import { AdHocVariableFilter, RawTimeRange, Scope } from '@grafana/data'; import { getPrometheusTime } from '@grafana/prometheus/src/language_utils'; +import { PromQueryModeller } from '@grafana/prometheus/src/querybuilder/PromQueryModeller'; import { config, getBackendSrv } from '@grafana/runtime'; import { limitOtelMatchTerms } from '../otel/util'; @@ -7,6 +8,8 @@ import { callSuggestionsApi, SuggestionsResponse } from '../utils'; const LIMIT_REACHED = 'results truncated due to limit'; +const queryModeller = new PromQueryModeller(); + export async function getMetricNames( dataSourceUid: string, timeRange: RawTimeRange, @@ -31,7 +34,11 @@ export async function getMetricNamesWithoutScopes( instances: string[], limit?: number ) { - const matchTerms = adhocFilters.map((filter) => `${filter.key}${filter.operator}"${filter.value}"`); + const matchTerms = config.featureToggles.prometheusSpecialCharsInLabelValues + ? adhocFilters.map((filter) => + removeBrackets(queryModeller.renderLabels([{ label: filter.key, op: filter.operator, value: filter.value }])) + ) + : adhocFilters.map((filter) => `${filter.key}${filter.operator}"${filter.value}"`); let missingOtelTargets = false; if (jobs.length > 0 && instances.length > 0) { @@ -99,3 +106,8 @@ export async function getMetricNamesWithScopes( missingOtelTargets: false, }; } + +function removeBrackets(input: string): string { + const match = input.match(/^\{(.*)\}$/); // extract the content inside the brackets + return match?.[1] ?? ''; +} diff --git a/public/app/features/trails/otel/util.ts b/public/app/features/trails/otel/util.ts index f4cdae14d36..c8633e2df95 100644 --- a/public/app/features/trails/otel/util.ts +++ b/public/app/features/trails/otel/util.ts @@ -1,4 +1,5 @@ import { MetricFindValue } from '@grafana/data'; +import { config } from '@grafana/runtime'; import { AdHocFiltersVariable, ConstantVariable, CustomVariable, sceneGraph, SceneObject } from '@grafana/scenes'; import { DataTrail } from '../DataTrail'; @@ -121,6 +122,9 @@ export function getOtelResourcesObject(scene: SceneObject, firstQueryVal?: strin // start with the deployment environment let allFilters = `deployment_environment${op}"${val}"`; + if (config.featureToggles.prometheusSpecialCharsInLabelValues) { + allFilters = `deployment_environment${op}'${val}'`; + } let allLabels = 'deployment_environment'; // add the other OTEL resource filters @@ -129,7 +133,11 @@ export function getOtelResourcesObject(scene: SceneObject, firstQueryVal?: strin const op = otelFilters[i].operator; const labelValue = otelFilters[i].value; - allFilters += `,${labelName}${op}"${labelValue}"`; + if (config.featureToggles.prometheusSpecialCharsInLabelValues) { + allFilters += `,${labelName}${op}'${labelValue}'`; + } else { + allFilters += `,${labelName}${op}"${labelValue}"`; + } const addLabelToGroupLeft = labelName !== 'job' && labelName !== 'instance'; @@ -167,8 +175,8 @@ export function limitOtelMatchTerms( let initialCharAmount = matchTerms.join(',').length; // start to add values to the regex and start quote - let jobsRegex = 'job=~"'; - let instancesRegex = 'instance=~"'; + let jobsRegex = `job=~'`; + let instancesRegex = `instance=~'`; // iterate through the jobs and instances, // count the chars as they are added, @@ -206,8 +214,8 @@ export function limitOtelMatchTerms( } } // complete the quote after values have been added - jobsRegex += '"'; - instancesRegex += '"'; + jobsRegex += `'`; + instancesRegex += `'`; return { missingOtelTargets, diff --git a/public/app/features/trails/otel/utils.test.ts b/public/app/features/trails/otel/utils.test.ts index ff54ef6f7ee..11d8657aa6b 100644 --- a/public/app/features/trails/otel/utils.test.ts +++ b/public/app/features/trails/otel/utils.test.ts @@ -164,8 +164,8 @@ describe('limitOtelMatchTerms', () => { const result = limitOtelMatchTerms(promMatchTerms, jobs, instances); expect(result.missingOtelTargets).toEqual(true); - expect(result.jobsRegex).toEqual('job=~"a"'); - expect(result.instancesRegex).toEqual('instance=~"d"'); + expect(result.jobsRegex).toEqual(`job=~'a'`); + expect(result.instancesRegex).toEqual(`instance=~'d'`); }); it('should include | char in the count', () => { @@ -189,8 +189,8 @@ describe('limitOtelMatchTerms', () => { const result = limitOtelMatchTerms(promMatchTerms, jobs, instances); expect(result.missingOtelTargets).toEqual(true); - expect(result.jobsRegex).toEqual('job=~"a|b"'); - expect(result.instancesRegex).toEqual('instance=~"d|e"'); + expect(result.jobsRegex).toEqual(`job=~'a|b'`); + expect(result.instancesRegex).toEqual(`instance=~'d|e'`); }); it('should add all OTel job and instance matches if the character count is less that 2000', () => { @@ -203,8 +203,8 @@ describe('limitOtelMatchTerms', () => { const result = limitOtelMatchTerms(promMatchTerms, jobs, instances); expect(result.missingOtelTargets).toEqual(false); - expect(result.jobsRegex).toEqual('job=~"job1|job2|job3|job4|job5"'); - expect(result.instancesRegex).toEqual('instance=~"instance1|instance2|instance3|instance4|instance5"'); + expect(result.jobsRegex).toEqual(`job=~'job1|job2|job3|job4|job5'`); + expect(result.instancesRegex).toEqual(`instance=~'instance1|instance2|instance3|instance4|instance5'`); }); });