Frontend logging: more generic context (#805)

* chore: more generic faro logging, e2e updates
* chore: e2e output console log on retries

src/services/logger.ts

@@ -42,46 +42,39 @@ const attemptFaroWarn = (msg: string, context?: LogContext) => {
   }
 };
 
+const isRecord = (obj: unknown): obj is Record<string, unknown> => typeof obj === 'object';
 /**
- * Checks unknown error for properties from FetchError and adds them to the context
- * Property names from the error are prepended with an underscore when added to the error context sent to Faro
- * Note this renames the "message" to "_errorMessage" in hopes of reducing conflicts of future use of that property name
+ * Checks unknown error for properties from Records like FetchError and adds them to the context
  * @param err
  * @param context
  */
 function populateFetchErrorContext(err: unknown | FetchError, context: LogContext) {
   if (typeof err === 'object' && err !== null) {
-    if (hasTraceId(err) && typeof err.traceId === 'string') {
-      context._traceId = err.traceId;
-    }
-    if (hasMessage(err) && typeof err.message === 'string') {
-      // If we have a conflicting `_errorMessage`, move the original value to `_contextMessage`
-      if (context._errorMessage !== undefined) {
-        context._contextMessage = context._errorMessage;
-      }
-      context._errorMessage = err.message;
-    }
-    // @todo, if the request was cancelled, do we want to log the error?
-    if (hasCancelled(err) && typeof err.cancelled === 'boolean' && context.cancelled) {
-      context._cancelled = err.cancelled.toString();
-    }
-    if (hasStatusText(err) && typeof err.statusText === 'string') {
-      context._statusText = err.statusText;
-    }
-    if (hasHandled(err) && typeof err.isHandled === 'boolean') {
-      context._isHandled = err.isHandled.toString();
+    if (isRecord(err)) {
+      Object.keys(err).forEach((key: string) => {
+        const value = err[key];
+        if (typeof value === 'string' || typeof value === 'boolean' || typeof value === 'number') {
+          context[key] = value.toString();
+        }
+      });
     }
+
     if (hasData(err)) {
-      try {
-        context._data = JSON.stringify(err.data);
-      } catch (e) {
-        // do nothing
+      if (typeof err.data === 'object' && err.data !== null) {
+        try {
+          context.data = JSON.stringify(err.data);
+        } catch (e) {
+          // do nothing
+        }
+      } else if (typeof err.data === 'string' || typeof err.data === 'boolean' || typeof err.data === 'number') {
+        context.data = err.data.toString();
       }
     }
   }
 }
 
-const attemptFaroErr = (err: Error | FetchError | unknown, context: LogContext) => {
+const attemptFaroErr = (err: Error | FetchError | unknown, context2: LogContext) => {
+  let context = context2;
   try {
     populateFetchErrorContext(err, context);
 
@@ -90,10 +83,10 @@ const attemptFaroErr = (err: Error | FetchError | unknown, context: LogContext)
     } else if (typeof err === 'string') {
       logError(new Error(err), context);
     } else if (err && typeof err === 'object') {
-      if (context._errorMessage) {
-        logError(new Error(context._errorMessage), context);
+      if (context.msg) {
+        logError(new Error(context.msg), context);
       } else {
-        logError(new Error('unknown error'), context);
+        logError(new Error('error object'), context);
       }
     } else {
       logError(new Error('unknown error'), context);
@@ -103,21 +96,6 @@ const attemptFaroErr = (err: Error | FetchError | unknown, context: LogContext)
   }
 };
 
-const hasMessage = (value: object): value is { message: unknown } => {
-  return 'message' in value;
-};
-const hasTraceId = (value: object): value is { traceId: unknown } => {
-  return 'traceId' in value;
-};
-const hasStatusText = (value: object): value is { statusText: unknown } => {
-  return 'statusText' in value;
-};
-const hasCancelled = (value: object): value is { cancelled: unknown } => {
-  return 'cancelled' in value;
-};
 const hasData = (value: object): value is { data: unknown } => {
   return 'data' in value;
 };
-const hasHandled = (value: object): value is { isHandled: unknown } => {
-  return 'isHandled' in value;
-};

tests/exploreServices.spec.ts

@@ -3,19 +3,22 @@ import { ExplorePage } from './fixtures/explore';
 import { testIds } from '../src/services/testIds';
 import { mockVolumeApiResponse } from './mocks/mockVolumeApiResponse';
 import { mockLabelsResponse } from './mocks/mockLabelsResponse';
+import { ConsoleMessage } from '@playwright/test';
 
 test.describe('explore services page', () => {
   let explorePage: ExplorePage;
 
-  test.beforeEach(async ({ page }) => {
-    explorePage = new ExplorePage(page);
+  test.beforeEach(async ({ page }, testInfo) => {
+    explorePage = new ExplorePage(page, testInfo);
     await explorePage.setDefaultViewportSize();
-    await page.evaluate(() => window.localStorage.clear());
+    await explorePage.clearLocalStorage();
     await explorePage.gotoServices();
+    explorePage.captureConsoleLogs();
   });
 
   test.afterEach(async ({ page }) => {
-    await page.unrouteAll({ behavior: 'ignoreErrors' });
+    await explorePage.unroute();
+    explorePage.echoConsoleLogsOnRetry();
   });
 
   test('should filter service labels on search', async ({ page }) => {

tests/exploreServicesBreakDown.spec.ts

@@ -10,19 +10,22 @@ const labelName = 'cluster';
 test.describe('explore services breakdown page', () => {
   let explorePage: ExplorePage;
 
-  test.beforeEach(async ({ page }) => {
-    explorePage = new ExplorePage(page);
+  test.beforeEach(async ({ page }, testInfo) => {
+    explorePage = new ExplorePage(page, testInfo);
+
     await explorePage.setExtraTallViewportSize();
-    await page.evaluate(() => window.localStorage.clear());
+    await explorePage.clearLocalStorage();
     await explorePage.gotoServicesBreakdown();
     explorePage.blockAllQueriesExcept({
       refIds: ['logsPanelQuery', fieldName],
       legendFormats: [`{{${levelName}}}`],
     });
+    explorePage.captureConsoleLogs();
   });
 
   test.afterEach(async ({ page }) => {
-    await page.unrouteAll({ behavior: 'ignoreErrors' });
+    await explorePage.unroute();
+    explorePage.echoConsoleLogsOnRetry();
   });
 
   test('should filter logs panel on search for broadcast field', async ({ page }) => {

tests/exploreServicesJsonBreakDown.spec.ts

@@ -7,18 +7,20 @@ const fieldName = 'method';
 test.describe('explore nginx-json breakdown pages ', () => {
   let explorePage: ExplorePage;
 
-  test.beforeEach(async ({ page }) => {
-    explorePage = new ExplorePage(page);
+  test.beforeEach(async ({ page }, testInfo) => {
+    explorePage = new ExplorePage(page, testInfo);
     await explorePage.setExtraTallViewportSize();
-    await page.evaluate(() => window.localStorage.clear());
+    await explorePage.clearLocalStorage();
     await explorePage.gotoServicesBreakdown('nginx-json');
     explorePage.blockAllQueriesExcept({
       refIds: ['logsPanelQuery', fieldName],
     });
+    explorePage.captureConsoleLogs();
   });
 
   test.afterEach(async ({ page }) => {
-    await page.unrouteAll({ behavior: 'ignoreErrors' });
+    await explorePage.unroute();
+    explorePage.echoConsoleLogsOnRetry();
   });
 
   test(`should exclude ${fieldName}, request should contain json`, async ({ page }) => {

tests/exploreServicesJsonMixedBreakDown.spec.ts

@@ -10,18 +10,19 @@ const serviceName = 'nginx-json-mixed';
 test.describe('explore nginx-json-mixed breakdown pages ', () => {
   let explorePage: ExplorePage;
 
-  test.beforeEach(async ({ page }) => {
-    explorePage = new ExplorePage(page);
+  test.beforeEach(async ({ page }, testInfo) => {
+    explorePage = new ExplorePage(page, testInfo);
     await explorePage.setExtraTallViewportSize();
-    await page.evaluate(() => window.localStorage.clear());
+    await explorePage.clearLocalStorage();
     await explorePage.gotoServicesBreakdown(serviceName);
     explorePage.blockAllQueriesExcept({
       refIds: ['logsPanelQuery', mixedFieldName],
     });
   });
 
   test.afterEach(async ({ page }) => {
-    await page.unrouteAll({ behavior: 'ignoreErrors' });
+    await explorePage.unroute();
+    explorePage.echoConsoleLogsOnRetry();
   });
 
   test(`should exclude ${mixedFieldName}, request should contain both parsers`, async ({ page }) => {

tests/fixtures/explore.ts

@@ -1,4 +1,4 @@
-import type { Locator, Page } from '@playwright/test';
+import { ConsoleMessage, Locator, Page, TestInfo } from '@playwright/test';
 import pluginJson from '../../src/plugin.json';
 import { testIds } from '../../src/services/testIds';
 import { expect } from '@grafana/plugin-e2e';
@@ -15,8 +15,9 @@ export class ExplorePage {
   serviceBreakdownSearch: Locator;
   serviceBreakdownOpenExplore: Locator;
   refreshPicker: Locator;
+  logs: Array<{ msg: ConsoleMessage; type: string }> = [];
 
-  constructor(public readonly page: Page) {
+  constructor(public readonly page: Page, public readonly testInfo: TestInfo) {
     this.firstServicePageSelect = this.page.getByText('Select').first();
     this.logVolumeGraph = this.page.getByText('Log volume');
     this.servicesSearch = this.page.getByTestId(testIds.exploreServiceSearch.search);
@@ -25,6 +26,22 @@ export class ExplorePage {
     this.refreshPicker = this.page.getByTestId(testIds.header.refreshPicker);
   }
 
+  captureConsoleLogs() {
+    this.page.on('console', (msg) => {
+      this.logs.push({ msg, type: msg.type() });
+    });
+  }
+
+  echoConsoleLogsOnRetry() {
+    if (this.testInfo.retry > 0) {
+      console.log('logs', this.logs);
+    }
+  }
+
+  async clearLocalStorage() {
+    await this.page.evaluate(() => window.localStorage.clear());
+  }
+
   async setDefaultViewportSize() {
     await this.page.setViewportSize({ width: 1280, height: 720 });
   }
@@ -36,6 +53,13 @@ export class ExplorePage {
     });
   }
 
+  /**
+   * Clears any custom routes created with page.route
+   */
+  async unroute() {
+    await this.page.unrouteAll({ behavior: 'ignoreErrors' });
+  }
+
   async gotoServices() {
     await this.page.goto(`/a/${pluginJson.id}/explore`);
   }