sentry-php icon indicating copy to clipboard operation
sentry-php copied to clipboard

Expose exception severity in data bag

Open kkmuffme opened this issue 3 years ago • 3 comments

Atm, the ExceptionDataBag does not include the exception's severity and it's not possible to get it either, since the exception itself is not in the DataBag too

It would be incredibly useful if either the original exception itself or at least the $exception->getSeverity() were available in the data bag for further, custom processing.

My use case: in setBeforeSendCallback I'd like to overwrite the type: "ErrorException", with a custom string, depending on if the error is a PHP native error (E_NOTICE) or comes from a user triggered error (E_USER_NOTICE) Atm I have to strpos the context_line for USER NOTICE, which is not very robust and leads to quite a few false positives unfortunately.

e.g. simplified:

function ( \Sentry\Event $event ) {
	$exceptions = $event->getExceptions();
	$raw_exception = $exceptions[0]->getException(); // @todo this is what I woudl be ideal, a getSeverity() on it would be fine though
	if ( $raw_exception->getSeverity() === \E_USER_NOTICE ) {
		$exceptions[0]->setType( 'E_USER_NOTICE' ); // @todo ExceptionDataBag.php also needs a setType method added
	}
	
	return $event;	
}

On a side notee: since we use Sentry.io should I open a support ticket with them for faster processing of this request?

kkmuffme avatar Aug 11 '22 09:08 kkmuffme

Opening an issue here is the correct way to request new features 🙂

Can you shed some more light on why you want to see this being added?

cleptric avatar Aug 12 '22 08:08 cleptric

I want to change the title shown (ErrorException) for E_USER_NOTICE (and E_USER_WARNING) to be able to more easily differentiate between PHP native errors and errors triggered by trigger_error

Therefore I'd need to be able to get the exception severity and also "setType" on the exception.

kkmuffme avatar Aug 12 '22 11:08 kkmuffme

Screenshot 2022-08-12 at 14 01 44

So you would like to set the title on the issue details page to something like this, correct?

cleptric avatar Aug 12 '22 12:08 cleptric

Yes (I wouldn't change the message, just the type though, so it would look like this for the title: image and this for the exception: image

kkmuffme avatar Aug 13 '22 09:08 kkmuffme

I like being able to have a little more control over the exception data, so adding a few getters and setter to make that possible get's a 👍 from me, I don't see many downsides.

stayallive avatar Aug 16 '22 10:08 stayallive

The ExceptionDataBag is modeled after the exception interface, so I don't think it would be appropriate to have a field that is not part of it. Looking at the code it seems that we pass the original exception as part of the event hint, which also gets forwarded to the before_send callback, so it should be possible to use $hint->exception to get the severity and replace the message with whatever you want

ste93cry avatar Aug 16 '22 11:08 ste93cry

Someone could indeed get the current and previous exceptions from the $hint object, but I do feel the way this is done feels a bit clunky. Having read access to the exception from the data bag could make the user-land code a bit nicer. Let me follow up with some folks about the interface you pointed out @ste93cry.

Adding ExceptionDataBag::setType() will allow folks to better hook into the SDK, if they so desire, so I'm 👍 on adding this.

cleptric avatar Aug 16 '22 11:08 cleptric

We had a discussion with the SDK folks internally about exposing the exception in the ExceptionDataBag. ~~As our Serializer works explicitly, they're ok to add these new accessors.~~

EDIT: So we're ok with adding new methods to the ExceptionDataBag, however adding new properties has a side effect on event passed to before_send. We actually want the event to represent what is sent to Sentry 1:1, so having the exception property added is currently not something we can do, as we would not serialize it and send it to Sentry.

cleptric avatar Aug 16 '22 15:08 cleptric