Change default hook system to use Monolog

[Jean85]

Nicolas, a Symfony Core member, suggested to change approach on how to attach Sentry to Symfony.

This requires a new major version since it's a big, impactful change, and still requires #322 as a fallback mechanism.

[Jean85]

@B-Galati would you like to help here? Due to your work on https://github.com/B-Galati/monolog-sentry-handler, you're probably an expert on this approach...

[tgalopin]

Hello!

This is exactly the reason why I never used this bundle is my own applications before, I'm really glad it's now on the table :) !

I'm posting a comment because I implemented this in most my projects and it may be useful to share it. I think Benoit may even have worked with my implementation on En Marche :) .

I created an open-source project with this implementation, which I didn't share much yet as it's not generic and well-tested enough: https://gitlab.com/citepolitique/oss/sentry.

There are 2 main files in my implementation:

• the handler, actually wiring Sentry with Monolog
• the activation strategy, to avoid logging in Sentry 404 and 403 exceptions

Both are used in Monolog configuration:

sentry:
    dsn: '%env(SENTRY_DSN)%'

monolog:
    handlers:
        main:
            type: fingers_crossed
            action_level: error
            activation_strategy: 'citepolitique.sentry.activation_strategy'
            handler: nested
        nested:
            type: group
            members: [file, buffer]
        file:
            type: stream
            path: '%kernel.logs_dir%/%kernel.environment%.log'
            level: debug
        buffer:
            type: buffer
            handler: sentry
        sentry:
            type: service
            id: 'citepolitique.sentry.monolog_handler'

This example of configuration is great to see why using Monolog is so powerful: in a few lines of configuration, I added an activation strategy to ignore 403/404, created debug log file, created a debug buffer in memory and sent exception to Sentry with the debug context from the buffer if there is a problem. I can update this behavior easily and chain other handlers if I want to easily.

[Jean85]

Thanks for the input @tgalopin! Do you think that that approach is feasible in this bundle? It doesn't seems so to me, but I'm unclear on some inner workings of the Monolog bundle, so I'm probably wrong; more input are more than welcome!

[tgalopin]

I'm not sure why it wouldn't be feasible, I just did it in a bundle myself :) . Are there specific reasons blocking you to do it here?

I feel like a boolean option such as enable_error_handler: true could be enough to enable/disable the current error handler, and then let users rely on Monolog if they want to. We could even have a recipe in the bundle to provide these examples.

[Jean85]

I'm not sure why it wouldn't be feasible, I just did it in a bundle myself :) . Are there specific reasons blocking you to do it here?

But you had to write the Monolog config by yourself, right? What I was expecting to do is:

• the user installs the bundle
• Flex installs the recipe (or the default config kicks in)
• the bundle just works out of the box

How to do that seems a bit foggy to me, since it seems that even @B-Galati did not take this route... Can I just call pushHandler from the bundle without great harm? Or that doesn't sit well with MonologBundle?

I feel like a boolean option such as enable_error_handler: true could be enough to enable/disable the current error handler, and then let users rely on Monolog if they want to. We could even have a recipe in the bundle to provide these examples.

That's the easiest part, there's already a monolog key in the options that right now defaults to disabled, I would just need to flip it.

[B-Galati]

Hello and thanks for pinging me, I am super happy about this issue :-)

I would be happy to help on any subject so feel free to ping me for code review, issues, submitting a PR or helping on a PR.

I explained my approach in this doc: https://github.com/B-Galati/monolog-sentry-handler/blob/master/doc/guide-symfony.md.
It's almost the same as @tgalopin but a bit simpler.

Basically, I did not try yet, but this bundle could already proposed monolog as the default hook system. It would require a doc update and a recipe update.

I deeply think that this bundle could be simplified a lot by being a bit more opinionated. I had a positive feedback here B-Galati/monolog-sentry-handler#19. (ping @temp, you may be interested by this issue).

So for the next major of this bundle I would see :

• Only propose monolog integration => simplest and most efficient solution, I cannot see any drawback, really.
• Any message that goes to the monolog handler must be flushed because that's the expected behavior of a handler. Delaying etc is already handled by combining handlers like fingers crossed and buffer handlers.
• Add a monolog handler in Sentry SDK or in this bundle or use my lib to support breadcrumbs. I still don't get what's the big deal actually.
• Simplify the instanciation of the SentryHub.
• Add a way to explicitly set the http client to be used, do not rely on auto discovery behavior as it is fragile. Using Symfony HTTP Client default looks like a good solution IMHO.

[Jean85]

I would be happy to help on any subject so feel free to ping me for code review, issues, submitting a PR or helping on a PR.

Sure! I'm more than happy to receive help on this. Once we settle on how to proceed, I would say that you can send as many PRs as you want!

Basically, I did not try yet, but this bundle could already proposed monolog as the default hook system. It would require a doc update and a recipe update.

And be released as a new major, or users could bump into unwanted behavior after a silly composer update!

Also: would the recipe be enough to hook into Monolog?

So for the next major of this bundle I would see :

• Only propose monolog integration => simplest and most efficient solution, I cannot see any drawback, really.

The drawback that I see is the absence of Monolog, or wanting to use Monolog for different purposes; anyhow, since #322 is already ready to merge, I woud just merge it and leave it as a fallback method, basically reverting the current bundle behavior (which has Monolog optional)

• Any message that goes to the monolog handler must be flushed because that's the expected behavior of a handler. Delaying etc is already handled by combining handlers like fingers crossed and buffer handlers.

Sure! Also, that could be handled at the transport level, with i.e. spooling.

• Add a monolog handler in Sentry SDK or in this bundle or use my lib to support breadcrumbs. I still don't get what's the big deal actually.

I would add into this bundle to not tie ourselves to the SDK releases. It would be always possible to port it upstream later.

• Simplify the instanciation of the SentryHub.

What do you mean with this?

• Add a way to explicitly set the http client to be used, do not rely on auto discovery behavior as it is fragile. Using Symfony HTTP Client default looks like a good solution IMHO.

That's something baked inside the base SDK. To do that, we would have to inject it in the ClientBuilder, but that's doable. Also, requiring the client directly could lead to some mess if a user uses 3.4 and maybe has Flex? This requires some investigation...

[temp]

@B-Galati thanks for the mention! I put my implementation of your guide here: #337
It is not very nifty, but it fits our needs. Hope the work that's put into this ticket here will make it uncessary ;-)

[B-Galati]

Thanks @temp!

Also: would the recipe be enough to hook into Monolog?

I don't think so, people would need to copy/paste a config example manually. It was the the problem with easy_log for example (https://github.com/symfony/recipes/blob/ce83283b198ddafcb9e0a053d34441723fdb9e3f/easycorp/easy-log-handler/1.0/config/packages/dev/easy_log_handler.yaml)

• Simplify the instanciation of the SentryHub.

What do you mean with this?

I meant having a simple factory class that can handle all cases instead of doing everything within the extension / compiler passes / etc. That's easier to test and maintain. But well that's not the point of this issue, sorry.

@Jean85 Let me know how you would like to move forward ;-)

[Jean85]

@B-Galati I've merged #337 and committed 02e6902, so now master is 4.x only.
IMHO now we need a couple of PRs:

• one to add a full monolog Handler
• one to start bundling up all logs as breadcrumbs
• another one to wire everything up
• one to use the Symfony HTTP client (if possible).. We could try to recover use Symfony HttpClient as client #290 (ping @garak)
• lastly one to switch the approach to prerfer monolog by default (this will need a lot of fixes in the E2E tests

Do you think I'm missing something?

I'll create a branch for 3.x (I have to fix #345) to continue there.

[B-Galati]

LGTM ;-)

Would you like to use my lib as monolog handler or you prefer something built-in the bundle or the SDK?

[Jean85]

Probably having it built-in would make it easier to maintain, but feel free to attribute your work if you want to port it inside!

[B-Galati]

Alright, IMHO better to have it in the SDK. What do you prefer?

[Jean85]

Do you mean committing it into the sentry/sentry-php package? Yeah sure, that's good. We would have to lock against the first version with that in though. It's fine for me.

[Jean85]

Reporting an overlooked issue from #363 (comment):

Using only monolog wouldn't allow to ignore soft fail exceptions. Which is a pretty useful feature.

This regards the Messenger specifically, but I fear that the same issue would arise elsewhere... @B-Galati did you encounter such issues? How did you handle those?

[B-Galati]

This regards the Messenger specifically, but I fear that the same issue would arise elsewhere... @B-Galati did you encounter such issues? How did you handle those?

I am doing something similar as what's currently in the bundle: https://github.com/getsentry/sentry-symfony/blob/c0d0ca35e825b96d1b4976901b2b8f373c30a99f/src/EventListener/MessengerListener.php.

But with a PSR logger.

I did not implement a soft_fails logic because I want to know as soon as one of my worker is failing (It's a strategic business advantage to me).

It's documented here: https://github.com/B-Galati/monolog-sentry-handler/blob/master/doc/guide-symfony.md#step-4-flush-monolog-on-each-handled-symfony-messenger-message.

The trick is to call $this->logger->reset(); on each message consumed.

I am not sure it would be possible to implement the soft_fails as it requires to remove logged messages from monolog handlers.
Perhaps by calling $this->logger->close(); but I think it would send logs to Sentry.

[B-Galati]

It looks like it would be possible with a BufferHandler by calling the method ->clear().

[VincentLanglet]

I did not implement a soft_fails logic because I want to know as soon as one of my worker is failing (It's a strategic business advantage to me).

I understand that sometime it's better without soft_fails.

But in my case I'm calling an API which failing about half the time.
As long as a call is successful, that's enough for me.

If I get a message on Sentry for every soft fail, I will miss the hard fail.

[Jean85]

Sorry to do this, but we can't hold back the 4.0 release with this issue too, it's too big of a change and we're already behind with #399 and #374.

Bumping this to the following major.

[Jean85]

I've just had a possible idea on how to roll out this feature without a major version bump. Let me explain:

• we already have a register_error_listener (default true)
• we had monolog.error_handler.enabled (default false) in 2.x, removed in Remove the config section related to Monolog #406
• we restore the monolog config section, but we make the registration automatic using a compiler pass that runs with the lowest possible priority and calling pushHandler on the logger service (hence ensuring being the first handler of the stack)
• we add another monolog handler (monolog.breadcrumbs_handler) that captures everything into breadcrumbs (maybe in the base SDK?) and we push that handler too
• we add a register_error_handler config option, default true
• we release it in a minor 4.x release, but thanks to the default settings nothing changes
• we test it out and see if anyone reports issues
• we deprecate not setting explicitly the three config options before bumping to 5.x
• in 5.x, we reverse those defaults (register_error_listener: false, register_error_handler: false and monolog.error_handler.enabled: true, monolog.breadcrumb_handler.enabled: true) and remove the deprecation

@ste93cry WDYT?

[Jean85]

Pinning #286, should be interesting to this feature.

[github-actions]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀