From 6a447a24fb7144ada86a651f5456251297d485e0 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Thu, 25 Oct 2018 14:16:01 +0200 Subject: [PATCH 1/7] stackdriver: don't set project name in query response since default project is now loaded in its own query --- public/app/plugins/datasource/stackdriver/datasource.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/public/app/plugins/datasource/stackdriver/datasource.ts b/public/app/plugins/datasource/stackdriver/datasource.ts index 4a81eb8a619..034333cbb86 100644 --- a/public/app/plugins/datasource/stackdriver/datasource.ts +++ b/public/app/plugins/datasource/stackdriver/datasource.ts @@ -114,7 +114,6 @@ export default class StackdriverDatasource { if (!queryRes.series) { return; } - this.projectName = queryRes.meta.defaultProject; const unit = this.resolvePanelUnitFromTargets(options.targets); queryRes.series.forEach(series => { let timeSerie: any = { From 2a4a19388f30220fd81586bd70b7e3aa8a1495e5 Mon Sep 17 00:00:00 2001 From: Michael Huynh Date: Sat, 27 Oct 2018 16:54:12 +0800 Subject: [PATCH 2/7] Fix label suggestions for multi-line aggregation queries No label suggestions were being returned for multi-line aggregation contexts because the parsed selector string does not see the full context before a `by` or `without` clause. This solution stitches together all text nodes that comprise the query editor to ensure the selector has sufficient context to generate suggestions. Also, an additional workaround has been included to ensure range vector syntax does not disrupt label suggestions in aggregation contexts. Related: #12890 --- .../prometheus/language_provider.ts | 27 ++++++++++++++----- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/public/app/plugins/datasource/prometheus/language_provider.ts b/public/app/plugins/datasource/prometheus/language_provider.ts index 3e406a71264..3dd15eb713e 100644 --- a/public/app/plugins/datasource/prometheus/language_provider.ts +++ b/public/app/plugins/datasource/prometheus/language_provider.ts @@ -162,16 +162,29 @@ export default class PromQlLanguageProvider extends LanguageProvider { let refresher: Promise = null; const suggestions: CompletionItemGroup[] = []; - // sum(foo{bar="1"}) by (|) - const line = value.anchorBlock.getText(); - const cursorOffset: number = value.anchorOffset; - // sum(foo{bar="1"}) by ( - const leftSide = line.slice(0, cursorOffset); + // Stitch all query lines together to support multi-line queries + let queryOffset; + const queryText = value.document.getBlocks().reduce((text, block) => { + const blockText = block.getText(); + if (value.anchorBlock.key === block.key) { + // Newline characters are not accounted for but this is irrelevant + // for the purpose of extracting the selector string + queryOffset = value.anchorOffset + text.length; + } + text += blockText; + return text; + }, ''); + + const leftSide = queryText.slice(0, queryOffset); const openParensAggregationIndex = leftSide.lastIndexOf('('); const openParensSelectorIndex = leftSide.slice(0, openParensAggregationIndex).lastIndexOf('('); const closeParensSelectorIndex = leftSide.slice(openParensSelectorIndex).indexOf(')') + openParensSelectorIndex; - // foo{bar="1"} - const selectorString = leftSide.slice(openParensSelectorIndex + 1, closeParensSelectorIndex); + + let selectorString = leftSide.slice(openParensSelectorIndex + 1, closeParensSelectorIndex); + + // Range vector syntax not accounted for by subsequent parse so discard it if present + selectorString = selectorString.replace(/\[[^\]]+\]$/, ''); + const selector = parseSelector(selectorString, selectorString.length - 2).selector; const labelKeys = this.labelKeys[selector]; From 61843b58db92fbed130f323a5ec0ab4abff62d92 Mon Sep 17 00:00:00 2001 From: Michael Huynh Date: Sat, 27 Oct 2018 17:02:03 +0800 Subject: [PATCH 3/7] Add tests to cover aggregation context cases This should cover use cases involving multi-line queries and range vector syntax inside aggregation contexts. Related: #12890 --- .../specs/language_provider.test.ts | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/public/app/plugins/datasource/prometheus/specs/language_provider.test.ts b/public/app/plugins/datasource/prometheus/specs/language_provider.test.ts index 3a46e2efaf3..20e148efd57 100644 --- a/public/app/plugins/datasource/prometheus/specs/language_provider.test.ts +++ b/public/app/plugins/datasource/prometheus/specs/language_provider.test.ts @@ -198,5 +198,76 @@ describe('Language completion provider', () => { expect(result.context).toBe('context-aggregation'); expect(result.suggestions).toEqual([{ items: [{ label: 'bar' }], label: 'Labels' }]); }); + + it('returns label suggestions inside a multi-line aggregation context', () => { + const instance = new LanguageProvider(datasource, { + labelKeys: { '{__name__="metric"}': ['label1', 'label2', 'label3'] }, + }); + const value = Plain.deserialize('sum(\nmetric\n)\nby ()'); + const aggregationTextBlock = value.document.getBlocksAsArray()[3]; + const range = value.selection.moveToStartOf(aggregationTextBlock).merge({ anchorOffset: 4 }); + const valueWithSelection = value.change().select(range).value; + const result = instance.provideCompletionItems({ + text: '', + prefix: '', + wrapperClasses: ['context-aggregation'], + value: valueWithSelection, + }); + expect(result.context).toBe('context-aggregation'); + expect(result.suggestions).toEqual([ + { + items: [{ label: 'label1' }, { label: 'label2' }, { label: 'label3' }], + label: 'Labels', + }, + ]); + }); + + it('returns label suggestions inside an aggregation context with a range vector', () => { + const instance = new LanguageProvider(datasource, { + labelKeys: { '{__name__="metric"}': ['label1', 'label2', 'label3'] }, + }); + const value = Plain.deserialize('sum(rate(metric[1h])) by ()'); + const range = value.selection.merge({ + anchorOffset: 26, + }); + const valueWithSelection = value.change().select(range).value; + const result = instance.provideCompletionItems({ + text: '', + prefix: '', + wrapperClasses: ['context-aggregation'], + value: valueWithSelection, + }); + expect(result.context).toBe('context-aggregation'); + expect(result.suggestions).toEqual([ + { + items: [{ label: 'label1' }, { label: 'label2' }, { label: 'label3' }], + label: 'Labels', + }, + ]); + }); + + it('returns label suggestions inside an aggregation context with a range vector and label', () => { + const instance = new LanguageProvider(datasource, { + labelKeys: { '{__name__="metric",label1="value"}': ['label1', 'label2', 'label3'] }, + }); + const value = Plain.deserialize('sum(rate(metric{label1="value"}[1h])) by ()'); + const range = value.selection.merge({ + anchorOffset: 42, + }); + const valueWithSelection = value.change().select(range).value; + const result = instance.provideCompletionItems({ + text: '', + prefix: '', + wrapperClasses: ['context-aggregation'], + value: valueWithSelection, + }); + expect(result.context).toBe('context-aggregation'); + expect(result.suggestions).toEqual([ + { + items: [{ label: 'label1' }, { label: 'label2' }, { label: 'label3' }], + label: 'Labels', + }, + ]); + }); }); }); From c255b5da1113b1eef115b3acab326e130eb77b88 Mon Sep 17 00:00:00 2001 From: Michael Huynh Date: Sun, 28 Oct 2018 21:03:39 +0800 Subject: [PATCH 4/7] Add sum aggregation query suggestion Implements rudimentary support for placeholder values inside a string with the `PlaceholdersBuffer` class. The latter helps the newly added sum aggregation query suggestion to automatically focus on the label so users can easily choose from the available typeahead options. Related: #13615 --- public/app/features/explore/Explore.tsx | 36 ++++-- .../features/explore/PlaceholdersBuffer.ts | 112 ++++++++++++++++++ public/app/features/explore/QueryField.tsx | 34 ++++-- .../datasource/prometheus/datasource.ts | 3 + .../datasource/prometheus/query_hints.ts | 24 ++++ public/app/types/explore.ts | 1 + 6 files changed, 190 insertions(+), 20 deletions(-) create mode 100644 public/app/features/explore/PlaceholdersBuffer.ts diff --git a/public/app/features/explore/Explore.tsx b/public/app/features/explore/Explore.tsx index 680cd1e6685..f29d38d283a 100644 --- a/public/app/features/explore/Explore.tsx +++ b/public/app/features/explore/Explore.tsx @@ -373,9 +373,10 @@ export class Explore extends React.PureComponent { this.onModifyQueries({ type: 'ADD_FILTER', key: columnKey, value: rowValue }); }; - onModifyQueries = (action: object, index?: number) => { + onModifyQueries = (action, index?: number) => { const { datasource } = this.state; if (datasource && datasource.modifyQuery) { + const preventSubmit = action.preventSubmit; this.setState( state => { const { queries, queryTransactions } = state; @@ -391,16 +392,26 @@ export class Explore extends React.PureComponent { nextQueryTransactions = []; } else { // Modify query only at index - nextQueries = [ - ...queries.slice(0, index), - { - key: generateQueryKey(index), - query: datasource.modifyQuery(this.queryExpressions[index], action), - }, - ...queries.slice(index + 1), - ]; - // Discard transactions related to row query - nextQueryTransactions = queryTransactions.filter(qt => qt.rowIndex !== index); + nextQueries = queries.map((q, i) => { + // Synchronise all queries with local query cache to ensure consistency + q.query = this.queryExpressions[i]; + return i === index + ? { + key: generateQueryKey(index), + query: datasource.modifyQuery(q.query, action), + } + : q; + }); + nextQueryTransactions = queryTransactions + // Consume the hint corresponding to the action + .map(qt => { + if (qt.hints != null && qt.rowIndex === index) { + qt.hints = qt.hints.filter(hint => hint.fix.action !== action); + } + return qt; + }) + // Preserve previous row query transaction to keep results visible if next query is incomplete + .filter(qt => preventSubmit || qt.rowIndex !== index); } this.queryExpressions = nextQueries.map(q => q.query); return { @@ -408,7 +419,8 @@ export class Explore extends React.PureComponent { queryTransactions: nextQueryTransactions, }; }, - () => this.onSubmit() + // Accepting certain fixes do not result in a well-formed query which should not be submitted + !preventSubmit ? () => this.onSubmit() : null ); } }; diff --git a/public/app/features/explore/PlaceholdersBuffer.ts b/public/app/features/explore/PlaceholdersBuffer.ts new file mode 100644 index 00000000000..9a0db18ef04 --- /dev/null +++ b/public/app/features/explore/PlaceholdersBuffer.ts @@ -0,0 +1,112 @@ +/** + * Provides a stateful means of managing placeholders in text. + * + * Placeholders are numbers prefixed with the `$` character (e.g. `$1`). + * Each number value represents the order in which a placeholder should + * receive focus if multiple placeholders exist. + * + * Example scenario given `sum($3 offset $1) by($2)`: + * 1. `sum( offset |) by()` + * 2. `sum( offset 1h) by(|)` + * 3. `sum(| offset 1h) by (label)` + */ +export default class PlaceholdersBuffer { + private nextMoveOffset: number; + private orders: number[]; + private parts: string[]; + + constructor(text: string) { + const result = this.parse(text); + const nextPlaceholderIndex = result.orders.length ? result.orders[0] : 0; + this.nextMoveOffset = this.getOffsetBetween(result.parts, 0, nextPlaceholderIndex); + this.orders = result.orders; + this.parts = result.parts; + } + + clearPlaceholders() { + this.nextMoveOffset = 0; + this.orders = []; + } + + getNextMoveOffset(): number { + return this.nextMoveOffset; + } + + hasPlaceholders(): boolean { + return this.orders.length > 0; + } + + setNextPlaceholderValue(value: string) { + if (this.orders.length === 0) { + return; + } + const currentPlaceholderIndex = this.orders[0]; + this.parts[currentPlaceholderIndex] = value; + this.orders = this.orders.slice(1); + if (this.orders.length === 0) { + this.nextMoveOffset = 0; + return; + } + const nextPlaceholderIndex = this.orders[0]; + // Case should never happen but handle it gracefully in case + if (currentPlaceholderIndex === nextPlaceholderIndex) { + this.nextMoveOffset = 0; + return; + } + const backwardMove = currentPlaceholderIndex > nextPlaceholderIndex; + const indices = backwardMove + ? { start: nextPlaceholderIndex + 1, end: currentPlaceholderIndex + 1 } + : { start: currentPlaceholderIndex + 1, end: nextPlaceholderIndex }; + this.nextMoveOffset = (backwardMove ? -1 : 1) * this.getOffsetBetween(this.parts, indices.start, indices.end); + } + + toString(): string { + return this.parts.join(''); + } + + private getOffsetBetween(parts: string[], startIndex: number, endIndex: number) { + return parts.slice(startIndex, endIndex).reduce((offset, part) => offset + part.length, 0); + } + + private parse(text: string): ParseResult { + const placeholderRegExp = /\$(\d+)/g; + const parts = []; + const orders = []; + let textOffset = 0; + while (true) { + const match = placeholderRegExp.exec(text); + if (!match) { + break; + } + const part = text.slice(textOffset, match.index); + parts.push(part); + // Accounts for placeholders at text boundaries + if (part !== '') { + parts.push(''); + } + const order = parseInt(match[1], 10); + orders.push({ index: parts.length - 1, order }); + textOffset += part.length + match.length; + } + // Ensures string serialisation still works if no placeholders were parsed + // and also accounts for the remainder of text with placeholders + parts.push(text.slice(textOffset)); + return { + // Placeholder values do not necessarily appear sequentially so sort the + // indices to traverse in priority order + orders: orders.sort((o1, o2) => o1.order - o2.order).map(o => o.index), + parts, + }; + } +} + +type ParseResult = { + /** + * Indices to placeholder items in `parts` in traversal order. + */ + orders: number[]; + /** + * Parts comprising the original text with placeholders occupying distinct items. + */ + parts: string[]; +}; diff --git a/public/app/features/explore/QueryField.tsx b/public/app/features/explore/QueryField.tsx index 86daaa43eac..9350682f1a0 100644 --- a/public/app/features/explore/QueryField.tsx +++ b/public/app/features/explore/QueryField.tsx @@ -12,6 +12,7 @@ import NewlinePlugin from './slate-plugins/newline'; import Typeahead from './Typeahead'; import { makeFragment, makeValue } from './Value'; +import PlaceholdersBuffer from './PlaceholdersBuffer'; export const TYPEAHEAD_DEBOUNCE = 100; @@ -61,12 +62,15 @@ export interface TypeaheadInput { class QueryField extends React.PureComponent { menuEl: HTMLElement | null; + placeholdersBuffer: PlaceholdersBuffer; plugins: any[]; resetTimer: any; constructor(props, context) { super(props, context); + this.placeholdersBuffer = new PlaceholdersBuffer(props.initialValue || ''); + // Base plugins this.plugins = [ClearPlugin(), NewlinePlugin(), ...props.additionalPlugins]; @@ -76,7 +80,7 @@ class QueryField extends React.PureComponent operation.type === 'insert_text'); + if (insertTextOperation) { + const suggestionText = insertTextOperation.text; + this.placeholdersBuffer.setNextPlaceholderValue(suggestionText); + if (this.placeholdersBuffer.hasPlaceholders()) { + nextChange.move(this.placeholdersBuffer.getNextMoveOffset()).focus(); + } + } + return true; } break; @@ -336,6 +352,8 @@ class QueryField extends React.PureComponent= SUM_HINT_THRESHOLD_COUNT) { + const simpleMetric = query.trim().match(/^\w+$/); + if (simpleMetric) { + hints.push({ + type: 'ADD_SUM', + label: 'Many time series results returned.', + fix: { + label: 'Consider aggregating with sum().', + action: { + type: 'ADD_SUM', + query: query, + preventSubmit: true, + }, + }, + }); + } + } + return hints.length > 0 ? hints : null; } diff --git a/public/app/types/explore.ts b/public/app/types/explore.ts index 807769212f3..1db37392b0b 100644 --- a/public/app/types/explore.ts +++ b/public/app/types/explore.ts @@ -119,6 +119,7 @@ export interface QueryFix { export interface QueryFixAction { type: string; query?: string; + preventSubmit?: boolean; } export interface QueryHint { From d1d5e9f7d3a81a957f512d7f52704a23e2ef8509 Mon Sep 17 00:00:00 2001 From: Michael Huynh Date: Sun, 28 Oct 2018 21:07:40 +0800 Subject: [PATCH 5/7] Add tests to cover PlaceholdersBuffer and sum hint Related: #13615 --- .../explore/PlaceholdersBuffer.test.ts | 72 +++++++++++++++++++ .../prometheus/specs/query_hints.test.ts | 21 +++++- 2 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 public/app/features/explore/PlaceholdersBuffer.test.ts diff --git a/public/app/features/explore/PlaceholdersBuffer.test.ts b/public/app/features/explore/PlaceholdersBuffer.test.ts new file mode 100644 index 00000000000..2ce31e79b05 --- /dev/null +++ b/public/app/features/explore/PlaceholdersBuffer.test.ts @@ -0,0 +1,72 @@ +import PlaceholdersBuffer from './PlaceholdersBuffer'; + +describe('PlaceholdersBuffer', () => { + it('does nothing if no placeholders are defined', () => { + const text = 'metric'; + const buffer = new PlaceholdersBuffer(text); + + expect(buffer.hasPlaceholders()).toBe(false); + expect(buffer.toString()).toBe(text); + expect(buffer.getNextMoveOffset()).toBe(0); + }); + + it('respects the traversal order of placeholders', () => { + const text = 'sum($2 offset $1) by ($3)'; + const buffer = new PlaceholdersBuffer(text); + + expect(buffer.hasPlaceholders()).toBe(true); + expect(buffer.toString()).toBe('sum( offset ) by ()'); + expect(buffer.getNextMoveOffset()).toBe(12); + + buffer.setNextPlaceholderValue('1h'); + + expect(buffer.hasPlaceholders()).toBe(true); + expect(buffer.toString()).toBe('sum( offset 1h) by ()'); + expect(buffer.getNextMoveOffset()).toBe(-10); + + buffer.setNextPlaceholderValue('metric'); + + expect(buffer.hasPlaceholders()).toBe(true); + expect(buffer.toString()).toBe('sum(metric offset 1h) by ()'); + expect(buffer.getNextMoveOffset()).toBe(16); + + buffer.setNextPlaceholderValue('label'); + + expect(buffer.hasPlaceholders()).toBe(false); + expect(buffer.toString()).toBe('sum(metric offset 1h) by (label)'); + expect(buffer.getNextMoveOffset()).toBe(0); + }); + + it('respects the traversal order of adjacent placeholders', () => { + const text = '$1$3$2$4'; + const buffer = new PlaceholdersBuffer(text); + + expect(buffer.hasPlaceholders()).toBe(true); + expect(buffer.toString()).toBe(''); + expect(buffer.getNextMoveOffset()).toBe(0); + + buffer.setNextPlaceholderValue('1'); + + expect(buffer.hasPlaceholders()).toBe(true); + expect(buffer.toString()).toBe('1'); + expect(buffer.getNextMoveOffset()).toBe(0); + + buffer.setNextPlaceholderValue('2'); + + expect(buffer.hasPlaceholders()).toBe(true); + expect(buffer.toString()).toBe('12'); + expect(buffer.getNextMoveOffset()).toBe(-1); + + buffer.setNextPlaceholderValue('3'); + + expect(buffer.hasPlaceholders()).toBe(true); + expect(buffer.toString()).toBe('132'); + expect(buffer.getNextMoveOffset()).toBe(1); + + buffer.setNextPlaceholderValue('4'); + + expect(buffer.hasPlaceholders()).toBe(false); + expect(buffer.toString()).toBe('1324'); + expect(buffer.getNextMoveOffset()).toBe(0); + }); +}); diff --git a/public/app/plugins/datasource/prometheus/specs/query_hints.test.ts b/public/app/plugins/datasource/prometheus/specs/query_hints.test.ts index 7eba54536fe..f5435bd5d39 100644 --- a/public/app/plugins/datasource/prometheus/specs/query_hints.test.ts +++ b/public/app/plugins/datasource/prometheus/specs/query_hints.test.ts @@ -1,4 +1,4 @@ -import { getQueryHints } from '../query_hints'; +import { getQueryHints, SUM_HINT_THRESHOLD_COUNT } from '../query_hints'; describe('getQueryHints()', () => { it('returns no hints for no series', () => { @@ -79,4 +79,23 @@ describe('getQueryHints()', () => { }, }); }); + + it('returns a sum hint when many time series results are returned for a simple metric', () => { + const seriesCount = SUM_HINT_THRESHOLD_COUNT; + const series = Array.from({ length: seriesCount }, _ => ({ + datapoints: [[0, 0], [0, 0]], + })); + const hints = getQueryHints('metric', series); + expect(hints.length).toBe(seriesCount); + expect(hints[0]).toMatchObject({ + label: 'Many time series results returned.', + index: 0, + fix: { + action: { + type: 'ADD_SUM', + query: 'metric', + }, + }, + }); + }); }); From e44dde3f145ae789c1519affa1a5dc7481d75107 Mon Sep 17 00:00:00 2001 From: Steve Kreitzer Date: Sun, 28 Oct 2018 10:25:42 -0400 Subject: [PATCH 6/7] Fixing issue 13855 --- docs/sources/auth/gitlab.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/sources/auth/gitlab.md b/docs/sources/auth/gitlab.md index e3a450f9fc7..56fc3b131a5 100644 --- a/docs/sources/auth/gitlab.md +++ b/docs/sources/auth/gitlab.md @@ -100,12 +100,12 @@ display name, especially if the display name contains spaces or special characters. Make sure you always use the group or subgroup name as it appears in the URL of the group or subgroup. -Here's a complete example with `alloed_sign_up` enabled, and access limited to +Here's a complete example with `allow_sign_up` enabled, and access limited to the `example` and `foo/bar` groups: ```ini [auth.gitlab] -enabled = false +enabled = true allow_sign_up = true client_id = GITLAB_APPLICATION_ID client_secret = GITLAB_SECRET From 9245dad53ea5faf984bd5cd2039b1038c2cf15e2 Mon Sep 17 00:00:00 2001 From: David Kaltschmidt Date: Sun, 28 Oct 2018 17:48:17 +0100 Subject: [PATCH 7/7] Fix query hint tests after refactor --- .../plugins/datasource/prometheus/specs/query_hints.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/public/app/plugins/datasource/prometheus/specs/query_hints.test.ts b/public/app/plugins/datasource/prometheus/specs/query_hints.test.ts index f5435bd5d39..3b782d3dd09 100644 --- a/public/app/plugins/datasource/prometheus/specs/query_hints.test.ts +++ b/public/app/plugins/datasource/prometheus/specs/query_hints.test.ts @@ -86,14 +86,16 @@ describe('getQueryHints()', () => { datapoints: [[0, 0], [0, 0]], })); const hints = getQueryHints('metric', series); - expect(hints.length).toBe(seriesCount); + expect(hints.length).toBe(1); expect(hints[0]).toMatchObject({ + type: 'ADD_SUM', label: 'Many time series results returned.', - index: 0, fix: { + label: 'Consider aggregating with sum().', action: { type: 'ADD_SUM', query: 'metric', + preventSubmit: true, }, }, });