honeymoose
/
angular-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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
This commit is contained in:
Pete Bacon Darwin 2018-03-12 11:05:36 +00:00 committed by Kara Erickson
parent 1f9734315d
commit 049757b237
11 changed files with 48 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
aio/src/app/documents/document.service.spec.ts
View File

@ -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);

4
aio/src/app/documents/document.service.ts
View File

@ -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,

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

@ -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:/);
});
});

4
aio/src/app/embedded/announcement-bar/announcement-bar.component.ts
View File

@ -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);

4
aio/src/app/embedded/code/code.component.spec.ts
View File

@ -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:/);
});
});
});

2
aio/src/app/embedded/code/code.component.ts
View File

@ -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,

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

@ -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); };
});

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

@ -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' });
});

2
aio/src/app/layout/doc-viewer/doc-viewer.component.ts
View File

@ -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$;

5
aio/src/app/shared/logger.service.spec.ts
View File

@ -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);
});
});
});

5
aio/src/app/shared/logger.service.ts
View File

@ -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[]) {