Skip to content

Commit 049757b

Browse files
petebacondarwinkara
petebacondarwin
authored and
kara
committed
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
1 parent 1f97343 commit 049757b

11 files changed

+48
-23
lines changed

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

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -69,15 +69,20 @@ describe('DocumentService', () => {
6969
it('should emit the not-found document if the document is not found on the server', () => {
7070
let currentDocument: DocumentContents|undefined;
7171
const notFoundDoc = { id: FILE_NOT_FOUND_ID, contents: '<h1>Page Not Found</h1>' };
72-
const { docService } = getServices('missing/doc');
72+
const { docService, logger } = getServices('missing/doc');
7373
docService.currentDocument.subscribe(doc => currentDocument = doc);
7474

7575
// Initial request return 404.
7676
httpMock.expectOne({}).flush(null, {status: 404, statusText: 'NOT FOUND'});
77+
expect(logger.output.error).toEqual([
78+
[jasmine.any(Error)]
79+
]);
80+
expect(logger.output.error[0][0].message).toEqual(`Document file not found at 'missing/doc'`);
7781

7882
// Subsequent request for not-found document.
83+
logger.output.error = [];
7984
httpMock.expectOne(CONTENT_URL_PREFIX + 'file-not-found.json').flush(notFoundDoc);
80-
85+
expect(logger.output.error).toEqual([]); // does not report repeate errors
8186
expect(currentDocument).toEqual(notFoundDoc);
8287
});
8388

@@ -102,12 +107,17 @@ describe('DocumentService', () => {
102107
let latestDocument: DocumentContents|undefined;
103108
const doc1 = { contents: 'doc 1' };
104109
const doc2 = { contents: 'doc 2' };
105-
const { docService, locationService } = getServices('initial/doc');
110+
const { docService, locationService, logger } = getServices('initial/doc');
106111

107112
docService.currentDocument.subscribe(doc => latestDocument = doc);
108113

109114
httpMock.expectOne({}).flush(null, {status: 500, statusText: 'Server Error'});
110115
expect(latestDocument!.id).toEqual(FETCHING_ERROR_ID);
116+
expect(logger.output.error).toEqual([
117+
[jasmine.any(Error)]
118+
]);
119+
expect(logger.output.error[0][0].message)
120+
.toEqual(`Error fetching document 'initial/doc': (Http failure response for generated/docs/initial/doc.json: 500 Server Error)`);
111121

112122
locationService.go('new/doc');
113123
httpMock.expectOne({}).flush(doc1);

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ export class DocumentService {
7878

7979
private getFileNotFoundDoc(id: string): Observable<DocumentContents> {
8080
if (id !== FILE_NOT_FOUND_ID) {
81-
this.logger.error(`Document file not found at '${id}'`);
81+
this.logger.error(new Error(`Document file not found at '${id}'`));
8282
// using `getDocument` means that we can fetch the 404 doc contents from the server and cache it
8383
return this.getDocument(FILE_NOT_FOUND_ID);
8484
} else {
@@ -90,7 +90,7 @@ export class DocumentService {
9090
}
9191

9292
private getErrorDoc(id: string, error: HttpErrorResponse): Observable<DocumentContents> {
93-
this.logger.error('Error fetching document', error);
93+
this.logger.error(new Error(`Error fetching document '${id}': (${error.message})`));
9494
this.cache.delete(id);
9595
return Observable.of({
9696
id: FETCHING_ERROR_ID,

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,15 +66,21 @@ describe('AnnouncementBarComponent', () => {
6666
const request = httpMock.expectOne('generated/announcements.json');
6767
request.flush('some random response');
6868
expect(component.announcement).toBeUndefined();
69-
expect(mockLogger.output.error[0][0]).toContain('generated/announcements.json contains invalid data:');
69+
expect(mockLogger.output.error).toEqual([
70+
[jasmine.any(Error)]
71+
]);
72+
expect(mockLogger.output.error[0][0].message).toMatch(/^generated\/announcements\.json contains invalid data:/);
7073
});
7174

7275
it('should handle a failed request for `announcements.json`', () => {
7376
component.ngOnInit();
7477
const request = httpMock.expectOne('generated/announcements.json');
7578
request.error(new ErrorEvent('404'));
7679
expect(component.announcement).toBeUndefined();
77-
expect(mockLogger.output.error[0][0]).toContain('generated/announcements.json request failed:');
80+
expect(mockLogger.output.error).toEqual([
81+
[jasmine.any(Error)]
82+
]);
83+
expect(mockLogger.output.error[0][0].message).toMatch(/^generated\/announcements\.json request failed:/);
7884
});
7985
});
8086

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,12 +59,12 @@ export class AnnouncementBarComponent implements OnInit {
5959
ngOnInit() {
6060
this.http.get<Announcement[]>(announcementsPath)
6161
.catch(error => {
62-
this.logger.error(`${announcementsPath} request failed: ${error.message}`);
62+
this.logger.error(new Error(`${announcementsPath} request failed: ${error.message}`));
6363
return [];
6464
})
6565
.map(announcements => this.findCurrentAnnouncement(announcements))
6666
.catch(error => {
67-
this.logger.error(`${announcementsPath} contains invalid data: ${error.message}`);
67+
this.logger.error(new Error(`${announcementsPath} contains invalid data: ${error.message}`));
6868
return [];
6969
})
7070
.subscribe(announcement => this.announcement = announcement);

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,10 +254,14 @@ describe('CodeComponent', () => {
254254
it('should display an error when copy fails', () => {
255255
const snackBar: MatSnackBar = TestBed.get(MatSnackBar);
256256
const copierService: CopierService = TestBed.get(CopierService);
257+
const logger: TestLogger = TestBed.get(Logger);
257258
spyOn(snackBar, 'open');
258259
spyOn(copierService, 'copyText').and.returnValue(false);
259260
getButton().click();
260261
expect(snackBar.open).toHaveBeenCalledWith('Copy failed. Please try again!', '', { duration: 800 });
262+
expect(logger.error).toHaveBeenCalledTimes(1);
263+
expect(logger.error).toHaveBeenCalledWith(jasmine.any(Error));
264+
expect(logger.error.calls.mostRecent().args[0].message).toMatch(/^ERROR copying code to clipboard:/);
261265
});
262266
});
263267
});

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ export class CodeComponent implements OnChanges {
148148
duration: 800,
149149
});
150150
} else {
151-
this.logger.error('ERROR copying code to clipboard:', code);
151+
this.logger.error(new Error(`ERROR copying code to clipboard: "${code}"`));
152152
// failure snackbar alert
153153
this.snackbar.open('Copy failed. Please try again!', '', {
154154
duration: 800,

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,8 @@ export class PrettyPrinter {
3333
.then(
3434
() => (window as any)['prettyPrintOne'],
3535
err => {
36-
const msg = 'Cannot get prettify.js from server';
37-
this.logger.error(msg, err);
36+
const msg = `Cannot get prettify.js from server: ${err.message}`;
37+
this.logger.error(new Error(msg));
3838
// return a pretty print fn that always fails.
3939
return () => { throw new Error(msg); };
4040
});

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

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -577,8 +577,9 @@ describe('DocViewerComponent', () => {
577577
expect(swapViewsSpy).not.toHaveBeenCalled();
578578
expect(docViewer.nextViewContainer.innerHTML).toBe('');
579579
expect(logger.output.error).toEqual([
580-
[`[DocViewer] Error preparing document 'foo': ${error.stack}`],
580+
[jasmine.any(Error)]
581581
]);
582+
expect(logger.output.error[0][0].message).toEqual(`[DocViewer] Error preparing document 'foo': ${error.stack}`);
582583
expect(TestBed.get(Meta).addTag).toHaveBeenCalledWith({ name: 'googlebot', content: 'noindex' });
583584
expect(TestBed.get(Meta).addTag).toHaveBeenCalledWith({ name: 'robots', content: 'noindex' });
584585
});
@@ -598,8 +599,9 @@ describe('DocViewerComponent', () => {
598599
expect(swapViewsSpy).not.toHaveBeenCalled();
599600
expect(docViewer.nextViewContainer.innerHTML).toBe('');
600601
expect(logger.output.error).toEqual([
601-
[`[DocViewer] Error preparing document 'bar': ${error.stack}`],
602+
[jasmine.any(Error)]
602603
]);
604+
expect(logger.output.error[0][0].message).toEqual(`[DocViewer] Error preparing document 'bar': ${error.stack}`);
603605
expect(TestBed.get(Meta).addTag).toHaveBeenCalledWith({ name: 'googlebot', content: 'noindex' });
604606
expect(TestBed.get(Meta).addTag).toHaveBeenCalledWith({ name: 'robots', content: 'noindex' });
605607
});
@@ -619,8 +621,9 @@ describe('DocViewerComponent', () => {
619621
expect(swapViewsSpy).not.toHaveBeenCalled();
620622
expect(docViewer.nextViewContainer.innerHTML).toBe('');
621623
expect(logger.output.error).toEqual([
622-
[`[DocViewer] Error preparing document 'baz': ${error.stack}`],
624+
[jasmine.any(Error)]
623625
]);
626+
expect(logger.output.error[0][0].message).toEqual(`[DocViewer] Error preparing document 'baz': ${error.stack}`);
624627
expect(TestBed.get(Meta).addTag).toHaveBeenCalledWith({ name: 'googlebot', content: 'noindex' });
625628
expect(TestBed.get(Meta).addTag).toHaveBeenCalledWith({ name: 'robots', content: 'noindex' });
626629
});
@@ -640,8 +643,9 @@ describe('DocViewerComponent', () => {
640643
expect(swapViewsSpy).toHaveBeenCalledTimes(1);
641644
expect(docViewer.nextViewContainer.innerHTML).toBe('');
642645
expect(logger.output.error).toEqual([
643-
[`[DocViewer] Error preparing document 'qux': ${error.stack}`],
646+
[jasmine.any(Error)]
644647
]);
648+
expect(logger.output.error[0][0].message).toEqual(`[DocViewer] Error preparing document 'qux': ${error.stack}`);
645649
expect(TestBed.get(Meta).addTag).toHaveBeenCalledWith({ name: 'googlebot', content: 'noindex' });
646650
expect(TestBed.get(Meta).addTag).toHaveBeenCalledWith({ name: 'robots', content: 'noindex' });
647651
});
@@ -658,8 +662,9 @@ describe('DocViewerComponent', () => {
658662
expect(swapViewsSpy).toHaveBeenCalledTimes(1);
659663
expect(docViewer.nextViewContainer.innerHTML).toBe('');
660664
expect(logger.output.error).toEqual([
661-
[`[DocViewer] Error preparing document 'qux': ${error}`],
665+
[jasmine.any(Error)]
662666
]);
667+
expect(logger.output.error[0][0].message).toEqual(`[DocViewer] Error preparing document 'qux': ${error}`);
663668
expect(TestBed.get(Meta).addTag).toHaveBeenCalledWith({ name: 'googlebot', content: 'noindex' });
664669
expect(TestBed.get(Meta).addTag).toHaveBeenCalledWith({ name: 'robots', content: 'noindex' });
665670
});

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ export class DocViewerComponent implements DoCheck, OnDestroy {
162162
.do(() => this.docRendered.emit())
163163
.catch(err => {
164164
const errorMessage = (err instanceof Error) ? err.stack : err;
165-
this.logger.error(`[DocViewer] Error preparing document '${doc.id}': ${errorMessage}`);
165+
this.logger.error(new Error(`[DocViewer] Error preparing document '${doc.id}': ${errorMessage}`));
166166
this.nextViewContainer.innerHTML = '';
167167
this.setNoIndex(true);
168168
return this.void$;

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,9 @@ describe('logger service', () => {
3434

3535
describe('error', () => {
3636
it('should delegate to ErrorHandler', () => {
37-
logger.error('param1', 'param2', 'param3');
38-
expect(errorHandler.handleError).toHaveBeenCalledWith('param1 param2 param3');
37+
const err = new Error('some error message');
38+
logger.error(err);
39+
expect(errorHandler.handleError).toHaveBeenCalledWith(err);
3940
});
4041
});
4142
});

0 commit comments

Comments
 (0)