fix(aio): constrain error logging to improve reporting (angular#22713)

The `Logger.error()` method now only accepts a single `Error` parameter
and passes this through to the error handler.
This allows the error handler to serialize the error more accurately.

The various places that use `Logger.error()` have been updated.

See angular#21943#issuecomment-370230047

PR Close angular#22713

Author: petebacondarwin

aio/src/app/documents/document.service.spec.ts

@@ -69,15 +69,20 @@ describe('DocumentService', () => {
     it('should emit the not-found document if the document is not found on the server', () => {
       let currentDocument: DocumentContents|undefined;
       const notFoundDoc = { id: FILE_NOT_FOUND_ID, contents: '<h1>Page Not Found</h1>' };
-      const { docService } = getServices('missing/doc');
+      const { docService, logger } = getServices('missing/doc');
       docService.currentDocument.subscribe(doc => currentDocument = doc);
 
       // Initial request return 404.
       httpMock.expectOne({}).flush(null, {status: 404, statusText: 'NOT FOUND'});
+      expect(logger.output.error).toEqual([
+        [jasmine.any(Error)]
+      ]);
+      expect(logger.output.error[0][0].message).toEqual(`Document file not found at 'missing/doc'`);
 
       // Subsequent request for not-found document.
+      logger.output.error = [];
       httpMock.expectOne(CONTENT_URL_PREFIX + 'file-not-found.json').flush(notFoundDoc);
-
+      expect(logger.output.error).toEqual([]); // does not report repeate errors
       expect(currentDocument).toEqual(notFoundDoc);
     });
 
@@ -102,12 +107,17 @@ describe('DocumentService', () => {
       let latestDocument: DocumentContents|undefined;
       const doc1 = { contents: 'doc 1' };
       const doc2 = { contents: 'doc 2' };
-      const { docService, locationService } = getServices('initial/doc');
+      const { docService, locationService, logger } = getServices('initial/doc');
 
       docService.currentDocument.subscribe(doc => latestDocument = doc);
 
       httpMock.expectOne({}).flush(null, {status: 500, statusText: 'Server Error'});
       expect(latestDocument!.id).toEqual(FETCHING_ERROR_ID);
+      expect(logger.output.error).toEqual([
+        [jasmine.any(Error)]
+      ]);
+      expect(logger.output.error[0][0].message)
+          .toEqual(`Error fetching document 'initial/doc': (Http failure response for generated/docs/initial/doc.json: 500 Server Error)`);
 
       locationService.go('new/doc');
       httpMock.expectOne({}).flush(doc1);

aio/src/app/documents/document.service.ts

@@ -78,7 +78,7 @@ export class DocumentService {
 
   private getFileNotFoundDoc(id: string): Observable<DocumentContents> {
     if (id !== FILE_NOT_FOUND_ID) {
-      this.logger.error(`Document file not found at '${id}'`);
+      this.logger.error(new Error(`Document file not found at '${id}'`));
       // using `getDocument` means that we can fetch the 404 doc contents from the server and cache it
       return this.getDocument(FILE_NOT_FOUND_ID);
     } else {
@@ -90,7 +90,7 @@ export class DocumentService {
   }
 
   private getErrorDoc(id: string, error: HttpErrorResponse): Observable<DocumentContents> {
-    this.logger.error('Error fetching document', error);
+    this.logger.error(new Error(`Error fetching document '${id}': (${error.message})`));
     this.cache.delete(id);
     return Observable.of({
       id: FETCHING_ERROR_ID,

aio/src/app/embedded/announcement-bar/announcement-bar.component.spec.ts

@@ -66,15 +66,21 @@ describe('AnnouncementBarComponent', () => {
       const request = httpMock.expectOne('generated/announcements.json');
       request.flush('some random response');
       expect(component.announcement).toBeUndefined();
-      expect(mockLogger.output.error[0][0]).toContain('generated/announcements.json contains invalid data:');
+      expect(mockLogger.output.error).toEqual([
+        [jasmine.any(Error)]
+      ]);
+      expect(mockLogger.output.error[0][0].message).toMatch(/^generated\/announcements\.json contains invalid data:/);
     });
 
     it('should handle a failed request for `announcements.json`', () => {
       component.ngOnInit();
       const request = httpMock.expectOne('generated/announcements.json');
       request.error(new ErrorEvent('404'));
       expect(component.announcement).toBeUndefined();
-      expect(mockLogger.output.error[0][0]).toContain('generated/announcements.json request failed:');
+      expect(mockLogger.output.error).toEqual([
+        [jasmine.any(Error)]
+      ]);
+      expect(mockLogger.output.error[0][0].message).toMatch(/^generated\/announcements\.json request failed:/);
     });
   });
 

aio/src/app/embedded/announcement-bar/announcement-bar.component.ts

@@ -59,12 +59,12 @@ export class AnnouncementBarComponent implements OnInit {
   ngOnInit() {
     this.http.get<Announcement[]>(announcementsPath)
       .catch(error => {
-        this.logger.error(`${announcementsPath} request failed: ${error.message}`);
+        this.logger.error(new Error(`${announcementsPath} request failed: ${error.message}`));
         return [];
       })
       .map(announcements => this.findCurrentAnnouncement(announcements))
       .catch(error => {
-        this.logger.error(`${announcementsPath} contains invalid data: ${error.message}`);
+        this.logger.error(new Error(`${announcementsPath} contains invalid data: ${error.message}`));
         return [];
       })
       .subscribe(announcement => this.announcement = announcement);

aio/src/app/embedded/code/code.component.spec.ts

@@ -254,10 +254,14 @@ describe('CodeComponent', () => {
     it('should display an error when copy fails', () => {
       const snackBar: MatSnackBar = TestBed.get(MatSnackBar);
       const copierService: CopierService = TestBed.get(CopierService);
+      const logger: TestLogger = TestBed.get(Logger);
       spyOn(snackBar, 'open');
       spyOn(copierService, 'copyText').and.returnValue(false);
       getButton().click();
       expect(snackBar.open).toHaveBeenCalledWith('Copy failed. Please try again!', '', { duration: 800 });
+      expect(logger.error).toHaveBeenCalledTimes(1);
+      expect(logger.error).toHaveBeenCalledWith(jasmine.any(Error));
+      expect(logger.error.calls.mostRecent().args[0].message).toMatch(/^ERROR copying code to clipboard:/);
     });
   });
 });

aio/src/app/embedded/code/code.component.ts

@@ -148,7 +148,7 @@ export class CodeComponent implements OnChanges {
         duration: 800,
       });
     } else {
-      this.logger.error('ERROR copying code to clipboard:', code);
+      this.logger.error(new Error(`ERROR copying code to clipboard: "${code}"`));
       // failure snackbar alert
       this.snackbar.open('Copy failed. Please try again!', '', {
         duration: 800,

aio/src/app/embedded/code/pretty-printer.service.ts

@@ -33,8 +33,8 @@ export class PrettyPrinter {
         .then(
           () => (window as any)['prettyPrintOne'],
           err => {
-            const msg = 'Cannot get prettify.js from server';
-            this.logger.error(msg, err);
+            const msg = `Cannot get prettify.js from server: ${err.message}`;
+            this.logger.error(new Error(msg));
             // return a pretty print fn that always fails.
             return () => { throw new Error(msg); };
           });

aio/src/app/layout/doc-viewer/doc-viewer.component.spec.ts

@@ -577,8 +577,9 @@ describe('DocViewerComponent', () => {
         expect(swapViewsSpy).not.toHaveBeenCalled();
         expect(docViewer.nextViewContainer.innerHTML).toBe('');
         expect(logger.output.error).toEqual([
-          [`[DocViewer] Error preparing document 'foo': ${error.stack}`],
+          [jasmine.any(Error)]
         ]);
+        expect(logger.output.error[0][0].message).toEqual(`[DocViewer] Error preparing document 'foo': ${error.stack}`);
         expect(TestBed.get(Meta).addTag).toHaveBeenCalledWith({ name: 'googlebot', content: 'noindex' });
         expect(TestBed.get(Meta).addTag).toHaveBeenCalledWith({ name: 'robots', content: 'noindex' });
       });
@@ -598,8 +599,9 @@ describe('DocViewerComponent', () => {
         expect(swapViewsSpy).not.toHaveBeenCalled();
         expect(docViewer.nextViewContainer.innerHTML).toBe('');
         expect(logger.output.error).toEqual([
-          [`[DocViewer] Error preparing document 'bar': ${error.stack}`],
+          [jasmine.any(Error)]
         ]);
+        expect(logger.output.error[0][0].message).toEqual(`[DocViewer] Error preparing document 'bar': ${error.stack}`);
         expect(TestBed.get(Meta).addTag).toHaveBeenCalledWith({ name: 'googlebot', content: 'noindex' });
         expect(TestBed.get(Meta).addTag).toHaveBeenCalledWith({ name: 'robots', content: 'noindex' });
       });
@@ -619,8 +621,9 @@ describe('DocViewerComponent', () => {
         expect(swapViewsSpy).not.toHaveBeenCalled();
         expect(docViewer.nextViewContainer.innerHTML).toBe('');
         expect(logger.output.error).toEqual([
-          [`[DocViewer] Error preparing document 'baz': ${error.stack}`],
+          [jasmine.any(Error)]
         ]);
+        expect(logger.output.error[0][0].message).toEqual(`[DocViewer] Error preparing document 'baz': ${error.stack}`);
         expect(TestBed.get(Meta).addTag).toHaveBeenCalledWith({ name: 'googlebot', content: 'noindex' });
         expect(TestBed.get(Meta).addTag).toHaveBeenCalledWith({ name: 'robots', content: 'noindex' });
       });
@@ -640,8 +643,9 @@ describe('DocViewerComponent', () => {
         expect(swapViewsSpy).toHaveBeenCalledTimes(1);
         expect(docViewer.nextViewContainer.innerHTML).toBe('');
         expect(logger.output.error).toEqual([
-          [`[DocViewer] Error preparing document 'qux': ${error.stack}`],
+          [jasmine.any(Error)]
         ]);
+        expect(logger.output.error[0][0].message).toEqual(`[DocViewer] Error preparing document 'qux': ${error.stack}`);
         expect(TestBed.get(Meta).addTag).toHaveBeenCalledWith({ name: 'googlebot', content: 'noindex' });
         expect(TestBed.get(Meta).addTag).toHaveBeenCalledWith({ name: 'robots', content: 'noindex' });
       });
@@ -658,8 +662,9 @@ describe('DocViewerComponent', () => {
         expect(swapViewsSpy).toHaveBeenCalledTimes(1);
         expect(docViewer.nextViewContainer.innerHTML).toBe('');
         expect(logger.output.error).toEqual([
-          [`[DocViewer] Error preparing document 'qux': ${error}`],
+          [jasmine.any(Error)]
         ]);
+        expect(logger.output.error[0][0].message).toEqual(`[DocViewer] Error preparing document 'qux': ${error}`);
         expect(TestBed.get(Meta).addTag).toHaveBeenCalledWith({ name: 'googlebot', content: 'noindex' });
         expect(TestBed.get(Meta).addTag).toHaveBeenCalledWith({ name: 'robots', content: 'noindex' });
       });

aio/src/app/layout/doc-viewer/doc-viewer.component.ts

@@ -162,7 +162,7 @@ export class DocViewerComponent implements DoCheck, OnDestroy {
         .do(() => this.docRendered.emit())
         .catch(err => {
           const errorMessage = (err instanceof Error) ? err.stack : err;
-          this.logger.error(`[DocViewer] Error preparing document '${doc.id}': ${errorMessage}`);
+          this.logger.error(new Error(`[DocViewer] Error preparing document '${doc.id}': ${errorMessage}`));
           this.nextViewContainer.innerHTML = '';
           this.setNoIndex(true);
           return this.void$;

aio/src/app/shared/logger.service.spec.ts

@@ -34,8 +34,9 @@ describe('logger service', () => {
 
   describe('error', () => {
     it('should delegate to ErrorHandler', () => {
-      logger.error('param1', 'param2', 'param3');
-      expect(errorHandler.handleError).toHaveBeenCalledWith('param1 param2 param3');
+      const err = new Error('some error message');
+      logger.error(err);
+      expect(errorHandler.handleError).toHaveBeenCalledWith(err);
     });
   });
 });