[APM] Catch health status error from ML (#80131)

dgieselaar · web-flow · commit a6b32ab0a232 · 2020-10-12T20:16:51.000+02:00
Closes #80119.
diff --git a/x-pack/plugins/apm/server/lib/services/get_services/get_services_items.ts b/x-pack/plugins/apm/server/lib/services/get_services/get_services_items.ts
@@ -3,6 +3,7 @@
  * or more contributor license agreements. Licensed under the Elastic License;
  * you may not use this file except in compliance with the Elastic License.
  */
+import { Logger } from '@kbn/logging';
 import { joinByKey } from '../../../../common/utils/join_by_key';
 import { PromiseReturnType } from '../../../../typings/common';
 import { Setup, SetupTimeRange } from '../../helpers/setup_request';
@@ -23,9 +24,11 @@ export type ServicesItemsProjection = ReturnType<typeof getServicesProjection>;
 export async function getServicesItems({
   setup,
   searchAggregatedTransactions,
+  logger,
 }: {
   setup: ServicesItemsSetup;
   searchAggregatedTransactions: boolean;
+  logger: Logger;
 }) {
   const params = {
     projection: getServicesProjection({
@@ -49,7 +52,10 @@ export async function getServicesItems({
     getTransactionRates(params),
     getTransactionErrorRates(params),
     getEnvironments(params),
-    getHealthStatuses(params, setup.uiFilters.environment),
+    getHealthStatuses(params, setup.uiFilters.environment).catch((err) => {
+      logger.error(err);
+      return [];
+    }),
   ]);
 
   const allMetrics = [
diff --git a/x-pack/plugins/apm/server/lib/services/get_services/index.ts b/x-pack/plugins/apm/server/lib/services/get_services/index.ts
@@ -5,6 +5,7 @@
  */
 
 import { isEmpty } from 'lodash';
+import { Logger } from '@kbn/logging';
 import { PromiseReturnType } from '../../../../typings/common';
 import { Setup, SetupTimeRange } from '../../helpers/setup_request';
 import { hasHistoricalAgentData } from './has_historical_agent_data';
@@ -16,14 +17,17 @@ export type ServiceListAPIResponse = PromiseReturnType<typeof getServices>;
 export async function getServices({
   setup,
   searchAggregatedTransactions,
+  logger,
 }: {
   setup: Setup & SetupTimeRange;
   searchAggregatedTransactions: boolean;
+  logger: Logger;
 }) {
   const [items, hasLegacyData] = await Promise.all([
     getServicesItems({
       setup,
       searchAggregatedTransactions,
+      logger,
     }),
     getLegacyDataStatus(setup),
   ]);
diff --git a/x-pack/plugins/apm/server/lib/services/queries.test.ts b/x-pack/plugins/apm/server/lib/services/queries.test.ts
@@ -47,7 +47,11 @@ describe('services queries', () => {
 
   it('fetches the service items', async () => {
     mock = await inspectSearchParams((setup) =>
-      getServicesItems({ setup, searchAggregatedTransactions: false })
+      getServicesItems({
+        setup,
+        searchAggregatedTransactions: false,
+        logger: {} as any,
+      })
     );
 
     const allParams = mock.spy.mock.calls.map((call) => call[0]);
diff --git a/x-pack/plugins/apm/server/routes/services.ts b/x-pack/plugins/apm/server/routes/services.ts
@@ -30,7 +30,11 @@ export const servicesRoute = createRoute(() => ({
       setup
     );
 
-    const services = await getServices({ setup, searchAggregatedTransactions });
+    const services = await getServices({
+      setup,
+      searchAggregatedTransactions,
+      logger: context.logger,
+    });
 
     return services;
   },
diff --git a/x-pack/test/apm_api_integration/common/authentication.ts b/x-pack/test/apm_api_integration/common/authentication.ts
@@ -14,6 +14,7 @@ export enum ApmUser {
   apmReadUser = 'apm_read_user',
   apmWriteUser = 'apm_write_user',
   apmAnnotationsWriteUser = 'apm_annotations_write_user',
+  apmReadUserWithoutMlAccess = 'apm_read_user_without_ml_access',
 }
 
 const roles = {
@@ -27,6 +28,15 @@ const roles = {
       },
     ],
   },
+  [ApmUser.apmReadUserWithoutMlAccess]: {
+    kibana: [
+      {
+        base: [],
+        feature: { apm: ['read'] },
+        spaces: ['*'],
+      },
+    ],
+  },
   [ApmUser.apmWriteUser]: {
     kibana: [
       {
@@ -63,6 +73,9 @@ const users = {
   [ApmUser.apmReadUser]: {
     roles: ['apm_user', ApmUser.apmReadUser],
   },
+  [ApmUser.apmReadUserWithoutMlAccess]: {
+    roles: ['apm_user', ApmUser.apmReadUserWithoutMlAccess],
+  },
   [ApmUser.apmWriteUser]: {
     roles: ['apm_user', ApmUser.apmWriteUser],
   },
diff --git a/x-pack/test/apm_api_integration/common/config.ts b/x-pack/test/apm_api_integration/common/config.ts
@@ -63,6 +63,10 @@ export function createTestConfig(settings: Settings) {
           servers.kibana,
           ApmUser.apmAnnotationsWriteUser
         ),
+        supertestAsApmReadUserWithoutMlAccess: supertestAsApmUser(
+          servers.kibana,
+          ApmUser.apmReadUserWithoutMlAccess
+        ),
       },
       junit: {
         reportName: name,
diff --git a/x-pack/test/apm_api_integration/trial/tests/service_maps/__snapshots__/service_maps.snap b/x-pack/test/apm_api_integration/trial/tests/service_maps/__snapshots__/service_maps.snap
diff --git a/x-pack/test/apm_api_integration/trial/tests/service_maps/service_maps.ts b/x-pack/test/apm_api_integration/trial/tests/service_maps/service_maps.ts
@@ -14,6 +14,8 @@ import { FtrProviderContext } from '../../../common/ftr_provider_context';
 
 export default function serviceMapsApiTests({ getService }: FtrProviderContext) {
   const supertest = getService('supertest');
+  const supertestAsApmReadUserWithoutMlAccess = getService('supertestAsApmReadUserWithoutMlAccess');
+
   const esArchiver = getService('esArchiver');
 
   const archiveName = 'apm_8.0.0';
@@ -128,34 +130,35 @@ export default function serviceMapsApiTests({ getService }: FtrProviderContext)
       before(() => esArchiver.load(archiveName));
       after(() => esArchiver.unload(archiveName));
 
-      let response: PromiseReturnType<typeof supertest.get>;
+      describe('with the default apm user', () => {
+        let response: PromiseReturnType<typeof supertest.get>;
 
-      before(async () => {
-        response = await supertest.get(`/api/apm/service-map?start=${start}&end=${end}`);
-      });
+        before(async () => {
+          response = await supertest.get(`/api/apm/service-map?start=${start}&end=${end}`);
+        });
 
-      it('returns service map elements with anomaly stats', () => {
-        expect(response.status).to.be(200);
-        const dataWithAnomalies = response.body.elements.filter(
-          (el: { data: { serviceAnomalyStats?: {} } }) => !isEmpty(el.data.serviceAnomalyStats)
-        );
+        it('returns service map elements with anomaly stats', () => {
+          expect(response.status).to.be(200);
+          const dataWithAnomalies = response.body.elements.filter(
+            (el: { data: { serviceAnomalyStats?: {} } }) => !isEmpty(el.data.serviceAnomalyStats)
+          );
 
-        expect(dataWithAnomalies).to.not.empty();
+          expect(dataWithAnomalies).to.not.empty();
 
-        dataWithAnomalies.forEach(({ data }: any) => {
-          expect(
-            Object.values(data.serviceAnomalyStats).filter((value) => isEmpty(value))
-          ).to.not.empty();
+          dataWithAnomalies.forEach(({ data }: any) => {
+            expect(
+              Object.values(data.serviceAnomalyStats).filter((value) => isEmpty(value))
+            ).to.not.empty();
+          });
         });
-      });
 
-      it('returns the correct anomaly stats', () => {
-        const dataWithAnomalies = response.body.elements.filter(
-          (el: { data: { serviceAnomalyStats?: {} } }) => !isEmpty(el.data.serviceAnomalyStats)
-        );
+        it('returns the correct anomaly stats', () => {
+          const dataWithAnomalies = response.body.elements.filter(
+            (el: { data: { serviceAnomalyStats?: {} } }) => !isEmpty(el.data.serviceAnomalyStats)
+          );
 
-        expectSnapshot(dataWithAnomalies.length).toMatchInline(`5`);
-        expectSnapshot(dataWithAnomalies.slice(0, 3)).toMatchInline(`
+          expectSnapshot(dataWithAnomalies.length).toMatchInline(`5`);
+          expectSnapshot(dataWithAnomalies.slice(0, 3)).toMatchInline(`
           Array [
             Object {
               "data": Object {
@@ -203,7 +206,28 @@ export default function serviceMapsApiTests({ getService }: FtrProviderContext)
           ]
         `);
 
-        expectSnapshot(response.body).toMatch();
+          expectSnapshot(response.body).toMatch();
+        });
+      });
+
+      describe('with a user that does not have access to ML', () => {
+        let response: PromiseReturnType<typeof supertest.get>;
+
+        before(async () => {
+          response = await supertestAsApmReadUserWithoutMlAccess.get(
+            `/api/apm/service-map?start=${start}&end=${end}`
+          );
+        });
+
+        it('returns service map elements without anomaly stats', () => {
+          expect(response.status).to.be(200);
+
+          const dataWithAnomalies = response.body.elements.filter(
+            (el: { data: { serviceAnomalyStats?: {} } }) => !isEmpty(el.data.serviceAnomalyStats)
+          );
+
+          expect(dataWithAnomalies).to.be.empty();
+        });
       });
     });
   });
diff --git a/x-pack/test/apm_api_integration/trial/tests/services/top_services.ts b/x-pack/test/apm_api_integration/trial/tests/services/top_services.ts
@@ -12,6 +12,7 @@ import archives_metadata from '../../../common/archives_metadata';
 
 export default function ApiTest({ getService }: FtrProviderContext) {
   const supertest = getService('supertest');
+  const supertestAsApmReadUserWithoutMlAccess = getService('supertestAsApmReadUserWithoutMlAccess');
   const esArchiver = getService('esArchiver');
 
   const archiveName = 'apm_8.0.0';
@@ -29,35 +30,36 @@ export default function ApiTest({ getService }: FtrProviderContext) {
       before(() => esArchiver.load(archiveName));
       after(() => esArchiver.unload(archiveName));
 
-      describe('and fetching a list of services', () => {
-        let response: PromiseReturnType<typeof supertest.get>;
-        before(async () => {
-          response = await supertest.get(
-            `/api/apm/services?start=${start}&end=${end}&uiFilters=${uiFilters}`
-          );
-        });
+      describe('with the default APM read user', () => {
+        describe('and fetching a list of services', () => {
+          let response: PromiseReturnType<typeof supertest.get>;
+          before(async () => {
+            response = await supertest.get(
+              `/api/apm/services?start=${start}&end=${end}&uiFilters=${uiFilters}`
+            );
+          });
 
-        it('the response is successful', () => {
-          expect(response.status).to.eql(200);
-        });
+          it('the response is successful', () => {
+            expect(response.status).to.eql(200);
+          });
 
-        it('there is at least one service', () => {
-          expect(response.body.items.length).to.be.greaterThan(0);
-        });
+          it('there is at least one service', () => {
+            expect(response.body.items.length).to.be.greaterThan(0);
+          });
 
-        it('some items have a health status set', () => {
-          // Under the assumption that the loaded archive has
-          // at least one APM ML job, and the time range is longer
-          // than 15m, at least one items should have a health status
-          // set. Note that we currently have a bug where healthy
-          // services report as unknown (so without any health status):
-          // https://github.com/elastic/kibana/issues/77083
+          it('some items have a health status set', () => {
+            // Under the assumption that the loaded archive has
+            // at least one APM ML job, and the time range is longer
+            // than 15m, at least one items should have a health status
+            // set. Note that we currently have a bug where healthy
+            // services report as unknown (so without any health status):
+            // https://github.com/elastic/kibana/issues/77083
 
-          const healthStatuses = response.body.items.map((item: any) => item.healthStatus);
+            const healthStatuses = response.body.items.map((item: any) => item.healthStatus);
 
-          expect(healthStatuses.filter(Boolean).length).to.be.greaterThan(0);
+            expect(healthStatuses.filter(Boolean).length).to.be.greaterThan(0);
 
-          expectSnapshot(healthStatuses).toMatchInline(`
+            expectSnapshot(healthStatuses).toMatchInline(`
             Array [
               "healthy",
               undefined,
@@ -69,6 +71,32 @@ export default function ApiTest({ getService }: FtrProviderContext) {
               "healthy",
             ]
           `);
+          });
+        });
+      });
+
+      describe('with a user that does not have access to ML', () => {
+        let response: PromiseReturnType<typeof supertest.get>;
+        before(async () => {
+          response = await supertestAsApmReadUserWithoutMlAccess.get(
+            `/api/apm/services?start=${start}&end=${end}&uiFilters=${uiFilters}`
+          );
+        });
+
+        it('the response is successful', () => {
+          expect(response.status).to.eql(200);
+        });
+
+        it('there is at least one service', () => {
+          expect(response.body.items.length).to.be.greaterThan(0);
+        });
+
+        it('contains no health statuses', () => {
+          const definedHealthStatuses = response.body.items
+            .map((item: any) => item.healthStatus)
+            .filter(Boolean);
+
+          expect(definedHealthStatuses.length).to.be(0);
         });
       });
     });
