add: breadcrumbs to monolog handler

[B-Galati]

It's very convenient when combined with finger_crossed and/or buffer handler as it gives more context to the error.

Let me know what you think about this idea. I will be happy to add tests and fix comments if you are ok with it.

[B-Galati]

@Jean85 @ste93cry Would you be interested?

[Jean85]

Why would you process breadcrumbs in batch and not add them on the fly, with a simpler flow? Breadcrumbs act like the finger crossed handler, since if no event is sent, no data will reach Sentry...

[ste93cry]

Given that there is the concept of scopes instead of bloating the Monolog handler with the support of every feature that Sentry has wouldn't it makes more sense to just write your own handler that decorates the default one, or even better create a processor that calls configureScope and does what you want to do in that moment?

[B-Galati]

@Jean85 @ste93cry Thank you for your great ideas! I'll give them them a try and keep you in touch ;-)

[B-Galati]

In fact, your proposal could work but would not fit my need since captureEvent is called within the monolog handler handle method which means one new record is created in Sentry for each monolog log.

I would like to have one Sentry event for each HTTP Request (or message Handled for worker) that triggers an errors, Like so I have a complete overview of the error + status code returned to the client.
Also, I would like to manage every errors with monolog since my framework (in my case Symfony) is already taking care of error reporting thanks to monolog: that's the reason why I am not using sentry bundle BTW.

If changing the current monolog handler is problematic, I can create a new one and factorize common piece of code in an abstract class.
In terms of DX, having a monolog handler that manage breadcrumbs out of the box is a nice to have feature from my POV. No custom code needed, no processor: just monolog to configure.

Let me know your thoughts @Jean85 @ste93cry please :-)

[Jean85]

Also, I would like to manage every errors with monolog since my framework (in my case Symfony) is already taking care of error reporting thanks to monolog: that's the reason why I am not using sentry bundle BTW.

What's missing in the bundle that wouldn't work for you? If the handler is used to populate breadcrumbs, having the bundle sending the events will work out of the box anyway...

[B-Galati]

Also, I would like to manage every errors with monolog since my framework (in my case Symfony) is already taking care of error reporting thanks to monolog: that's the reason why I am not using sentry bundle BTW.

What's missing in the bundle that wouldn't work for you? If the handler is used to populate breadcrumbs, having the bundle sending the events will work out of the box anyway...

Relying on Symfony exception event is not what I want. It would miss PHP warning for example.
All the mechanic of logging / error management is perfectly handled by Symfony and Monolog bundle so why reinventing the wheel? I really want to rely as most as I can on PSR log so that Sentry SDK stays an implementation detail.

Another thing, I want to know about recoverable error, i.e. sometime I catch an exception and then logged it as error in order to let my app failing over another system. I could then inform the end-user of a problem without a violent 500 while I still know that something is wrong thanks to Sentry notifications.

Also the bundle is poorly documented and thus does not encourage its usage. I would like to opt-out from Listeners to only integrate what I want for example, the bundle would then only provide the Hub, etc. as a service. In my case, I don't use the bundle and I did a small factory class to instantiate the Hub with the config I want.

PS: @ste93cry Monolog processor are not reliable to capture breadcrumbs as they can miss other processor data. Anyway they are meant to enrich a record not anything else IMHO.

[B-Galati]

@Jean85 @ste93cry

Just saw your message in #790:

in other SDKs the decision was to avoid as much as possible pulling information from structured logging entries

Why? I don't get it.

[B-Galati]

Also what would be the big deal of providing 2 monolog handlers?

[ste93cry]

Why? I don't get it.

To be honest, I still didn't really get it too. I have my personal opinion of course which is just a factor of code to maintain and the fact that being the data unstructured, taking everything and putting it into the event (contexts, tags, whatever) doesn't seems a good idea because there may be a lot of things that you don't care about or don't want to see in Sentry. Anyway, @HazAT or @untitaker are the people that can give a better explanation in this case 😉

Also what would be the big deal of providing 2 monolog handlers?

Apart from having to maintain more code, probably none that comes to my mind

[untitaker]

@B-Galati in Python we have a logging framework in the standard library that is used by every library out there. It allows you to add extra attributes to every record. The Python SDK (both the old and new one) captures all logs from both your app and your libraries. The old SDK mapped some specific attribute names to Sentry concepts, and the new SDK just puts everything into extra/"additional data".

The reason for this change was that a key called transaction in a log record could come from any library (which might have its own idea of what a transaction is). We actually have had support cases where some third-party dependency of an application logged an error, and the resulting event contained random data in random places. It took a while to figure this out.

As a logical consequence we had the feature request to disable this behavior for certain keys. So now you have extra code like in this PR to have "special keys" that mean something in the Sentry event, and then you have more extra code to disable the previous extra code if it breaks stuff. That's a lot of extra code.

I would suggest to use beforeSend to move the data from event.extra to somewhere else if you want to have this behavior. Then you can check some other attributes (logger name) to determine if this is a safe thing to do.

[ste93cry]

The reason for this change was that a key called transaction in a log record could come from any library (which might have its own idea of what a transaction is)

This is the exact reason I initially made the new Monolog handler as simple as possible and only extracted data from the "standard" payload provided by Monolog itself, e.g. the message or the channel name. Not the right place to discuss about, but FYI (ping @JanMikes) there was some discussion about reverting the support for setting the transaction name, the tags and the extra data. I feel extremely sorry for this as you put a lot of effort on coding such features and I want to make it clear that no final decision has been taken yet afaik, but it will be in the near future (for sure before the 2.2 release)

[JanMikes]

Hi, sure, not need to be sorry, only constant thing in life is change 😄.

Though I feel sad for not being (in future) able to configure sentry event using monolog, as i used to always do it like this (in past we used Raven handler everywhere and were happy with it) and will have to write extra code for configuring sentry only, basically as mentioned by @untitaker there will be something that will take everything from monolog's context and put it into sentry. Is it doable? Yes. Is it hard to write? No. But it still takes some extra effort instead of just adding sentry handler to existing application with monolog and having everything working.

But i am still completely okay with that and if this is well documented (maybe this is what i was missing now, recommended way how to use sentry+monolog) it might be very helpful for future users of official sentry monolog handler.

[ste93cry]

there will be something that will take everything from monolog's context and put it into sentry. Is it doable? Yes. Is it hard to write? No. But it still takes some extra effort instead of just adding sentry handler to existing application with monolog and having everything working.

This is the exact point about why we don't want to support anymore such features. Some people may want to do exactly this (blindly copy data from Monolog to Sentry) and some people (like it happened in other SDKs) want to exactly specify what must be copied. After thinking a bit about @untitaker reason I effectively agree with him, better safe than sorry. I'm going to close this issue as it's by now pretty clear the direction we are going to take, but to summarize: everything not in the "main" payload of Monolog is not going to be considered further for feeding information to Sentry and it will be up to the user to decide how and what to extract from there.

[B-Galati]

I would suggest to use beforeSend to move the data from event.extra to somewhere else if you want to have this behavior. Then you can check some other attributes (logger name) to determine if this is a safe thing to do.

Cannot find what is beforeSend.

Fair enough for extra data or any external data that could conflict with any other data but IMHO all of these could be opt-in. Provide the experience you all think is the best by default then let people opt-in.

Though I feel sad for not being (in future) able to configure sentry event using monolog, as i used to always do it like this (in past we used Raven handler everywhere and were happy with it) and will have to write extra code for configuring sentry only, basically as mentioned by @untitaker there will be something that will take everything from monolog's context and put it into sentry. Is it doable? Yes. Is it hard to write? No. But it still takes some extra effort instead of just adding sentry handler to existing application with monolog and having everything working.

I cannot agree more. That's a pity TBH.

@ste93cry Don't you want the SDK to provide the best possible DX?

but to summarize: everything not in the "main" payload of Monolog is not going to be considered further for feeding information to Sentry and it will be up to the user to decide how and what to extract from there.

Good but please consider providing extension point so that one can easily add any info he wants the way he wants.

FYI I will try to release an external package to support the use case of this PR if you are interested @JanMikes.

Thank you all for sharing your thoughts! :-)

Cheers!

[untitaker]

beforeSend would be this extension point... I am not sure why it is not documented at https://docs.sentry.io/learn/filtering/?platform=python#before-send

[ste93cry]

@ste93cry Don't you want the SDK to provide the best possible DX?

Of course I do, but for any feature we add to the library we must consider how much useful it is, the burden of maintaining it and if it can work for everyone and not only for a few people 😉 In this case, the feature depends too much on how people expect things to work in their projects

[B-Galati]

I fully agree and would like to add that the usage of a library is highly oriented by how it is meant to be used in the 1st place ;)

I think we are a lot to do things we don't really like in the 1st place just because the usecase is not supported or not documented/recommended.

[B-Galati]

@untitaker Thanks, I did not find anything related to beforeSend in the PHP SDK sadly.

[Jean85]

It's part of the unified API, it's documented here: https://docs.sentry.io/error-reporting/configuration/?platform=php#before-send

@untitaker IMHO the fact that options docs are splitted this way is creating some friction, this is not the first time that we have issues like this one, see getsentry/sentry-symfony#224

[untitaker]

@Jean85 I suppose so. We are restructuring the docs at the moment so changing this might happen. In the meantime let's make sure we have an example here: https://docs.sentry.io/learn/filtering/?platform=python#before-send

[B-Galati]

Nice thank you :-)

[esetnik]

Maybe we should maintain a separate sentry/sentry-php-monolog integration package which includes both a breadcrumb and error handler? This would solve the problem of code bloat in the core sdk and could become a recommended composer package. For me, having log breadcrumbs leading up to a crash is very useful for debugging.

#790 (comment)

[JanMikes]

FYI I will try to release an external package to support the use case of this PR if you are interested @JanMikes.

@B-Galati feel free to ping me if you start writing package like this.

Maybe we should maintain a separate sentry/sentry-php-monolog integration package which includes both a breadcrumb and error handler?

I was thinking about exactly the same!

[B-Galati]

@JanMikes Done! -> https://github.com/B-Galati/monolog-sentry-handler

Feedbacks are welcomed!

[Wirone]

I have cloned @B-Galati's handler with some tweaks and have running Symfony project with Monolog, that sends logger records to Sentry as breadcrumbs. However, I would like it to send those logger records as breadcrumbs also in exceptions, but at the moment it works only when reporting errors with Monolog's error() method which obviously is correct from technical POV (fingers crossed handler catches all records, when error occurs Sentry handler converts all to breadcrumbs leaving error as main event).

Should I create custom integration (implementing Sentry\Integration\IntegrationInterface)? Use before_send option? Anyone can point me in right direction?

[Jean85]

That's strange to me... Any uncatched exception should bubble up and be reported as error in Monolog.