Skip to content

Commit

Permalink
Graphite: fix autocomplete when tags are not available (grafana#31680)
Browse files Browse the repository at this point in the history 
* Return empty list of tags when tags are not available

In some configurations graphite-web fail to return the list of tags. It shouldn't however prevent displaying list of metrics (which is concatenated with tags).

* Populate jsonData with default version of Graphite

The version of Graphite is preselected in the dropdown but was not saved in jsonData initially.

* Fix a typo

* Show a popup with an error message

* Always use the latest Graphite value as the default one when creating a datasource

* Move autocomplete error handling to GraphiteQueryCtrl

* Test error handing in Graphite autocomplete

* Test default Graphite version fallback

* Rename graphite_versions.ts to versions.ts

* Remove redundant import

* Code formatting, minor renaming

* Remove redundant error info
  • Loading branch information
@ifrost @ryantxu
ifrost authored and ryantxu committed Mar 30, 2021
1 parent 6a8344b commit f1dacaa
Show file tree
Hide file tree
Showing 6 changed files with 122 additions and 24 deletions.
19 changes: 12 additions & 7 deletions public/app/plugins/datasource/graphite/configuration/ConfigEditor.tsx
View file
Expand Up @@ -3,16 +3,14 @@ import { DataSourceHttpSettings, InlineFormLabel, LegacyForms } from '@grafana/u
const { Select, Switch } = LegacyForms;
import {
DataSourcePluginOptionsEditorProps,
updateDatasourcePluginJsonDataOption,
onUpdateDatasourceJsonDataOptionSelect,
onUpdateDatasourceJsonDataOptionChecked,
} from '@grafana/data';
import { GraphiteOptions, GraphiteType } from '../types';
import { DEFAULT_GRAPHITE_VERSION, GRAPHITE_VERSIONS } from '../versions';

const graphiteVersions = [
{ label: '0.9.x', value: '0.9' },
{ label: '1.0.x', value: '1.0' },
{ label: '1.1.x', value: '1.1' },
];
const graphiteVersions = GRAPHITE_VERSIONS.map((version) => ({ label: `${version}.x`, value: version }));

const graphiteTypes = Object.entries(GraphiteType).map(([label, value]) => ({
label,
Expand Down Expand Up @@ -40,11 +38,14 @@ export class ConfigEditor extends PureComponent<Props> {
);
};

componentDidMount() {
updateDatasourcePluginJsonDataOption(this.props, 'graphiteVersion', this.currentGraphiteVersion);
}

render() {
const { options, onOptionsChange } = this.props;

const currentVersion =
graphiteVersions.find((item) => item.value === options.jsonData.graphiteVersion) ?? graphiteVersions[2];
const currentVersion = graphiteVersions.find((item) => item.value === this.currentGraphiteVersion);

return (
<>
Expand Down Expand Up @@ -96,4 +97,8 @@ export class ConfigEditor extends PureComponent<Props> {
</>
);
}

private get currentGraphiteVersion() {
return this.props.options.jsonData.graphiteVersion || DEFAULT_GRAPHITE_VERSION;
}
}
7 changes: 5 additions & 2 deletions public/app/plugins/datasource/graphite/datasource.ts
View file
Expand Up @@ -18,6 +18,7 @@ import { getTemplateSrv, TemplateSrv } from 'app/features/templating/template_sr
import { GraphiteOptions, GraphiteQuery, GraphiteType, MetricTankRequestMeta } from './types';
import { getRollupNotice, getRuntimeConsolidationNotice } from 'app/plugins/datasource/graphite/meta';
import { getSearchFilterScopedVar } from '../../../features/variables/utils';
import { DEFAULT_GRAPHITE_VERSION } from './versions';

export class GraphiteDatasource extends DataSourceApi<GraphiteQuery, GraphiteOptions> {
basicAuth: string;
Expand All @@ -38,7 +39,9 @@ export class GraphiteDatasource extends DataSourceApi<GraphiteQuery, GraphiteOpt
this.basicAuth = instanceSettings.basicAuth;
this.url = instanceSettings.url;
this.name = instanceSettings.name;
this.graphiteVersion = instanceSettings.jsonData.graphiteVersion || '1.1';
// graphiteVersion is set when a datasource is created but it hadn't been set in the past so we're
// still falling back to the default behavior here for backwards compatibility (see also #17429)
this.graphiteVersion = instanceSettings.jsonData.graphiteVersion || DEFAULT_GRAPHITE_VERSION;
this.isMetricTank = instanceSettings.jsonData.graphiteType === GraphiteType.Metrictank;
this.supportsTags = supportsTags(this.graphiteVersion);
this.cacheTimeout = instanceSettings.cacheTimeout;
Expand Down Expand Up @@ -449,7 +452,7 @@ export class GraphiteDatasource extends DataSourceApi<GraphiteQuery, GraphiteOpt
});
}

getTagsAutoComplete(expressions: any[], tagPrefix: any, optionalOptions: any) {
getTagsAutoComplete(expressions: any[], tagPrefix: any, optionalOptions?: any) {
const options = optionalOptions || {};

const httpOptions: any = {
Expand Down
52 changes: 37 additions & 15 deletions public/app/plugins/datasource/graphite/query_ctrl.ts
View file
Expand Up @@ -4,11 +4,12 @@ import './func_editor';
import _ from 'lodash';
import GraphiteQuery from './graphite_query';
import { QueryCtrl } from 'app/plugins/sdk';
import appEvents from 'app/core/app_events';
import { promiseToDigest } from 'app/core/utils/promiseToDigest';
import { auto } from 'angular';
import { TemplateSrv } from '@grafana/runtime';
import { AppEvents } from '@grafana/data';
import { dispatch } from 'app/store/store';
import { notifyApp } from 'app/core/actions';
import { createErrorNotification } from 'app/core/copy/appNotification';

const GRAPHITE_TAG_OPERATORS = ['=', '!=', '=~', '!=~'];
const TAG_PREFIX = 'tag: ';
Expand All @@ -23,6 +24,9 @@ export class GraphiteQueryCtrl extends QueryCtrl {
supportsTags: boolean;
paused: boolean;

// to avoid error flooding, it's shown only once per session
private _tagsAutoCompleteErrorShown = false;

/** @ngInject */
constructor(
$scope: any,
Expand Down Expand Up @@ -106,7 +110,7 @@ export class GraphiteQueryCtrl extends QueryCtrl {
}
})
.catch((err: any) => {
appEvents.emit(AppEvents.alertError, ['Error', err]);
dispatch(notifyApp(createErrorNotification('Error', err)));
});
}

Expand Down Expand Up @@ -331,24 +335,34 @@ export class GraphiteQueryCtrl extends QueryCtrl {

getTags(index: number, tagPrefix: any) {
const tagExpressions = this.queryModel.renderTagExpressions(index);
return this.datasource.getTagsAutoComplete(tagExpressions, tagPrefix).then((values: any) => {
const altTags = _.map(values, 'text');
altTags.splice(0, 0, this.removeTagValue);
return mapToDropdownOptions(altTags);
});
return this.datasource
.getTagsAutoComplete(tagExpressions, tagPrefix)
.then((values: any) => {
const altTags = _.map(values, 'text');
altTags.splice(0, 0, this.removeTagValue);
return mapToDropdownOptions(altTags);
})
.catch((err: any) => {
this.handleTagsAutoCompleteError(err);
});
}

getTagsAsSegments(tagPrefix: string) {
const tagExpressions = this.queryModel.renderTagExpressions();
return this.datasource.getTagsAutoComplete(tagExpressions, tagPrefix).then((values: any) => {
return _.map(values, (val) => {
return this.uiSegmentSrv.newSegment({
value: val.text,
type: 'tag',
expandable: false,
return this.datasource
.getTagsAutoComplete(tagExpressions, tagPrefix)
.then((values: any) => {
return _.map(values, (val) => {
return this.uiSegmentSrv.newSegment({
value: val.text,
type: 'tag',
expandable: false,
});
});
})
.catch((err: any) => {
this.handleTagsAutoCompleteError(err);
});
});
}

getTagOperators() {
Expand Down Expand Up @@ -415,6 +429,14 @@ export class GraphiteQueryCtrl extends QueryCtrl {
getCollapsedText() {
return this.target.target;
}

private handleTagsAutoCompleteError(error: Error): void {
console.log(error);
if (!this._tagsAutoCompleteErrorShown) {
this._tagsAutoCompleteErrorShown = true;
dispatch(notifyApp(createErrorNotification(`Fetching tags failed: ${error.message}.`)));
}
}
}

function mapToDropdownOptions(results: any[]) {
Expand Down
5 changes: 5 additions & 0 deletions public/app/plugins/datasource/graphite/specs/datasource.test.ts
View file
Expand Up @@ -4,6 +4,7 @@ import _ from 'lodash';
import { TemplateSrv } from 'app/features/templating/template_srv';
import { dateTime, getFrameDisplayName } from '@grafana/data';
import { backendSrv } from 'app/core/services/backend_srv'; // will use the version in __mocks__
import { DEFAULT_GRAPHITE_VERSION } from '../versions';

jest.mock('@grafana/runtime', () => ({
...((jest.requireActual('@grafana/runtime') as unknown) as object),
Expand Down Expand Up @@ -35,6 +36,10 @@ describe('graphiteDatasource', () => {
ctx = { templateSrv, ds };
});

it('uses default Graphite version when no graphiteVersion is provided', () => {
expect(ctx.ds.graphiteVersion).toBe(DEFAULT_GRAPHITE_VERSION);
});

describe('convertResponseToDataFrames', () => {
it('should transform regular result', () => {
const result = ctx.ds.convertResponseToDataFrames({
Expand Down
58 changes: 58 additions & 0 deletions public/app/plugins/datasource/graphite/specs/query_ctrl.test.ts
View file
@@ -1,14 +1,21 @@
import { dispatch } from 'app/store/store';
import { uiSegmentSrv } from 'app/core/services/segment_srv';
import gfunc from '../gfunc';
import { GraphiteQueryCtrl } from '../query_ctrl';
import { TemplateSrvStub } from 'test/specs/helpers';
import { silenceConsoleOutput } from 'test/core/utils/silenceConsoleOutput';

jest.mock('app/core/utils/promiseToDigest', () => ({
promiseToDigest: (scope: any) => {
return (p: Promise<any>) => p;
},
}));

jest.mock('app/store/store', () => ({
dispatch: jest.fn(),
}));
const mockDispatch = dispatch as jest.Mock;

describe('GraphiteQueryCtrl', () => {
const ctx = {
datasource: {
Expand All @@ -17,6 +24,7 @@ describe('GraphiteQueryCtrl', () => {
getFuncDef: gfunc.getFuncDef,
waitForFuncDefsLoaded: jest.fn(() => Promise.resolve(null)),
createFuncInstance: gfunc.createFuncInstance,
getTagsAutoComplete: jest.fn().mockReturnValue(Promise.resolve([])),
},
target: { target: 'aliasByNode(scaleToSeconds(test.prod.*,1),2)' },
panelCtrl: {
Expand All @@ -29,6 +37,7 @@ describe('GraphiteQueryCtrl', () => {
};

beforeEach(() => {
jest.clearAllMocks();
GraphiteQueryCtrl.prototype.target = ctx.target;
GraphiteQueryCtrl.prototype.datasource = ctx.datasource;
GraphiteQueryCtrl.prototype.panelCtrl = ctx.panelCtrl;
Expand Down Expand Up @@ -184,6 +193,55 @@ describe('GraphiteQueryCtrl', () => {
});
});

describe('when autocomplete for tags is not available', () => {
silenceConsoleOutput();
beforeEach(() => {
ctx.ctrl.datasource.getTagsAutoComplete.mockReturnValue(
new Promise(() => {
throw new Error();
})
);
});

it('getTags should handle autocomplete errors', async () => {
await expect(async () => {
await ctx.ctrl.getTags(0, 'any');
expect(mockDispatch).toBeCalledWith(
expect.objectContaining({
type: 'appNotifications/notifyApp',
})
);
}).not.toThrow();
});

it('getTags should display the error message only once', async () => {
await ctx.ctrl.getTags(0, 'any');
expect(mockDispatch.mock.calls.length).toBe(1);

await ctx.ctrl.getTags(0, 'any');
expect(mockDispatch.mock.calls.length).toBe(1);
});

it('getTagsAsSegments should handle autocomplete errors', async () => {
await expect(async () => {
await ctx.ctrl.getTagsAsSegments('any');
expect(mockDispatch).toBeCalledWith(
expect.objectContaining({
type: 'appNotifications/notifyApp',
})
);
}).not.toThrow();
});

it('getTagsAsSegments should display the error message only once', async () => {
await ctx.ctrl.getTagsAsSegments('any');
expect(mockDispatch.mock.calls.length).toBe(1);

await ctx.ctrl.getTagsAsSegments('any');
expect(mockDispatch.mock.calls.length).toBe(1);
});
});

describe('targetChanged', () => {
beforeEach(() => {
ctx.ctrl.target.target = 'aliasByNode(scaleToSeconds(test.prod.*, 1), 2)';
Expand Down
5 changes: 5 additions & 0 deletions public/app/plugins/datasource/graphite/versions.ts
View file
@@ -0,0 +1,5 @@
import { last } from 'lodash';

export const GRAPHITE_VERSIONS = ['0.9', '1.0', '1.1'];

export const DEFAULT_GRAPHITE_VERSION = last(GRAPHITE_VERSIONS)!;

0 comments on commit f1dacaa

Please sign in to comment.