Skip to content

Commit

Permalink
[APM] Fix missing ML data, and NaN issue (#34333)
Browse files Browse the repository at this point in the history
  • Loading branch information
@sorenlouv
sorenlouv authored Apr 2, 2019
1 parent 2bba68f commit 949a267
Show file tree
Hide file tree
Showing 5 changed files with 266 additions and 99 deletions.
226 changes: 154 additions & 72 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 };
}

describe('setupRequest', () => {
it('should call callWithRequest with default args', async () => {
const setup = setupRequest(mockReq);
await setup.client('myType', { body: { foo: 'bar' } });
expect(callWithRequestSpy).toHaveBeenCalledWith(mockReq, 'myType', {
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 @@ -47,51 +47,82 @@ describe('setupRequest', () => {
});

describe('omitLegacyData', () => {
it('should add `observer.version_major` filter if `omitLegacyData=true` ', async () => {
const setup = setupRequest(mockReq);
await setup.client('myType', {
omitLegacyData: true,
body: { query: { bool: { filter: [{ term: 'someTerm' }] } } }
});
expect(callWithRequestSpy.mock.calls[0][2].body).toEqual({
query: {
bool: {
filter: [
{ term: 'someTerm' },
{ range: { 'observer.version_major': { gte: 7 } } }
]
describe('if index is apm-*', () => {
it('should add `observer.version_major` filter if `omitLegacyData=true` ', async () => {
const { mockRequest, callWithRequestSpy } = getMockRequest();
const setup = setupRequest(mockRequest);
await setup.client('myType', {
index: 'apm-*',
omitLegacyData: true,
body: { query: { bool: { filter: [{ term: 'someTerm' }] } } }
});
expect(callWithRequestSpy.mock.calls[0][2].body).toEqual({
query: {
bool: {
filter: [
{ term: 'someTerm' },
{ range: { 'observer.version_major': { gte: 7 } } }
]
}
}
}
});
});
});

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

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 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 } } }]
it('should have `omitLegacyData=true` as default and merge 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('should have `omitLegacyData=true` as default and merge boolean filters', async () => {
const setup = setupRequest(mockReq);
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' }] } }
}
Expand All @@ -100,24 +131,75 @@ describe('setupRequest', () => {
expect(params.body).toEqual({
query: {
bool: {
filter: [
{ term: 'someTerm' },
{ range: { 'observer.version_major': { gte: 7 } } }
]
filter: [{ term: 'someTerm' }]
}
}
});
});
});

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('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);
});
});

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);
});
});

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);
});
});
});
});
});
42 changes: 34 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 { cloneDeep, has, set } 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 @@ -43,12 +43,36 @@ interface APMRequestQuery {
esFilterQuery: string;
}

function addFilterForLegacyData({
omitLegacyData = true,
...params
}: APMSearchParams): SearchParams {
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[],
{ omitLegacyData = true, ...params }: APMSearchParams
): SearchParams {
// search across all data (including data)
if (!omitLegacyData) {
if (!omitLegacyData || !isApmIndex(apmIndices, params.index)) {
return params;
}

Expand All @@ -69,12 +93,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 @@ -99,6 +125,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 949a267

Please sign in to comment.