Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4179a95
fix(aio): constrain error logging to improve reporting
petebacondarwin Mar 12, 2018
d6a2663
fixup! fix(aio): constrain error logging to improve reporting
petebacondarwin Mar 13, 2018
abaa31b
fixup! fix(aio): constrain error logging to improve reporting
petebacondarwin Mar 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 13 additions & 3 deletions aio/src/app/documents/document.service.spec.ts
View file
Open in desktop
Expand Up @@ -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);
});

Expand All @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions aio/src/app/documents/document.service.ts
View file
Open in desktop
Expand Up @@ -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 {
Expand All @@ -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})`));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OOC, why the parenthesis around error.message?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I parenthesized the original error because it is going to have some implementation specific error information, which not the main message of the error.

this.cache.delete(id);
return Observable.of({
id: FETCHING_ERROR_ID,
Expand Down
10 changes: 8 additions & 2 deletions aio/src/app/embedded/announcement-bar/announcement-bar.component.spec.ts
View file
Open in desktop
Expand Up @@ -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:/);
});
});

Expand Down
4 changes: 2 additions & 2 deletions aio/src/app/embedded/announcement-bar/announcement-bar.component.ts
View file
Open in desktop
Expand Up @@ -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);
Expand Down
4 changes: 4 additions & 0 deletions aio/src/app/embedded/code/code.component.spec.ts
View file
Open in desktop
Expand Up @@ -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:/);
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion aio/src/app/embedded/code/code.component.ts
View file
Open in desktop
Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions aio/src/app/embedded/code/pretty-printer.service.ts
View file
Open in desktop
Expand Up @@ -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); };
});
Expand Down
15 changes: 10 additions & 5 deletions aio/src/app/layout/doc-viewer/doc-viewer.component.spec.ts
View file
Open in desktop
Expand Up @@ -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' });
});
Expand All @@ -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' });
});
Expand All @@ -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' });
});
Expand All @@ -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' });
});
Expand All @@ -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' });
});
Expand Down
2 changes: 1 addition & 1 deletion aio/src/app/layout/doc-viewer/doc-viewer.component.ts
View file
Open in desktop
Expand Up @@ -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$;
Expand Down
5 changes: 3 additions & 2 deletions aio/src/app/shared/logger.service.spec.ts
View file
Open in desktop
Expand Up @@ -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);
});
});
});
Expand Down
5 changes: 2 additions & 3 deletions aio/src/app/shared/logger.service.ts
View file
Open in desktop
Expand Up @@ -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[]) {
Expand Down