Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[10.x] Throttle exceptions #48391

Merged
merged 4 commits into from Sep 20, 2023
Merged

Conversation

timacdonald
Copy link
Member

@timacdonald timacdonald commented Sep 14, 2023

Adds the ability to sample and rate limit exception reporting.

These two approaches are similar, but differ in their outcome. Sampling is useful to get an "overall" feel for how the exception is impacting your system while rate limiting is useful to now chew through your exception reporting plan threshold when there is a massive spike in errors. Rate limiting also ensures that you know when the error starts happening and continues at a regular interval.

Sampling

<?php

namespace App\Exceptions;

use Aceme\ApiMonitoringException;
use Illuminate\Foundation\Exceptions\Handler as ExceptionHandler;
use Illuminate\Support\Lottery;
use Throwable;

class Handler extends ExceptionHandler
{
    /**
     * Throttle the given exception.
     *
     * @param  \Throwable  $e
     * @return \Illuminate\Support\Lottery|\Illuminate\Cache\RateLimiting\Limit|null
     */
    protected function throttle(Throwable $e)
    {
        if ($e instanceof ApiMonitoringException) {
            return Lottery::odds(1, 1000);
        }
    }


    // ...
}

Sampling exceptions is something that is usually built into exception tracking packages. For example, Sentry offers a sample_rate config option.

I've only ever seen this as a global option, i.e., I cannot sample a specific exception or an exception based on the message.

It also means re-configuring it if you change error tracker.

Lastly, if you are using a log aggregate tool instead, e.g., you report logs to your syslog that something like Papertrail then ingests, you might not have this configuration option specifically for exceptions.

So now we can sample exceptions based on their type or message regardless of the error tracking approach or tooling we are using.

To sample, you may return a Illuminate\Support]Lottery class with the sample rate.

if ($e instanceof ApiMonitoringException) {
    return Lottery::odds(1, 1000);
}

Rate Limiting

<?php

namespace App\Exceptions;

use Illuminate\Broadcasting\BroadcastException;
use Illuminate\Cache\RateLimiting\Limit;
use Illuminate\Foundation\Exceptions\Handler as ExceptionHandler;
use Throwable;

class Handler extends ExceptionHandler
{
    /**
     * Throttle the given exception.
     *
     * @param  \Throwable  $e
     * @return \Illuminate\Support\Lottery|\Illuminate\Cache\RateLimiting\Limit|null
     */
    protected function throttle(Throwable $e)
    {
        if ($e instanceof BroadcastException) {
            return Limit::perMinute(100);
        }
    }

    // ...
}

If a service like Pusher goes down and you are sending a LOT of exceptions, you are going to have a bad time. You quota for exceptions is likely to be exceeded pretty quickly.

With this feature you may rate limit the exceptions to ensure that there is a maximum number of exceptions per minute, hour, or day.

Unlike sampling, you will know as soon as the issue starts and each minute, for this example, you will know if it is continuing. You will have a high fidelity signal that something is wrong without having to flood your exception tracker with potentially thousands of errors per minute.

if ($e instanceof BroadcastException) {
    return Limit::perMinute(100);
}

Customising the rate limit key

By default, the rate limit will be applied against the $exception::class. If you want to limit the exception based on other factors, for example, the exception message, you may use the by method on the limit.

if ($e->getCode() === 987) {
    return Limit::perMinute(5)->by('Error 987');
}

if (str_starts_with($e->getMessage(), 'INTERNAL_ERROR')) {
    return Limit::perMinute(100)->by($e->getMessage());
}

Hashing keys

By default, rate limit keys will be hashed using md5. This is nice as developers can just pass an entire exception message through in the Limit::by method without needing to format it into something sensible.

If you want to turn off hashing to have complete control over the cache keys used you can set the handlers $hashThrottleKeys property to false.

<?php

namespace App\Exceptions;

use Illuminate\Foundation\Exceptions\Handler as ExceptionHandler;
use Throwable;

class Handler extends ExceptionHandler
{
    /**
     * Indicates that throttled keys should be hashed.
     *
     * @var bool
     */
    protected $hashThrottleKeys = false;

    // ...
}

Handling exceptions while throttling

All the throttling logix is wrapped in a rescue to ensure that exceptions that occur while attempting to throttle do not interfere with the reporting of the original exception.

You could imagine a scenario where the cache connection is down. If we attempt to throttle that exception using the cache we are going to end up in an infinite loop.

If an exception occurs while throttling, we do not throttle the original exception and it is reported normally without throttling applied.

Bringing it all together

You may mix and match as needed.

<?php

namespace App\Exceptions;

use Aceme\ApiMonitoringException;
use Illuminate\Broadcasting\BroadcastException;
use Illuminate\Cache\RateLimiting\Limit;
use Illuminate\Foundation\Exceptions\Handler as ExceptionHandler;
use Illuminate\Support\Lottery;
use Throwable;

class Handler extends ExceptionHandler
{
    /**
     * Throttle the given exception.
     *
     * @param  \Throwable  $e
     * @return \Illuminate\Support\Lottery|\Illuminate\Cache\RateLimiting\Limit|null
     */
    protected function throttle(Throwable $e)
    {
        return match (true) {
            $e->getCode() === 987 => Limit::perMinute(5)->by('Error 987'),
            str_starts_with($e->getMessage(), 'INTERNAL_ERROR: ') => Limit::perMinute(60)->by($e->getMessage()),
            $e instanceof ApiMonitoringException => Lottery::odds(1, 1000),
            $e instanceof BroadcastException => Limit::perMinute(100),
            default => Limit::none(),
        };
    }
}

@timacdonald timacdonald marked this pull request as ready for review September 14, 2023 03:12
Comment on lines +359 to +365
return ! $this->container->make(RateLimiter::class)->attempt(
with($throttle->key ?: 'illuminate:foundation:exceptions:'.$e::class, fn ($key) => $this->hashThrottleKeys ? md5($key) : $key),
$throttle->maxAttempts,
fn () => true,
$throttle->decayMinutes
);
}), rescue: false, report: false);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a prefix here when there is not a key specified by the user.

I think that it makes sense to only apply the custom prefix when the framework generates the key, but open to feedback.

Alternative would be to have a protected $cachePrefix key with a value that is applied to both framework generated and user generated keys. Users could then change this value as they want.

@taylorotwell taylorotwell merged commit bbf0b4c into laravel:10.x Sep 20, 2023
20 checks passed
@timacdonald timacdonald deleted the throttle-errors branch September 21, 2023 02:33
@siarheipashkevich
Copy link
Contributor

@timacdonald what about docs for this?

@siarheipashkevich
Copy link
Contributor

siarheipashkevich commented Nov 3, 2023

Hi @timacdonald

I've started using your implementation and found that it's works incorrectly. For example I set throttle like this:

image

But in Sentry I saw that the same errors maybe catched more times then 5 in hour

image

As I can saw it's works incorrectly and I can't trust this logic. What do you think?

Note: I didn't clear any cache or something else in that time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants