From 949a267672d69d6aba67b1bfbfe084498bfa9e75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Louv-Jansen?= Date: Tue, 2 Apr 2019 16:39:19 +0200 Subject: [PATCH] [APM] Fix missing ML data, and NaN issue (#34333) --- .../server/lib/helpers/setup_request.test.ts | 226 ++++++++++++------ .../apm/server/lib/helpers/setup_request.ts | 42 +++- .../__snapshots__/transform.test.ts.snap | 56 +++++ .../get_timeseries_data/transform.test.ts | 37 +-- .../charts/get_timeseries_data/transform.ts | 4 +- 5 files changed, 266 insertions(+), 99 deletions(-) diff --git a/x-pack/plugins/apm/server/lib/helpers/setup_request.test.ts b/x-pack/plugins/apm/server/lib/helpers/setup_request.test.ts index e99176b3da564..a2b69bb5629d9 100644 --- a/x-pack/plugins/apm/server/lib/helpers/setup_request.test.ts +++ b/x-pack/plugins/apm/server/lib/helpers/setup_request.test.ts @@ -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: { @@ -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' }] } } } @@ -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); + }); + }); + }); }); }); diff --git a/x-pack/plugins/apm/server/lib/helpers/setup_request.ts b/x-pack/plugins/apm/server/lib/helpers/setup_request.ts index 1571de3e57965..db9c6f921d9cc 100644 --- a/x-pack/plugins/apm/server/lib/helpers/setup_request.ts +++ b/x-pack/plugins/apm/server/lib/helpers/setup_request.ts @@ -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'; @@ -43,12 +43,36 @@ interface APMRequestQuery { esFilterQuery: string; } -function addFilterForLegacyData({ - omitLegacyData = true, - ...params -}: APMSearchParams): SearchParams { +function getApmIndices(config: Legacy.KibanaConfig) { + return [ + config.get('apm_oss.errorIndices'), + config.get('apm_oss.metricsIndices'), + config.get('apm_oss.onboardingIndices'), + config.get('apm_oss.sourcemapIndices'), + config.get('apm_oss.spanIndices'), + config.get('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; } @@ -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 }; @@ -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 }; } diff --git a/x-pack/plugins/apm/server/lib/transactions/charts/get_timeseries_data/__snapshots__/transform.test.ts.snap b/x-pack/plugins/apm/server/lib/transactions/charts/get_timeseries_data/__snapshots__/transform.test.ts.snap index ba0a07e04ad9d..c137b8718bb88 100644 --- a/x-pack/plugins/apm/server/lib/transactions/charts/get_timeseries_data/__snapshots__/transform.test.ts.snap +++ b/x-pack/plugins/apm/server/lib/transactions/charts/get_timeseries_data/__snapshots__/transform.test.ts.snap @@ -5,6 +5,10 @@ Object { "overallAvgDuration": 32861.15660262639, "responseTimes": Object { "avg": Array [ + Object { + "x": 1528113600000, + "y": 26310.63483891513, + }, Object { "x": 1528124400000, "y": 26193.277795595466, @@ -321,8 +325,16 @@ Object { "x": 1528966800000, "y": 52360.30017052116, }, + Object { + "x": 1528977600000, + "y": null, + }, ], "p95": Array [ + Object { + "x": 1528113600000, + "y": 82172.85648714812, + }, Object { "x": 1528124400000, "y": 80738.78571428556, @@ -639,8 +651,16 @@ Object { "x": 1528966800000, "y": 194970.75667682925, }, + Object { + "x": 1528977600000, + "y": null, + }, ], "p99": Array [ + Object { + "x": 1528113600000, + "y": 293866.3866666665, + }, Object { "x": 1528124400000, "y": 293257.27333333343, @@ -957,12 +977,20 @@ Object { "x": 1528966800000, "y": 473485.4199999998, }, + Object { + "x": 1528977600000, + "y": null, + }, ], }, "totalHits": 1297673, "tpmBuckets": Array [ Object { "dataPoints": Array [ + Object { + "x": 1528113600000, + "y": 82230, + }, Object { "x": 1528124400000, "y": 81460, @@ -1279,11 +1307,19 @@ Object { "x": 1528966800000, "y": 81845, }, + Object { + "x": 1528977600000, + "y": 0, + }, ], "key": "HTTP 2xx", }, Object { "dataPoints": Array [ + Object { + "x": 1528113600000, + "y": 0, + }, Object { "x": 1528124400000, "y": 0, @@ -1600,11 +1636,19 @@ Object { "x": 1528966800000, "y": 0, }, + Object { + "x": 1528977600000, + "y": 0, + }, ], "key": "HTTP 3xx", }, Object { "dataPoints": Array [ + Object { + "x": 1528113600000, + "y": 5930, + }, Object { "x": 1528124400000, "y": 6065, @@ -1921,11 +1965,19 @@ Object { "x": 1528966800000, "y": 6070, }, + Object { + "x": 1528977600000, + "y": 0, + }, ], "key": "HTTP 4xx", }, Object { "dataPoints": Array [ + Object { + "x": 1528113600000, + "y": 6045, + }, Object { "x": 1528124400000, "y": 6015, @@ -2242,6 +2294,10 @@ Object { "x": 1528966800000, "y": 5915, }, + Object { + "x": 1528977600000, + "y": 0, + }, ], "key": "HTTP 5xx", }, diff --git a/x-pack/plugins/apm/server/lib/transactions/charts/get_timeseries_data/transform.test.ts b/x-pack/plugins/apm/server/lib/transactions/charts/get_timeseries_data/transform.test.ts index dfaf86c589868..a584cd70e2f8b 100644 --- a/x-pack/plugins/apm/server/lib/transactions/charts/get_timeseries_data/transform.test.ts +++ b/x-pack/plugins/apm/server/lib/transactions/charts/get_timeseries_data/transform.test.ts @@ -4,7 +4,6 @@ * you may not use this file except in compliance with the Elastic License. */ -import { first, last } from 'lodash'; import { timeseriesResponse } from './mock-responses/timeseries_response'; import { ApmTimeSeriesResponse, @@ -21,18 +20,6 @@ describe('timeseriesTransformer', () => { }); }); - it('should not contain first and last bucket', () => { - const mockDates = timeseriesResponse.aggregations.transaction_results.buckets[0].timeseries.buckets.map( - bucket => bucket.key - ); - - expect(first(res.responseTimes.avg).x).not.toBe(first(mockDates)); - expect(last(res.responseTimes.avg).x).not.toBe(last(mockDates)); - - expect(first(res.tpmBuckets[0].dataPoints).x).not.toBe(first(mockDates)); - expect(last(res.tpmBuckets[0].dataPoints).x).not.toBe(last(mockDates)); - }); - it('should have correct order', () => { expect(res.tpmBuckets.map(bucket => bucket.key)).toEqual([ 'HTTP 2xx', @@ -74,7 +61,7 @@ describe('getTpmBuckets', () => { { key_as_string: '', key: 3, - doc_count: 1337 + doc_count: 400 } ] } @@ -102,7 +89,7 @@ describe('getTpmBuckets', () => { { key_as_string: '', key: 3, - doc_count: 1337 + doc_count: 300 } ] } @@ -110,8 +97,24 @@ describe('getTpmBuckets', () => { ]; const bucketSize = 10; expect(getTpmBuckets(buckets, bucketSize)).toEqual([ - { dataPoints: [{ x: 1, y: 1200 }, { x: 2, y: 1800 }], key: 'HTTP 4xx' }, - { dataPoints: [{ x: 1, y: 3000 }, { x: 2, y: 600 }], key: 'HTTP 5xx' } + { + dataPoints: [ + { x: 0, y: 0 }, + { x: 1, y: 1200 }, + { x: 2, y: 1800 }, + { x: 3, y: 2400 } + ], + key: 'HTTP 4xx' + }, + { + dataPoints: [ + { x: 0, y: 0 }, + { x: 1, y: 3000 }, + { x: 2, y: 600 }, + { x: 3, y: 1800 } + ], + key: 'HTTP 5xx' + } ]); }); }); diff --git a/x-pack/plugins/apm/server/lib/transactions/charts/get_timeseries_data/transform.ts b/x-pack/plugins/apm/server/lib/transactions/charts/get_timeseries_data/transform.ts index ef9a643580b11..641f88341ed1a 100644 --- a/x-pack/plugins/apm/server/lib/transactions/charts/get_timeseries_data/transform.ts +++ b/x-pack/plugins/apm/server/lib/transactions/charts/get_timeseries_data/transform.ts @@ -59,7 +59,7 @@ export function getTpmBuckets( ) { const buckets = transactionResultBuckets.map( ({ key: resultKey, timeseries }) => { - const dataPoints = timeseries.buckets.slice(1, -1).map(bucket => { + const dataPoints = timeseries.buckets.map(bucket => { return { x: bucket.key, y: round(bucket.doc_count * (60 / bucketSize), 1) @@ -82,7 +82,7 @@ export function getTpmBuckets( function getResponseTime( responseTimeBuckets: ESResponse['aggregations']['response_times']['buckets'] = [] ) { - return responseTimeBuckets.slice(1, -1).reduce( + return responseTimeBuckets.reduce( (acc, bucket) => { const { '95.0': p95, '99.0': p99 } = bucket.pct.values;