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

Add Monolog handler #808

Merged
merged 2 commits into from
May 1, 2019
Merged

Conversation

ste93cry
Copy link
Collaborator

Q A
Branch? 2.1
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
License MIT

This PR implements a Monolog handler and closes #790, which is a highly requested feature. Code is a stripped down version of the one submitted by @HazAT in the official Monolog repository. There are a few questions to discuss before merging:

  • Like in the original PR, the handler needs an hub instance to work, but since the current hub is something that can change during the lifecycle of an application (e.g. when the Hub::setCurrent() method is called) it's not clear how this should affect the handler itself: e.g. if the handler is binded to an old or different hub than the current one it will report errors to the wrong client (if it's different between hubs)
  • Since Monolog is a logger library a message is likely to always be present along with the context that may include the exception and imho we want to log it regardless. Calling the captureMessage method provides no way to pass the exception while calling the captureException has no way to pass the message. To solve the issue, instead of calling those methods like in the original PR, I'm building the raw payload of data to build the event from and I'm using the captureEvent method directly. Since the two methods mentioned before have a slightly different behavior regarding the stacktrace capture (attach_stacktrace option), the same exact behavior cannot be accomplished with such way

Copy link
Member

@HazAT HazAT left a comment

Choose a reason for hiding this comment

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

LGTM, I think it makes sense that the passed hub is used for logging instead of always getting the currentHub.

Copy link
Collaborator

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

Great job, LGTM! 👍 The handler is very clean and slim!

Copy link
Collaborator

@stayallive stayallive left a comment

Choose a reason for hiding this comment

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

Looks good 💪

@Jean85
Copy link
Collaborator

Jean85 commented Apr 29, 2019

We're missing just one small thing! The changelog!

@ste93cry ste93cry force-pushed the feature/monolog-handler branch from 9192e0d to e8f5aa9 Compare May 1, 2019 22:19
@ste93cry
Copy link
Collaborator Author

ste93cry commented May 1, 2019

Done, I always forget to update it!

@ste93cry ste93cry merged commit 59a2113 into getsentry:master May 1, 2019
@ste93cry ste93cry deleted the feature/monolog-handler branch May 1, 2019 22:28
@JanMikes
Copy link
Contributor

JanMikes commented May 1, 2019

Amazing! Thank you 👍

@linaori
Copy link

linaori commented May 14, 2019

Trying to update but am being blocked by this PR not being released. What's the ETA? 😄

@Jean85
Copy link
Collaborator

Jean85 commented May 14, 2019

No ETA, but this will be released with 2.1. Check out the milestone for progress: https://github.com/getsentry/sentry-php/milestone/2

@linaori
Copy link

linaori commented May 14, 2019

Alright, thanks for the reply! I'll check back in a while and stay at sentry/sentry-symfony 2.x for now as it seems to not cause issues with Symfony 4.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[2.0] Monolog
6 participants