From fe96cafd03ad9eb9f29af2252119d31e3efc3c63 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Mon, 12 Mar 2018 11:05:36 +0000 Subject: [PATCH] fix(aio): constrain error logging to improve reporting (#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 #21943#issuecomment-370230047 PR Close #22713 --- aio/src/app/documents/document.service.spec.ts | 16 +++++++++++++--- aio/src/app/documents/document.service.ts | 4 ++-- .../announcement-bar.component.spec.ts | 10 ++++++++-- .../announcement-bar.component.ts | 4 ++-- aio/src/app/embedded/code/code.component.spec.ts | 4 ++++ aio/src/app/embedded/code/code.component.ts | 2 +- .../app/embedded/code/pretty-printer.service.ts | 4 ++-- .../doc-viewer/doc-viewer.component.spec.ts | 15 ++++++++++----- .../layout/doc-viewer/doc-viewer.component.ts | 2 +- aio/src/app/shared/logger.service.spec.ts | 5 +++-- aio/src/app/shared/logger.service.ts | 5 ++--- 11 files changed, 48 insertions(+), 23 deletions(-) diff --git a/aio/src/app/documents/document.service.spec.ts b/aio/src/app/documents/document.service.spec.ts index 504e120075..f4af31aa4a 100644 --- a/aio/src/app/documents/document.service.spec.ts +++ b/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: '

Page Not Found

' }; - 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); diff --git a/aio/src/app/documents/document.service.ts b/aio/src/app/documents/document.service.ts index 84a4170811..70169fc5d0 100644 --- a/aio/src/app/documents/document.service.ts +++ b/aio/src/app/documents/document.service.ts @@ -78,7 +78,7 @@ export class DocumentService { private getFileNotFoundDoc(id: string): Observable { 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 { - 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, diff --git a/aio/src/app/embedded/announcement-bar/announcement-bar.component.spec.ts b/aio/src/app/embedded/announcement-bar/announcement-bar.component.spec.ts index 009c720c15..068f792ad3 100644 --- a/aio/src/app/embedded/announcement-bar/announcement-bar.component.spec.ts +++ b/aio/src/app/embedded/announcement-bar/announcement-bar.component.spec.ts @@ -66,7 +66,10 @@ 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`', () => { @@ -74,7 +77,10 @@ describe('AnnouncementBarComponent', () => { 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:/); }); }); diff --git a/aio/src/app/embedded/announcement-bar/announcement-bar.component.ts b/aio/src/app/embedded/announcement-bar/announcement-bar.component.ts index ebe511873f..abd88d0b2d 100644 --- a/aio/src/app/embedded/announcement-bar/announcement-bar.component.ts +++ b/aio/src/app/embedded/announcement-bar/announcement-bar.component.ts @@ -59,12 +59,12 @@ export class AnnouncementBarComponent implements OnInit { ngOnInit() { this.http.get(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); diff --git a/aio/src/app/embedded/code/code.component.spec.ts b/aio/src/app/embedded/code/code.component.spec.ts index 91d8ce0db8..bb47c765b2 100644 --- a/aio/src/app/embedded/code/code.component.spec.ts +++ b/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:/); }); }); }); diff --git a/aio/src/app/embedded/code/code.component.ts b/aio/src/app/embedded/code/code.component.ts index ab9877999c..856e9bb040 100644 --- a/aio/src/app/embedded/code/code.component.ts +++ b/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, diff --git a/aio/src/app/embedded/code/pretty-printer.service.ts b/aio/src/app/embedded/code/pretty-printer.service.ts index 28131046c9..cdfed57360 100644 --- a/aio/src/app/embedded/code/pretty-printer.service.ts +++ b/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); }; }); diff --git a/aio/src/app/layout/doc-viewer/doc-viewer.component.spec.ts b/aio/src/app/layout/doc-viewer/doc-viewer.component.spec.ts index aa8d6eade0..6643f1d5e0 100644 --- a/aio/src/app/layout/doc-viewer/doc-viewer.component.spec.ts +++ b/aio/src/app/layout/doc-viewer/doc-viewer.component.spec.ts @@ -555,8 +555,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' }); }); @@ -576,8 +577,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' }); }); @@ -597,8 +599,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' }); }); @@ -618,8 +621,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' }); }); @@ -636,8 +640,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' }); }); diff --git a/aio/src/app/layout/doc-viewer/doc-viewer.component.ts b/aio/src/app/layout/doc-viewer/doc-viewer.component.ts index 7ead74d128..d1ea0efcf7 100644 --- a/aio/src/app/layout/doc-viewer/doc-viewer.component.ts +++ b/aio/src/app/layout/doc-viewer/doc-viewer.component.ts @@ -157,7 +157,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$; diff --git a/aio/src/app/shared/logger.service.spec.ts b/aio/src/app/shared/logger.service.spec.ts index 7094bd3297..a197cdf643 100644 --- a/aio/src/app/shared/logger.service.spec.ts +++ b/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); }); }); }); diff --git a/aio/src/app/shared/logger.service.ts b/aio/src/app/shared/logger.service.ts index 6b6207a41d..2a86d766ea 100644 --- a/aio/src/app/shared/logger.service.ts +++ b/aio/src/app/shared/logger.service.ts @@ -13,9 +13,8 @@ export class Logger { } } - error(value: any, ...rest: any[]) { - const message = [value, ...rest].join(' '); - this.errorHandler.handleError(message); + error(error: Error) { + this.errorHandler.handleError(error); } warn(value: any, ...rest: any[]) {