It's unclear if LoggerHandler implement overload protection

[martosaur]

Logger docs make it clear that by default there's no overload protection for handlers and imply that it's somewhat recommended to implement it. Sentry.LoggerBackend benefits from it through LoggerBackends, but I don't believe there is any mention of overload protection in LoggerHandler docs? It'd be nice to mention it!

[whatyouhide]

The :rate_limiting option is what you want (https://hexdocs.pm/sentry/Sentry.LoggerHandler.html#module-configuration). Feel free to send a PR with clearer docs if you think that would help. Thanks! 💟

[martosaur]

Thanks! I'll gladly contribute a doc entry, but I'll need to understand the issue first 😅 After comparing Sentry code to Logger Backends, here's my recap:

1. LoggerBackends routes all events through a single gen_event process. Whenever the process has more than sync_threshold messages in its queue (default 20) it switches to sync mode and thus applies backpressure to the application code preventing logger from being starved of cpu resources and losing logs. Whenever number of messages is higher than :discard_threshold (defaults to 500) it begins to drop messages all together.
2. Sentry.LoggerHandler first applies a discard logic in a shape of :rate_limiting + :max_events + :interval (disabled by default). If an event gets past the limit, the execution continues in the same client process all the way up to the moment when it's time to fire off an http request, which is done asynchronously, unless an undocumented :send_result option is set to sync (?). This is essentially where any backpressure ends. All https requests are processed using a pool of processes that fire one request per event.

Does this sound right? And are there any recommended sensible defaults for rate limiting?

[whatyouhide]

@martosaur you are correct in most things you wrote. Logger backends do use Logger's overload protection, which works by dynamically switching between "async" and "sync" Logger calls to prevent overloading.

The Sentry.LoggerHandler, on the other hand, never reports messages to Sentry in a "sync" mode, only "async". But, you have rate limiting to protect you from surges. So, yes if an event passes rate limiting then there is no backpressure, but rate limiting should make that case pretty unlikely.

unless an undocumented :send_result option is set to sync (?).

That's not fair (docs) 😉

Next Steps

We can implement surge protection too if that's something we want. This is not an expected feature (as per Sentry's docs), but it might make sense, I’m not sure. Thoughts?

And are there any recommended sensible defaults for rate limiting?

Depends on your HTTP connection pool mostly.

[martosaur]

We can implement surge protection too if that's something we want. This is not an expected feature (as per Sentry's docs), but it might make sense, I’m not sure. Thoughts?

To be honest, it feels a bit awkward to me to use rate limiting as an overload protection, as they come from different domains? Overload protection is, according to the docs, a mechanism that every erlang logger handler should ideally implement to ensure it's responsive when the system is under load. Rate limiting goal on the other hand is to not overload Sentry.

When we look at the LoggerBackends implementation (and judging by the docs, default logger_std_h handler works in a similar way) we see that it's reactive to the current state of the system: if messages piling up, we enable backpressure. Is they pile up even more, we start to drop them.

The way rate limiting is setup now, it needs an absolute value and it's really really hard to come up with one? I don't think I can predict how many log messages per second my node will generate when it's close to being overwhelmed.

Realistically though, LoggersBackend won't probably go anywhere for the time being, so it might not be worth changing stuff. But it may surely hurt handler adoption, as people like me will read through the Logger docs, see magic words "overload protection" and then look for them in every logger handler they consider hooking in 😄