How can we add monolog context to Sentry

devnix commented 3 years ago

Core or SDK?

Platform/SDK

Which part? Which one?

sentry/sentry-symfony package

Description

In the Symfony integration guide there is some documentation to integrate it with Monolog: https://docs.sentry.io/platforms/php/guides/symfony/

I'm going crazy between closed issues and non-working examples for old versions of the SDK to send a message with Monolog, adding the message's own context, for example:

$this->logger->error('Unexpected entity state.', ['state' => $state, 'entity' => $entity]);

Suggested Solution

Add a definitive documentation for current version to add the capacity to log Monolog's message context to Sentry's log

imatwawana commented 3 years ago

@stayallive is this something you can help with?

getsentry-release commented 3 years ago

Routing to @getsentry/team-webplatform for triage. ⏲️

stayallive commented 3 years ago

@Jean85 / @ste93cry since this is partly Symfony specific are you able to get me some info about how this should work or what should be explained (or maybe even PR this to the docs yourselfs) so we can document this?

Jean85 commented 3 years ago

The point is that by default (and by design as said in ) we do not copy context from Monolog in the base SDK. So we need a basic example for the PHP SDK first, and the we can add on top how to fit that in a Symfony app.

I don't recall exactly what you need to do for the first step, but ping me for the second one.

stayallive commented 3 years ago

@Jean85 you mean just like this: https://docs.sentry.io/platforms/php/enriching-events/context/ or referring to Monolog specifically or are they the same?

Jean85 commented 3 years ago

Yes I meant specific code related to Monolog.

Ok I've dug a bit here and there, and here's what I've found:

there's a 3rd party package to do it with the older SDK: https://github.com/B-Galati/monolog-sentry-handler but it only adds the exception if found in the context: https://github.com/B-Galati/monolog-sentry-handler/blob/6e34b9e2ad4e196bd2cfe54187b795fea3060e05/src/SentryHandler.php#L97-L99
This originated the package: https://github.com/getsentry/sentry-php/pull/844
This is the PR where we shut down the idea of parsing extra by default: https://github.com/getsentry/sentry-php/pull/848
You already wrote it for Laravel! https://github.com/getsentry/sentry-laravel/blob/master/src/Sentry/Laravel/SentryHandler.php

So... Seeing such high demand, IMHO we could revisit this issue and add the implementation in the SDK, but NOT use it by default. At a first glance, your Laravel seems framework agnostic, so we could simply port it in the base SDK (along with tests) and make it optional; at that point, it would be straightforward to make it a configuration option in the Symfony bundle too.

WDYT @ste93cry?

ste93cry commented 3 years ago

it only adds the exception if found in the context

We do the same, the exception key is reserved by Monolog so it's one of the few things we can consider safe

so we could simply port it in the base SDK (along with tests) and make it optional

I'm not strongly opposed to this, but I also think that we had good reasons in the past to deny implementing this out-of-the-box. Moreover, it was @untitaker the first one to suggest that there were similar issues in the Python SDK that led to the decision of not going forward with such feature request. From what I saw in the past, the problem is mainly lack of knowledge about how to implement this on your own (well, it's actually quite simple so I'm not sure what's the issue here) or the laziness of the people in doing it. At the same time, there is clearly an high demand for supporting this. I think that:

Documenting how to implement this would be a great improvement
Making this feature available out of the box makes no sense to me, mainly because as already said here and there blindly copying the data from the logger context to Sentry extras to me is pontless

Jean85 commented 3 years ago

Why do you think it's pointless? It seems to me that people are just trying to avoid duplication, by tying Sentry to Monolog and logging just once, but getting the same info in two places (logs and Sentry). Otherwise you're forced to duplicate code everywhere you log something.

untitaker commented 3 years ago

mainly because as already said here and there blindly copying the data from the logger context to Sentry extras to me is pontless

This is what the Python SDK does, it attaches all logger "context" to sentry extras. However what I would strongly recommend against is to create tags/user context out of logger context based on specially named "context keys"

ste93cry commented 3 years ago

Why do you think it's pointless?

Because as I told a lot of times, in the logger context you can put a lot of data, from whole entities to punctual information, and copying everything blindly to Sentry just pollutes the information by hiding the interesting things, increase the size of the event to be sent and is more likely to make the server answer with the HTTP status code 413. I won't oppose if we want to provide the feature that does it because there is some precedent, but I see little to no value in this: implementing this is simple, you just have to decorate a small class and you're done. Moreover, forcing people to implement it themselves means that hopefully they will do it with a bit of logic, making it easier for them to find what's valuable in the resulting event when they need it

devnix commented 3 years ago

Excuse me, I don't want to sound rude, and I will not pollute the issue section with more comments like this: the request is for documentation, and if I was asking for it was not because of laziness, because I've spent a good couple of days of work and personal time because the effort was worth and valuable to me.

Because as I told a lot of times, in the logger context you can put a lot of data, from whole entities to punctual information, and copying everything blindly to Sentry just pollutes the information by hiding the interesting things, increase the size of the event to be sent and is more likely to make the server answer with the HTTP status code 413. I won't oppose it if we want to provide the feature that does it because there is some precedent, but I see little to no value in this: implementing this is simple, you just have to decorate a small class and you're done. Moreover, forcing people to implement it themselves means that hopefully, they will do it with a bit of logic, making it easier for them to find what's valuable in the resulting event when they need it

At least for me, this argument has been clear enough on each and every one of the issues I've visited, and I've never argued against it, but I was constantly redirected to outdated issues with partially working solutions, and non maintained 3rd party frameworks for previous versions of the current SDK.

Even after that, there has been another amount of work noticing more caveats (for example, decorating the class pollutes the stacktraces making all the registered messages looking like they come from the same place.

Was it easy to fix in the end? Yes. I've spent a lot of hours that I could have saved for resting or working on other things if this was documented? Yes. It's expectable to see people looking for a feature like this, especially if it was supported before? Hell yes. Could it be supported by just activating a flag? In my humble opinion, yes. Would I've been more than happy just with documentation? omgrofllmaoyes.

My personal issue is solved, and I know how to do it (until the next SDK major update I guess). I just wonder how many issues will get reported asking for the same feature support, or a documented alternative, answered each day with bigger desperation, wondering why people are so lazy (or stupid), and reaffirming how pointless is to clarify the decision on the right place and providing the documentation to do it yourself.

stayallive commented 3 years ago

(I have placed some excerpts from https://develop.sentry.dev/sdk/philosophy/ in this comment to clarify a few points and reasoning why that point might be correct for the Sentry SDK)

I have to kind of agree here, I don't see the pushback for an entirely optional feature and/or page in the documentation with just the exact code you can copy/pasta and modify. If you don't like it @ste93cry I can understand, but since it's optional and lot of people do like it I think it's at least worth considering it instead of being a hard no.

And I think adding the class ourselfs or just documenting what you need to create is a very good thing to do considering the amount of questions we get and there not being a definitive answer and indeed every answer is incomplete and expects people to be Monolog/Symfony masters to get started with.

Documentation, guides and marketing material should assume a novice developer unfamiliar with the language. Do not use potentially unfamiliar language features just to aim for shorter examples.

I hoped to get the an answer on how to do it correctly and in the best way (since I don't know it myself, especially in Symfony context) so I could create a doc page. Instead we are discussing if instead of how which is what we should do.

It's okay to do something slightly incorrect if that results in a better customer experience. It's okay for our SDK to ship outdated code if that increases support for more platforms. It's sensible for our SDKs to be doing "things that should not be done" if that results in a better customer experience. For instance we prefer monkeypatching over manual configuration even if those monkeypatches can be brittle in their implementation.

While we generally should try to keep the API surfaces of SDKs reasonable small. At the same time we also need to make sure that we enable customers to achieve their goals. Think about cases that the SDK maybe wouldn't be able to solve out of the box. If there are enough APIs that customers can use our SDKs in more creative ways, we generally see this as a added benefit.

I think adding a doc page with the implementation people can start using is a great place to start, having a class in the SDK people can just require / implement is even better and totally in line with the SDK philosophy IMHO.

So long story short, I think we either document it or built it into the SDK and document how to use it.

Let's make that happen!?

ste93cry commented 3 years ago

if I was asking for it was not because of laziness

My comment was not directed towards you or anyone in particular, it's just a matter of facts that a lot of time requests are opened without people even reading the documentation or trying on their own before and expecting things to just be laid on the table by someone else

Even after that, there has been another amount of work noticing more caveats (for example, decorating the class pollutes the stacktraces making all the registered messages looking like they come from the same place.

I would expect this to happen because you are logging a message without an exception, aren't you? In that case, since there is no stacktrace we can use, we use the debug_backtrace() function and this means that the starting point is the handler itself. It doesn't matter if we implement this on our own or not, either we decide to drop or mark as "non in-app" those first frames or the user will have to do it on his own if he's bothered by this happening. If the Monolog context contained an exception, its stacktrace should be used and I don't expect to see any frame about the handler

My personal issue is solved, and I know how to do it (until the next SDK major update I guess). I just wonder how many issues will get reported asking for the same feature support, or a documented alternative, answered each day with bigger desperation, wondering why people are so lazy (or stupid), and reaffirming how pointless is to clarify the decision on the right place and providing the documentation to do it yourself.

I didn't mean to offend anyone, of course, and I never intended to mean that people are stupid. However, I don't think that there is the need for a manual to use the decorator pattern and with the help of the already available documentation which explains the basic of the SDK come up with a solution. The Symfony documentation instead, is self-explanatory on how to wire up a service decorating another one.

I have to kind of agree here, I don't see the pushback for an entirely optional feature and/or page in the documentation with just the exact code you can copy/pasta and modify. If you don't like it @ste93cry I can understand, but since it's optional and lot of people do like it I think it's at least worth considering it instead of being a hard no.

I think there are two distinct things to say here. First of all, I never said I'm against documenting how to do this, if it helps people and prevents more and more issues being opened regarding the same thing. The second thing is that I'm opposed to adding the feature to the SDK for the reasons I explained before and because I was totally convinced that in the Python SDK this was clearly something unwanted. Maybe I wrongly interpreted the comment from @untitaker since the beginning, maybe things changed in the meanwhile, but if there is a precedent and this precedent is the standard for the SDKs then I see no problem in jumping into the boat even though I think it's the wrong choice

So long story short, I think we either document it or built it into the SDK and document how to use it.

I think that at this point it's clear that the majority wants this feature, users asked for this from when it was dropped from the PHP SDK 2.0 and there are precedent like Laraval and the Python SDK that do this. By having this feature in the core SDK, all users will be able to use it without more work on the framework's integrations.

Disclaimer, just for the sake of clarity: I don't work for Sentry, so those are my opinions as a contributor of the PHP SDK and nothing more

imatwawana commented 3 years ago

I've tagged this as in the backlog for the moment as it seems like some documentation will need to be created here. @stayallive, are you comfortable having this assigned to you?

stayallive commented 3 years ago

Yep sounds good. We are working on some SDK changes and I'll make sure the documentation will be updated once those changes are incorporated and I know what it's going to look like 👍

bastien-wink commented 2 years ago

For those who are still wondering how to add context to sentry log, just add fill_extra_context: true in your monolog config

monolog:
    handlers:
        sentry:
            type: sentry
            level: !php/const Monolog\Logger::ERROR
            hub_id: Sentry\State\HubInterface
            fill_extra_context: true

getsantry[bot] commented 3 months ago

Routing to @getsentry/product-owners-apis for triage ⏲️

getsentry / sentry-docs