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

Fixed #461 Added support for psr/log context exception #582

Merged
merged 3 commits into from
Dec 6, 2022

Conversation

danielmorell
Copy link
Collaborator

Description of the change

This change brings us up to compliance with the PSR-3 logging standard. PSR-3 requires an Exception in the context array must be in the exception key.

For example, the following is a pretty standard non-exception log...

RollbarLogger::log(
    level: Level::INFO, 
    message: "something interesting happened", 
    context: []
);

However, the PSR-3 standard expects us to be able to accept the following log...

RollbarLogger::log(
    level: Level::ERROR, 
    message: "something bad happened", 
    context: ['exception' => new Exception('it broke')]
);

Until now we have treated the exception key the same as any other key. Meaning we would not have created a stack trace of the Exception. We only did that if the Exception was the message.

This change adds a check to determine if the context array contains an exception key and it is a Throwable. If it is we use that as our Exception when generating the trace. To retain as much backwards compatibility as possible Exceptions can still be passed in as the message. In fact, this is the default behavior for our exception handler that automatically catches and logs uncaught exceptions.

This does create a change in how we handle exceptions in the $message. If there is an exception in $context['exception'] we will ignore any exception in $message. Instead, we take the string value of $message and use it as the description of the exception. Until now the description was always omitted.

The last additional change was to update some of our internal type annotations.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Maintenance
  • New release

Related issues

Checklists

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests
  • All tests related to the changed code pass in development

Code review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
  • "Ready for review" label attached to the PR and reviewers assigned
  • Issue from task tracker has a link to this pull request
  • Changes have been reviewed by at least one other engineer

@danielmorell danielmorell added the Type: Bug Fix a component so that its behavior aligns with its documentation. label Dec 6, 2022
@danielmorell danielmorell added this to the v4.0.0 milestone Dec 6, 2022
@danielmorell danielmorell self-assigned this Dec 6, 2022
@danielmorell danielmorell removed their assignment Dec 6, 2022
Copy link
Contributor

@waltjones waltjones left a comment

Choose a reason for hiding this comment

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

@danielmorell nice improvement!

tests/DataBuilderTest.php Outdated Show resolved Hide resolved
@danielmorell danielmorell merged commit 92abb6c into next/4.0/main Dec 6, 2022
@danielmorell danielmorell deleted the 4.0/fixed-psr-log-context-exception branch December 6, 2022 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Fix a component so that its behavior aligns with its documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants