[APM] WrappedElasticsearchClientError: Request aborted (#135752)

* adding error handler

* refactoring

* fixing pr

* fixing PR

* fixing CI

* fixing issue

x-pack/plugins/apm/server/routes/services/annotations/get_derived_service_annotations.test.ts

@@ -0,0 +1,132 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+
+import {
+  ESSearchRequest,
+  ESSearchResponse,
+} from '@kbn/core/types/elasticsearch';
+import {
+  inspectSearchParams,
+  SearchParamsMock,
+} from '../../../utils/test_helpers';
+import { getDerivedServiceAnnotations } from './get_derived_service_annotations';
+import multipleVersions from './__fixtures__/multiple_versions.json';
+import noVersions from './__fixtures__/no_versions.json';
+import oneVersion from './__fixtures__/one_version.json';
+import versionsFirstSeen from './__fixtures__/versions_first_seen.json';
+
+describe('getDerivedServiceAnnotations', () => {
+  let mock: SearchParamsMock;
+
+  afterEach(() => {
+    mock.teardown();
+  });
+
+  describe('with 0 versions', () => {
+    it('returns no annotations', async () => {
+      mock = await inspectSearchParams(
+        (setup) =>
+          getDerivedServiceAnnotations({
+            setup,
+            serviceName: 'foo',
+            environment: 'bar',
+            searchAggregatedTransactions: false,
+            start: 0,
+            end: 50000,
+          }),
+        {
+          mockResponse: () =>
+            noVersions as ESSearchResponse<
+              unknown,
+              ESSearchRequest,
+              {
+                restTotalHitsAsInt: false;
+              }
+            >,
+        }
+      );
+
+      expect(mock.response).toEqual([]);
+    });
+  });
+
+  describe('with 1 version', () => {
+    it('returns no annotations', async () => {
+      mock = await inspectSearchParams(
+        (setup) =>
+          getDerivedServiceAnnotations({
+            setup,
+            serviceName: 'foo',
+            environment: 'bar',
+            searchAggregatedTransactions: false,
+            start: 0,
+            end: 50000,
+          }),
+        {
+          mockResponse: () =>
+            oneVersion as ESSearchResponse<
+              unknown,
+              ESSearchRequest,
+              {
+                restTotalHitsAsInt: false;
+              }
+            >,
+        }
+      );
+
+      expect(mock.response).toEqual([]);
+    });
+  });
+
+  describe('with more than 1 version', () => {
+    it('returns two annotations', async () => {
+      const responses = [
+        multipleVersions,
+        versionsFirstSeen,
+        versionsFirstSeen,
+      ];
+      mock = await inspectSearchParams(
+        (setup) =>
+          getDerivedServiceAnnotations({
+            setup,
+            serviceName: 'foo',
+            environment: 'bar',
+            searchAggregatedTransactions: false,
+            start: 1528113600000,
+            end: 1528977600000,
+          }),
+        {
+          mockResponse: () =>
+            responses.shift() as unknown as ESSearchResponse<
+              unknown,
+              ESSearchRequest,
+              {
+                restTotalHitsAsInt: false;
+              }
+            >,
+        }
+      );
+
+      expect(mock.spy.mock.calls.length).toBe(3);
+
+      expect(mock.response).toEqual([
+        {
+          id: '8.0.0',
+          text: '8.0.0',
+          '@timestamp': new Date('2018-06-04T12:00:00.000Z').getTime(),
+          type: 'version',
+        },
+        {
+          id: '7.5.0',
+          text: '7.5.0',
+          '@timestamp': new Date('2018-06-04T12:00:00.000Z').getTime(),
+          type: 'version',
+        },
+      ]);
+    });
+  });
+});

x-pack/plugins/apm/server/routes/services/annotations/index.test.ts

@@ -4,129 +4,250 @@
  * 2.0; you may not use this file except in compliance with the Elastic License
  * 2.0.
  */
-
 import {
-  ESSearchRequest,
-  ESSearchResponse,
-} from '@kbn/core/types/elasticsearch';
-import {
-  inspectSearchParams,
-  SearchParamsMock,
-} from '../../../utils/test_helpers';
-import { getDerivedServiceAnnotations } from './get_derived_service_annotations';
-import multipleVersions from './__fixtures__/multiple_versions.json';
-import noVersions from './__fixtures__/no_versions.json';
-import oneVersion from './__fixtures__/one_version.json';
-import versionsFirstSeen from './__fixtures__/versions_first_seen.json';
+  ScopedAnnotationsClient,
+  WrappedElasticsearchClientError,
+} from '@kbn/observability-plugin/server';
+import { ElasticsearchClient, Logger } from '@kbn/core/server';
+import { getServiceAnnotations } from '.';
+import { Setup } from '../../../lib/helpers/setup_request';
+import * as GetDerivedServiceAnnotations from './get_derived_service_annotations';
+import * as GetStoredAnnotations from './get_stored_annotations';
+import { Annotation, AnnotationType } from '../../../../common/annotations';
+import { errors } from '@elastic/elasticsearch';
 
 describe('getServiceAnnotations', () => {
-  let mock: SearchParamsMock;
+  const storedAnnotations = [
+    {
+      type: AnnotationType.VERSION,
+      id: '1',
+      '@timestamp': Date.now(),
+      text: 'foo',
+    },
+  ] as Annotation[];
 
   afterEach(() => {
-    mock.teardown();
+    jest.clearAllMocks();
+    jest.resetAllMocks();
   });
 
-  describe('with 0 versions', () => {
-    it('returns no annotations', async () => {
-      mock = await inspectSearchParams(
-        (setup) =>
-          getDerivedServiceAnnotations({
-            setup,
-            serviceName: 'foo',
-            environment: 'bar',
-            searchAggregatedTransactions: false,
-            start: 0,
-            end: 50000,
-          }),
-        {
-          mockResponse: () =>
-            noVersions as ESSearchResponse<
-              unknown,
-              ESSearchRequest,
-              {
-                restTotalHitsAsInt: false;
-              }
-            >,
-        }
+  it('returns stored annotarions even though derived annotations throws an error', async () => {
+    jest
+      .spyOn(GetDerivedServiceAnnotations, 'getDerivedServiceAnnotations')
+      .mockImplementation(
+        () =>
+          new Promise((resolve, reject) => {
+            setTimeout(() => {
+              reject(new Error('BOOM'));
+            }, 20);
+          })
       );
+    jest.spyOn(GetStoredAnnotations, 'getStoredAnnotations').mockImplementation(
+      async () =>
+        new Promise((resolve) => {
+          setTimeout(() => {
+            resolve(storedAnnotations);
+          }, 10);
+        })
+    );
 
-      expect(mock.response).toEqual([]);
+    const annotations = await getServiceAnnotations({
+      serviceName: 'foo',
+      environment: 'bar',
+      searchAggregatedTransactions: false,
+      start: Date.now(),
+      end: Date.now(),
+      client: {} as ElasticsearchClient,
+      logger: {} as Logger,
+      annotationsClient: {} as ScopedAnnotationsClient,
+      setup: {} as Setup,
+    });
+    expect(annotations).toEqual({
+      annotations: storedAnnotations,
     });
   });
 
-  describe('with 1 version', () => {
-    it('returns no annotations', async () => {
-      mock = await inspectSearchParams(
-        (setup) =>
-          getDerivedServiceAnnotations({
-            setup,
-            serviceName: 'foo',
-            environment: 'bar',
-            searchAggregatedTransactions: false,
-            start: 0,
-            end: 50000,
-          }),
-        {
-          mockResponse: () =>
-            oneVersion as ESSearchResponse<
-              unknown,
-              ESSearchRequest,
-              {
-                restTotalHitsAsInt: false;
-              }
-            >,
-        }
+  it('returns stored annotarions even when derived annotations throws an error first', async () => {
+    jest
+      .spyOn(GetDerivedServiceAnnotations, 'getDerivedServiceAnnotations')
+      .mockImplementation(
+        () =>
+          new Promise((resolve, reject) => {
+            setTimeout(() => {
+              reject(new Error('BOOM'));
+            }, 10);
+          })
       );
+    jest.spyOn(GetStoredAnnotations, 'getStoredAnnotations').mockImplementation(
+      async () =>
+        new Promise((resolve) => {
+          setTimeout(() => {
+            resolve(storedAnnotations);
+          }, 20);
+        })
+    );
 
-      expect(mock.response).toEqual([]);
+    const annotations = await getServiceAnnotations({
+      serviceName: 'foo',
+      environment: 'bar',
+      searchAggregatedTransactions: false,
+      start: Date.now(),
+      end: Date.now(),
+      client: {} as ElasticsearchClient,
+      logger: {} as Logger,
+      annotationsClient: {} as ScopedAnnotationsClient,
+      setup: {} as Setup,
+    });
+    expect(annotations).toEqual({
+      annotations: storedAnnotations,
     });
   });
 
-  describe('with more than 1 version', () => {
-    it('returns two annotations', async () => {
-      const responses = [
-        multipleVersions,
-        versionsFirstSeen,
-        versionsFirstSeen,
-      ];
-      mock = await inspectSearchParams(
-        (setup) =>
-          getDerivedServiceAnnotations({
-            setup,
-            serviceName: 'foo',
-            environment: 'bar',
-            searchAggregatedTransactions: false,
-            start: 1528113600000,
-            end: 1528977600000,
-          }),
-        {
-          mockResponse: () =>
-            responses.shift() as unknown as ESSearchResponse<
-              unknown,
-              ESSearchRequest,
-              {
-                restTotalHitsAsInt: false;
-              }
-            >,
-        }
+  it('Throws an exception when derived annotations fires an error before stored annotations is completed and return an empty array', async () => {
+    jest
+      .spyOn(GetDerivedServiceAnnotations, 'getDerivedServiceAnnotations')
+      .mockImplementation(
+        () =>
+          new Promise((resolve, reject) => {
+            setTimeout(() => {
+              reject(new Error('BOOM'));
+            }, 10);
+          })
       );
+    jest.spyOn(GetStoredAnnotations, 'getStoredAnnotations').mockImplementation(
+      async () =>
+        new Promise((resolve) => {
+          setTimeout(() => {
+            resolve([] as Annotation[]);
+          }, 20);
+        })
+    );
 
-      expect(mock.spy.mock.calls.length).toBe(3);
+    expect(
+      getServiceAnnotations({
+        serviceName: 'foo',
+        environment: 'bar',
+        searchAggregatedTransactions: false,
+        start: Date.now(),
+        end: Date.now(),
+        client: {} as ElasticsearchClient,
+        logger: {} as Logger,
+        annotationsClient: {} as ScopedAnnotationsClient,
+        setup: {} as Setup,
+      })
+    ).rejects.toThrow('BOOM');
+  });
 
-      expect(mock.response).toEqual([
-        {
-          id: '8.0.0',
-          text: '8.0.0',
-          '@timestamp': new Date('2018-06-04T12:00:00.000Z').getTime(),
-          type: 'version',
-        },
-        {
-          id: '7.5.0',
-          text: '7.5.0',
-          '@timestamp': new Date('2018-06-04T12:00:00.000Z').getTime(),
-          type: 'version',
-        },
-      ]);
+  it('returns empty derived annotations when RequestAbortedError is thrown and stored annotations is empty', async () => {
+    jest
+      .spyOn(GetDerivedServiceAnnotations, 'getDerivedServiceAnnotations')
+      .mockImplementation(
+        () =>
+          new Promise((resolve, reject) => {
+            setTimeout(() => {
+              reject(
+                new WrappedElasticsearchClientError(
+                  new errors.RequestAbortedError('foo')
+                )
+              );
+            }, 20);
+          })
+      );
+    jest.spyOn(GetStoredAnnotations, 'getStoredAnnotations').mockImplementation(
+      async () =>
+        new Promise((resolve) => {
+          setTimeout(() => {
+            resolve([] as Annotation[]);
+          }, 10);
+        })
+    );
+
+    const annotations = await getServiceAnnotations({
+      serviceName: 'foo',
+      environment: 'bar',
+      searchAggregatedTransactions: false,
+      start: Date.now(),
+      end: Date.now(),
+      client: {} as ElasticsearchClient,
+      logger: {} as Logger,
+      annotationsClient: {} as ScopedAnnotationsClient,
+      setup: {} as Setup,
+    });
+    expect(annotations).toEqual({ annotations: [] });
+  });
+
+  it('Throws an exception when derived annotations fires an error after stored annotations is completed and return an empty array', async () => {
+    jest
+      .spyOn(GetDerivedServiceAnnotations, 'getDerivedServiceAnnotations')
+      .mockImplementation(
+        () =>
+          new Promise((resolve, reject) => {
+            setTimeout(() => {
+              reject(new Error('BOOM'));
+            }, 20);
+          })
+      );
+    jest.spyOn(GetStoredAnnotations, 'getStoredAnnotations').mockImplementation(
+      async () =>
+        new Promise((resolve) => {
+          setTimeout(() => {
+            resolve([] as Annotation[]);
+          }, 10);
+        })
+    );
+
+    expect(
+      getServiceAnnotations({
+        serviceName: 'foo',
+        environment: 'bar',
+        searchAggregatedTransactions: false,
+        start: Date.now(),
+        end: Date.now(),
+        client: {} as ElasticsearchClient,
+        logger: {} as Logger,
+        annotationsClient: {} as ScopedAnnotationsClient,
+        setup: {} as Setup,
+      })
+    ).rejects.toThrow('BOOM');
+  });
+
+  it('returns stored annotations when derived annotations throws RequestAbortedError', async () => {
+    jest
+      .spyOn(GetDerivedServiceAnnotations, 'getDerivedServiceAnnotations')
+      .mockImplementation(
+        () =>
+          new Promise((resolve, reject) => {
+            setTimeout(() => {
+              reject(
+                new WrappedElasticsearchClientError(
+                  new errors.RequestAbortedError('foo')
+                )
+              );
+            }, 20);
+          })
+      );
+    jest.spyOn(GetStoredAnnotations, 'getStoredAnnotations').mockImplementation(
+      async () =>
+        new Promise((resolve) => {
+          setTimeout(() => {
+            resolve(storedAnnotations);
+          }, 10);
+        })
+    );
+
+    const annotations = await getServiceAnnotations({
+      serviceName: 'foo',
+      environment: 'bar',
+      searchAggregatedTransactions: false,
+      start: Date.now(),
+      end: Date.now(),
+      client: {} as ElasticsearchClient,
+      logger: {} as Logger,
+      annotationsClient: {} as ScopedAnnotationsClient,
+      setup: {} as Setup,
+    });
+    expect(annotations).toEqual({
+      annotations: storedAnnotations,
     });
   });
 });

x-pack/plugins/apm/server/routes/services/annotations/index.ts

@@ -7,8 +7,8 @@
 
 import { ElasticsearchClient, Logger } from '@kbn/core/server';
 import { ScopedAnnotationsClient } from '@kbn/observability-plugin/server';
-import { getDerivedServiceAnnotations } from './get_derived_service_annotations';
 import { Setup } from '../../../lib/helpers/setup_request';
+import { getDerivedServiceAnnotations } from './get_derived_service_annotations';
 import { getStoredAnnotations } from './get_stored_annotations';
 
 export async function getServiceAnnotations({
@@ -32,6 +32,9 @@ export async function getServiceAnnotations({
   start: number;
   end: number;
 }) {
+  // Variable to store any error happened on getDerivedServiceAnnotations other than RequestAborted
+  let derivedAnnotationError: Error | undefined;
+
   // start fetching derived annotations (based on transactions), but don't wait on it
   // it will likely be significantly slower than the stored annotations
   const derivedAnnotationsPromise = getDerivedServiceAnnotations({
@@ -41,6 +44,10 @@ export async function getServiceAnnotations({
     searchAggregatedTransactions,
     start,
     end,
+  }).catch((error) => {
+    // Save Error and wait for Stored annotations before re-throwing it
+    derivedAnnotationError = error;
+    return [];
   });
 
   const storedAnnotations = annotationsClient
@@ -56,12 +63,16 @@ export async function getServiceAnnotations({
     : [];
 
   if (storedAnnotations.length) {
-    derivedAnnotationsPromise.catch(() => {
-      // handle error silently to prevent Kibana from crashing
-    });
     return { annotations: storedAnnotations };
   }
 
+  // At this point storedAnnotations returned an empty array,
+  // so if derivedAnnotationError is not undefined throws the error reported by getDerivedServiceAnnotations
+  // and there's no reason to await the function anymore
+  if (derivedAnnotationError) {
+    throw derivedAnnotationError;
+  }
+
   return {
     annotations: await derivedAnnotationsPromise,
   };