expose transport exceptions

[aand18]

Send transport exceptions to logger, instead of ignoring, thus allowing for the problem to be noticed and handled.

Currently an exception thrown on line 155 gets swallowed by the "catch" block and causes Sentry events to be discarded silently (e.g. in case of unsuccessful HTTP requests).

https://github.com/getsentry/sentry-php/blob/5b611e3f09035f5ad5edf494443e3236bd5ea482/src/Client.php#L153-L162

This happens when HttpTransport->send() returns a RejectedPromise . Line 155 becomes:

$response = RejectedPromise->wait()

which throws a RejectionException.

[Jean85]

IIRC this was intentional, to avoid creating a loop where, if Sentry is hooked to the logger, a transport failure triggers another event, that triggers a new failure transport...

@ste93cry do I remember this right?

[aand18]

@Jean85 I think this loop already can already happen in HttpTransport:

https://github.com/getsentry/sentry-php/blob/5b611e3f09035f5ad5edf494443e3236bd5ea482/src/Transport/HttpTransport.php#L125-L135

I'm not sure there's a use case for logging library messages to Sentry, sounds like a recipe for failure.

OTOH outright dropping events can become a very time-consuming issue (there's no trace of it anywhere!).

I'd suggest, and please correct me if it doesn't make sense, to either:

• document this possible loop issue e.g. on the setLogger() doc block "Logs library exceptions. Don't use Sentry here because it will cause a loop" or
• provide the possibility to set a second logger for transport exceptions

[ste93cry]

I see one issue with the current implementation: in case of an exception thrown by the HTTP client, we are already logging the error. With the proposed change, we would log it again in the Client class. Moving the logging of the message inside the HttpTransport itself before the last return should be enough though...

I remember this right?

Surely, when we introduced the support for the PSR-3 logger, we were concerned that it could create a loop. However, as @aand18 mentioned, if such logger is misconfigured, it can already happen that we create a recursion, so I don't think that this change is relevant.

I'm not sure there's a use case for logging library messages to Sentry, sounds like a recipe for failure.

Of course, it doesn't make sense to report errors to Sentry if the library itself errored. The logger was put in place as a sort of fallback, so that if something happens inside the SDK, you can still log it to stdout/stderr or to your favourite place.

document this possible loop issue e.g. on the setLogger() doc block "Logs library exceptions. Don't use Sentry here because it will cause a loop" or

Setting a logger is actually an advanced topic, so people should invest time understanding what they do, but I definitely see value in documenting this warning. A contribution to getsentry/sentry-docs would be welcome, in case.

[github-actions[bot]]

This pull request 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 🥀

[aand18]

I see one issue with the current implementation: in case of an exception thrown by the HTTP client, we are already logging the error. With the proposed change, we would log it again in the Client class. Moving the logging of the message inside the HttpTransport itself before the last return should be enough though...

Sorry if I wasn't clear, the exception I'm talking about happens when the destination Sentry server responses with a non-OK code (e.g. "413 Request Entity Too Large"). It's not caused by an HTTP connection error, like "cannot connect" or "DNS resolution failed" etc.

This, in the current code, creates a RejectedPromise in send($event). Then wait() gets called on it and that generates an exception .

https://github.com/getsentry/sentry-php/blob/5b611e3f09035f5ad5edf494443e3236bd5ea482/src/Client.php#L155

This failure to deliver to Sentry doesn't show up anywhere, it disappears in the empty catch block.

I take it more as a bug than anything else. It's completely unexpected.

[aand18]

Of course, it doesn't make sense to report errors to Sentry if the library itself errored. The logger was put in place as a sort of fallback, so that if something happens inside the SDK, you can still log it to stdout/stderr or to your favourite place.

I realise there isn't a trivial solution when checking for infinite recursion detection.

I'm thinking of Laravel, where lots of things get intercepted and sent to Sentry, depending on configuration, from "warning" logs, to exceptions, to errors, and where if you have a "faulty" event (that causes Sentry delivery to fail), you will find it immediately next in a breadcrumb of an event triggered by Log::error(...) which then causes an infinite loop. The same I think is true when a PHP error occurs.

One (maybe naïve) approach would be to put a property on each breadcrumb that references the "causingObject" (an exception, error etc.). Then, before reporting, check that none of the breadcrumbs reference the event's "causingObject". If they do then abort! abort! The downside would be memory usage, because referenced objects don't get garbage collected...

[cleptric]

Closing this for now.