Skip to content

Commit

Permalink
[APM] Fix missing ML data, and NaN issue (elastic#34333) (elastic#34356)
Browse files Browse the repository at this point in the history 
# Conflicts:
#	x-pack/plugins/apm/server/lib/helpers/setup_request.test.ts
#	x-pack/plugins/apm/server/lib/helpers/setup_request.ts
  • Loading branch information
@sorenlouv
sorenlouv authored Apr 2, 2019
1 parent 180f1ec commit d359122
Show file tree
Hide file tree
Showing 5 changed files with 259 additions and 86 deletions.
198 changes: 139 additions & 59 deletions x-pack/plugins/apm/server/lib/helpers/setup_request.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,35 +4,35 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { setupRequest } from './setup_request';
import { Legacy } from 'kibana';
import { isApmIndex, setupRequest } from './setup_request';

describe('setupRequest', () => {
let callWithRequestSpy: jest.Mock;
let mockReq: any;

beforeEach(() => {
callWithRequestSpy = jest.fn();
mockReq = {
params: {},
query: {},
server: {
config: () => 'myConfig',
plugins: {
elasticsearch: {
getCluster: () => ({ callWithRequest: callWithRequestSpy })
}
function getMockRequest() {
const callWithRequestSpy = jest.fn();
const mockRequest = ({
params: {},
query: {},
server: {
config: () => ({ get: () => 'apm-*' }),
plugins: {
elasticsearch: {
getCluster: () => ({ callWithRequest: callWithRequestSpy })
}
},
getUiSettingsService: jest.fn(() => ({
get: jest.fn(() => Promise.resolve(false))
}))
};
});
}
},
getUiSettingsService: () => ({ get: async () => false })
} as any) as Legacy.Request;

return { callWithRequestSpy, mockRequest };
}

it('should call callWithRequest with correct args', async () => {
const setup = setupRequest(mockReq);
await setup.client('myType', { body: { foo: 'bar' } });
expect(callWithRequestSpy).toHaveBeenCalledWith(mockReq, 'myType', {
describe('setupRequest', () => {
it('should call callWithRequest with default args', async () => {
const { mockRequest, callWithRequestSpy } = getMockRequest();
const setup = setupRequest(mockRequest);
await setup.client('myType', { index: 'apm-*', body: { foo: 'bar' } });
expect(callWithRequestSpy).toHaveBeenCalledWith(mockRequest, 'myType', {
index: 'apm-*',
body: {
foo: 'bar',
query: {
Expand All @@ -46,45 +46,125 @@ describe('setupRequest', () => {
});
});

it('should set ignore_throttled to false if includeFrozen is true', async () => {
// mock includeFrozen to return true
mockReq.getUiSettingsService.mockImplementation(() => ({
get: jest.fn(() => Promise.resolve(true))
}));
const setup = setupRequest(mockReq);
await setup.client('myType', {});
const params = callWithRequestSpy.mock.calls[0][2];
expect(params.ignore_throttled).toBe(false);
describe('observer.version_major filter', () => {
describe('if index is apm-*', () => {
it('should add `observer.version_major` filter if none exists', async () => {
const { mockRequest, callWithRequestSpy } = getMockRequest();
const setup = setupRequest(mockRequest);
await setup.client('myType', { index: 'apm-*' });
const params = callWithRequestSpy.mock.calls[0][2];
expect(params.body).toEqual({
query: {
bool: {
filter: [{ range: { 'observer.version_major': { gte: 7 } } }]
}
}
});
});

it('should merge `observer.version_major` filter with existing boolean filters', async () => {
const { mockRequest, callWithRequestSpy } = getMockRequest();
const setup = setupRequest(mockRequest);
await setup.client('myType', {
index: 'apm-*',
body: { query: { bool: { filter: [{ term: 'someTerm' }] } } }
});
const params = callWithRequestSpy.mock.calls[0][2];
expect(params.body).toEqual({
query: {
bool: {
filter: [
{ term: 'someTerm' },
{ range: { 'observer.version_major': { gte: 7 } } }
]
}
}
});
});
});

it('if index is not an APM index, it should not add `observer.version_major` filter', async () => {
const { mockRequest, callWithRequestSpy } = getMockRequest();
const setup = setupRequest(mockRequest);
await setup.client('myType', {
index: '.ml-*',
body: {
query: { bool: { filter: [{ term: 'someTerm' }] } }
}
});
const params = callWithRequestSpy.mock.calls[0][2];
expect(params.body).toEqual({
query: {
bool: {
filter: [{ term: 'someTerm' }]
}
}
});
});
});

it('should set filter if none exists', async () => {
const setup = setupRequest(mockReq);
await setup.client('myType', {});
const params = callWithRequestSpy.mock.calls[0][2];
expect(params.body).toEqual({
query: {
bool: { filter: [{ range: { 'observer.version_major': { gte: 7 } } }] }
}
describe('ignore_throttled', () => {
it('should set `ignore_throttled=true` if `includeFrozen=false`', async () => {
const { mockRequest, callWithRequestSpy } = getMockRequest();

// mock includeFrozen to return false
mockRequest.getUiSettingsService = () => ({ get: async () => false });
const setup = setupRequest(mockRequest);
await setup.client('myType', {});
const params = callWithRequestSpy.mock.calls[0][2];
expect(params.ignore_throttled).toBe(true);
});

it('should set `ignore_throttled=false` if `includeFrozen=true`', async () => {
const { mockRequest, callWithRequestSpy } = getMockRequest();

// mock includeFrozen to return true
mockRequest.getUiSettingsService = () => ({ get: async () => true });
const setup = setupRequest(mockRequest);
await setup.client('myType', {});
const params = callWithRequestSpy.mock.calls[0][2];
expect(params.ignore_throttled).toBe(false);
});
});

it('should merge filters if one exists', async () => {
const setup = setupRequest(mockReq);
await setup.client('myType', {
body: {
query: { bool: { filter: [{ term: 'someTerm' }] } }
}
describe('isApmIndex', () => {
const apmIndices = [
'apm-*-metric-*',
'apm-*-onboarding-*',
'apm-*-span-*',
'apm-*-transaction-*',
'apm-*-error-*'
];
describe('when indexParam is a string', () => {
it('should return true if it matches any of the items in apmIndices', () => {
const indexParam = 'apm-*-transaction-*';
expect(isApmIndex(apmIndices, indexParam)).toBe(true);
});

it('should return false if it does not match any of the items in `apmIndices`', () => {
const indexParam = '.ml-anomalies-*';
expect(isApmIndex(apmIndices, indexParam)).toBe(false);
});
});
const params = callWithRequestSpy.mock.calls[0][2];
expect(params.body).toEqual({
query: {
bool: {
filter: [
{ term: 'someTerm' },
{ range: { 'observer.version_major': { gte: 7 } } }
]
}
}

describe('when indexParam is an array', () => {
it('should return true if all values in `indexParam` matches values in `apmIndices`', () => {
const indexParam = ['apm-*-transaction-*', 'apm-*-span-*'];
expect(isApmIndex(apmIndices, indexParam)).toBe(true);
});

it("should return false if some of the values don't match with `apmIndices`", () => {
const indexParam = ['apm-*-transaction-*', '.ml-anomalies-*'];
expect(isApmIndex(apmIndices, indexParam)).toBe(false);
});
});

describe('when indexParam is neither a string or an array', () => {
it('should return false', () => {
[true, false, undefined].forEach(indexParam => {
expect(isApmIndex(apmIndices, indexParam)).toBe(false);
});
});
});
});
});
50 changes: 42 additions & 8 deletions x-pack/plugins/apm/server/lib/helpers/setup_request.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
SearchParams
} from 'elasticsearch';
import { Legacy } from 'kibana';
import { merge } from 'lodash';
import { cloneDeep, has, isString, set } from 'lodash';
import moment from 'moment';
import { OBSERVER_VERSION_MAJOR } from 'x-pack/plugins/apm/common/elasticsearch_fieldnames';

Expand Down Expand Up @@ -39,11 +39,43 @@ interface APMRequestQuery {
esFilterQuery: string;
}

function addFilterForLegacyData(params: SearchParams) {
// ensure a filter exists
const nextParams = merge({}, params, {
body: { query: { bool: { filter: [] } } }
});
function getApmIndices(config: Legacy.KibanaConfig) {
return [
config.get<string>('apm_oss.errorIndices'),
config.get<string>('apm_oss.metricsIndices'),
config.get<string>('apm_oss.onboardingIndices'),
config.get<string>('apm_oss.sourcemapIndices'),
config.get<string>('apm_oss.spanIndices'),
config.get<string>('apm_oss.transactionIndices')
];
}

export function isApmIndex(
apmIndices: string[],
indexParam: SearchParams['index']
) {
if (isString(indexParam)) {
return apmIndices.includes(indexParam);
} else if (Array.isArray(indexParam)) {
// return false if at least one of the indices is not an APM index
return indexParam.every(index => apmIndices.includes(index));
}
return false;
}

function addFilterForLegacyData(
apmIndices: string[],
params: SearchParams
): SearchParams {
// search across all data (including 6.x data)
if (!isApmIndex(apmIndices, params.index)) {
return params;
}

const nextParams = cloneDeep(params);
if (!has(nextParams, 'body.query.bool.filter')) {
set(nextParams, 'body.query.bool.filter', []);
}

// add to filter
nextParams.body.query.bool.filter.push({
Expand All @@ -57,12 +89,14 @@ export function setupRequest(req: Legacy.Request): Setup {
const query = (req.query as unknown) as APMRequestQuery;
const cluster = req.server.plugins.elasticsearch.getCluster('data');
const uiSettings = req.getUiSettingsService();
const config = req.server.config();
const apmIndices = getApmIndices(config);

const client: ESClient = async (type, params) => {
const includeFrozen = await uiSettings.get('search:includeFrozen');

const nextParams = {
...addFilterForLegacyData(params), // filter out pre-7.0 data
...addFilterForLegacyData(apmIndices, params), // filter out pre-7.0 data
ignore_throttled: !includeFrozen, // whether to query frozen indices or not
rest_total_hits_as_int: true // ensure that ES returns accurate hits.total with pre-6.6 format
};
Expand All @@ -87,6 +121,6 @@ export function setupRequest(req: Legacy.Request): Setup {
end: moment.utc(query.end).valueOf(),
esFilterQuery: decodeEsQuery(query.esFilterQuery),
client,
config: req.server.config()
config
};
}
Loading

0 comments on commit d359122

Please sign in to comment.