Security feedback: add warn logging to telemetry collector

see #66922 (comment)
- add if statement
- pass log dependency around (this is kinda medium, should maybe refactor)
- update tests
- move test file comment to the right file (was meant for telemetry route file)

x-pack/plugins/enterprise_search/server/collectors/app_search/telemetry.test.ts

@@ -4,14 +4,20 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
+import { loggingSystemMock } from 'src/core/server/mocks';
+
+jest.mock('../../../../../../src/core/server', () => ({
+  SavedObjectsErrorHelpers: {
+    isNotFoundError: jest.fn(),
+  },
+}));
+import { SavedObjectsErrorHelpers } from '../../../../../../src/core/server';
+
 import { registerTelemetryUsageCollector, incrementUICounter } from './telemetry';
 
-/**
- * Since these route callbacks are so thin, these serve simply as integration tests
- * to ensure they're wired up to the lib functions correctly. Business logic is tested
- * more thoroughly in the lib/telemetry tests.
- */
 describe('App Search Telemetry Usage Collector', () => {
+  const mockLogger = loggingSystemMock.create().get();
+
   const makeUsageCollectorStub = jest.fn();
   const registerStub = jest.fn();
   const usageCollectionMock = {
@@ -42,7 +48,7 @@ describe('App Search Telemetry Usage Collector', () => {
 
   describe('registerTelemetryUsageCollector', () => {
     it('should make and register the usage collector', () => {
-      registerTelemetryUsageCollector(usageCollectionMock, savedObjectsMock);
+      registerTelemetryUsageCollector(usageCollectionMock, savedObjectsMock, mockLogger);
 
       expect(registerStub).toHaveBeenCalledTimes(1);
       expect(makeUsageCollectorStub).toHaveBeenCalledTimes(1);
@@ -53,7 +59,7 @@ describe('App Search Telemetry Usage Collector', () => {
 
   describe('fetchTelemetryMetrics', () => {
     it('should return existing saved objects data', async () => {
-      registerTelemetryUsageCollector(usageCollectionMock, savedObjectsMock);
+      registerTelemetryUsageCollector(usageCollectionMock, savedObjectsMock, mockLogger);
       const savedObjectsCounts = await makeUsageCollectorStub.mock.calls[0][0].fetch();
 
       expect(savedObjectsCounts).toEqual({
@@ -72,10 +78,14 @@ describe('App Search Telemetry Usage Collector', () => {
       });
     });
 
-    it('should not error & should return a default telemetry object if no saved data exists', async () => {
-      const emptySavedObjectsMock = { createInternalRepository: () => ({}) } as any;
+    it('should return a default telemetry object if no saved data exists', async () => {
+      const emptySavedObjectsMock = {
+        createInternalRepository: () => ({
+          get: () => ({ attributes: null }),
+        }),
+      } as any;
 
-      registerTelemetryUsageCollector(usageCollectionMock, emptySavedObjectsMock);
+      registerTelemetryUsageCollector(usageCollectionMock, emptySavedObjectsMock, mockLogger);
       const savedObjectsCounts = await makeUsageCollectorStub.mock.calls[0][0].fetch();
 
       expect(savedObjectsCounts).toEqual({
@@ -93,6 +103,25 @@ describe('App Search Telemetry Usage Collector', () => {
         },
       });
     });
+
+    it('should not throw but log a warning if saved objects errors', async () => {
+      const errorSavedObjectsMock = { createInternalRepository: () => ({}) } as any;
+      registerTelemetryUsageCollector(usageCollectionMock, errorSavedObjectsMock, mockLogger);
+
+      // Without log warning (not found)
+      (SavedObjectsErrorHelpers.isNotFoundError as jest.Mock).mockImplementationOnce(() => true);
+      await makeUsageCollectorStub.mock.calls[0][0].fetch();
+
+      expect(mockLogger.warn).not.toHaveBeenCalled();
+
+      // With log warning
+      (SavedObjectsErrorHelpers.isNotFoundError as jest.Mock).mockImplementationOnce(() => false);
+      await makeUsageCollectorStub.mock.calls[0][0].fetch();
+
+      expect(mockLogger.warn).toHaveBeenCalledWith(
+        'Failed to retrieve App Search telemetry data: TypeError: savedObjectsRepository.get is not a function'
+      );
+    });
   });
 
   describe('incrementUICounter', () => {

x-pack/plugins/enterprise_search/server/collectors/app_search/telemetry.ts

@@ -9,9 +9,13 @@ import {
   ISavedObjectsRepository,
   SavedObjectsServiceStart,
   SavedObjectAttributes,
+  Logger,
 } from 'src/core/server';
 import { UsageCollectionSetup } from 'src/plugins/usage_collection/server';
 
+// This throws `Error: Cannot find module 'src/core/server'` if I import it via alias ¯\_(ツ)_/¯
+import { SavedObjectsErrorHelpers } from '../../../../../../src/core/server';
+
 interface ITelemetry {
   ui_viewed: {
     setup_guide: number;
@@ -35,11 +39,12 @@ export const AS_TELEMETRY_NAME = 'app_search_telemetry';
 
 export const registerTelemetryUsageCollector = (
   usageCollection: UsageCollectionSetup,
-  savedObjects: SavedObjectsServiceStart
+  savedObjects: SavedObjectsServiceStart,
+  log: Logger
 ) => {
   const telemetryUsageCollector = usageCollection.makeUsageCollector<ITelemetry>({
     type: 'app_search',
-    fetch: async () => fetchTelemetryMetrics(savedObjects),
+    fetch: async () => fetchTelemetryMetrics(savedObjects, log),
     isReady: () => true,
     schema: {
       ui_viewed: {
@@ -63,10 +68,11 @@ export const registerTelemetryUsageCollector = (
  * Fetch the aggregated telemetry metrics from our saved objects
  */
 
-const fetchTelemetryMetrics = async (savedObjects: SavedObjectsServiceStart) => {
+const fetchTelemetryMetrics = async (savedObjects: SavedObjectsServiceStart, log: Logger) => {
   const savedObjectsRepository = savedObjects.createInternalRepository();
   const savedObjectAttributes = (await getSavedObjectAttributesFromRepo(
-    savedObjectsRepository
+    savedObjectsRepository,
+    log
   )) as SavedObjectAttributes;
 
   const defaultTelemetrySavedObject: ITelemetry = {
@@ -114,11 +120,15 @@ const fetchTelemetryMetrics = async (savedObjects: SavedObjectsServiceStart) =>
  */
 
 const getSavedObjectAttributesFromRepo = async (
-  savedObjectsRepository: ISavedObjectsRepository
+  savedObjectsRepository: ISavedObjectsRepository,
+  log: Logger
 ) => {
   try {
     return (await savedObjectsRepository.get(AS_TELEMETRY_NAME, AS_TELEMETRY_NAME)).attributes;
   } catch (e) {
+    if (!SavedObjectsErrorHelpers.isNotFoundError(e)) {
+      log.warn(`Failed to retrieve App Search telemetry data: ${e}`);
+    }
     return null;
   }
 };

x-pack/plugins/enterprise_search/server/plugin.ts

@@ -122,7 +122,7 @@ export class EnterpriseSearchPlugin implements Plugin {
         getSavedObjectsService: () => savedObjectsStarted,
       });
       if (usageCollection) {
-        registerTelemetryUsageCollector(usageCollection, savedObjectsStarted);
+        registerTelemetryUsageCollector(usageCollection, savedObjectsStarted, this.logger);
       }
     });
   }

x-pack/plugins/enterprise_search/server/routes/app_search/telemetry.test.ts

@@ -14,6 +14,11 @@ jest.mock('../../collectors/app_search/telemetry', () => ({
 }));
 import { incrementUICounter } from '../../collectors/app_search/telemetry';
 
+/**
+ * Since these route callbacks are so thin, these serve simply as integration tests
+ * to ensure they're wired up to the collector functions correctly. Business logic
+ * is tested more thoroughly in the collectors/telemetry tests.
+ */
 describe('App Search Telemetry API', () => {
   const mockRouter = new MockRouter({ method: 'put', payload: 'body' });
   const mockLogger = loggingSystemMock.create().get();