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.

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

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

Closed

Conversation

petebacondarwin
Copy link
Member

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

@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer comp: aio target: patch This PR is targeted for the next patch release labels Mar 12, 2018
@IgorMinar IgorMinar mentioned this pull request Mar 12, 2018
34 tasks
@petebacondarwin petebacondarwin added this to REVIEW in docs-infra Mar 12, 2018
@mary-poppins
Copy link

You can preview 4ddfea2 at https://pr22713-4ddfea2.ngbuilds.io/.

logger.error('param1', 'param2', 'param3');
expect(errorHandler.handleError).toHaveBeenCalledWith('param1 param2 param3');
let err: Error;
try { throw new Error('some error message'); } catch (e) { err = e; }
Copy link
Member

Choose a reason for hiding this comment

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

??? 😕
Why not logger.error(new Error('some 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.

Good question! I think I was thinking about checking the stack or something. I'll remove as it doesn't make this test any better.

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

@gkalpak gkalpak added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 12, 2018
@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Mar 13, 2018
@petebacondarwin petebacondarwin moved this from REVIEW to MERGE in docs-infra Mar 13, 2018
@mary-poppins
Copy link

You can preview f3c37b0 at https://pr22713-f3c37b0.ngbuilds.io/.

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
@mary-poppins
Copy link

You can preview abaa31b at https://pr22713-abaa31b.ngbuilds.io/.

kara pushed a commit that referenced this pull request Mar 14, 2018
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
@kara kara closed this in 049757b Mar 14, 2018
@petebacondarwin petebacondarwin removed this from MERGE in docs-infra Mar 14, 2018
@petebacondarwin petebacondarwin deleted the aio-404-error-logging branch March 14, 2018 19:30
This was referenced Mar 15, 2018
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
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
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants