interceptors

[TomerAberbach]

No description provided.

[AbhiPrasad]

In general this pattern seems good, I just have two items I'd like see addressed

1. How should exceptions be handled here? Should we add for the ability for interceptors to transform the thrown exception (or add metadata?). Ideally request errors are differentiated between response errors. On the same vein, what behaviour should happen if the interceptor throws an error? IMO we should swallow but I think other options are fine
2. It would be nice if the interceptor doesn't have to differentiate between async and sync calls, but without reflection I guess it's hard to make this happen. The API feels like extra complexity to add to a user because they'll often end up duplicating their implementation (it's a 99 percentile case IMO to have to change interceptor behaviour between sync and async)

[AbhiPrasad]

Many users are used to chaining interceptors (or middleware) instead of one big call. Given this can also be done conditionally (for perf reasons for ex.), I think it makes for a better API than enforce all the wrapping in a single call.

Ex. https://square.github.io/okhttp/features/interceptors/

but you can also see this generally with python, js, ruby, java server library middlewares

[TomerAberbach]

We actually looked at the OkHttp API and considered using the same chaining API, but we felt that the chaining API makes it more difficult to understand the order things run. i.e. does addInterceptor(A).addInterceptor(B) result in A(B(...)) or B(A(...)), while if you do the wrapping yourself the order is crystal clear.

It's possible we're overthinking this though; in your experience have folks been confused about ordering of these kinds of things?

[AbhiPrasad]

in your experience have folks been confused about ordering of these kinds of things

There's a general understanding about top-to-bottom/left-to-right ordering, like with GRPC or middleware chains for web server frameworks (think Express for JavaScript or Gin for Go) in most programming language ecosystems. I wouldn't worry about this as an issue from my experience working on the Sentry SDKs.

[TomerAberbach]

Thanks for the great feedback!

In general this pattern seems good, I just have two items I'd like see addressed

Nice!

1. How should exceptions be handled here? Should we add for the ability for interceptors to transform the thrown exception (or add metadata?). Ideally request errors are differentiated between response errors. On the same vein, what behaviour should happen if the interceptor throws an error? IMO we should swallow but I think other options are fine

This is a good callout!

At this layer of the SDK the only exceptions that can be thrown are IOException instances, I think. After this layer, status codes from the HttpResponse are translated to thrown exceptions (see here). So if a user writes an interceptor that changes the returned response's status code, then that would affect the exceptions thrown (or not thrown) later.

For metadata, the exceptions contain the error body (e.g. here), so users should be able to include details in the response if they want.

For request vs response errors, do you mean 4xx errors vs 5xx errors? If so, then yeah, that should be handled by the exception hierarchy linked above.

For if the interceptor throws, my feeling is the exception should propagate and blow things up if it's not caught. If the user wants the interceptor to be ultra resilient, then they should catch the appropriate exceptions. It's also worth noting that if the user specifies an interceptor (instead of a networkInteceptor), then it's subject to retries for thrown IOException or RetryableException.

2. It would be nice if the interceptor doesn't have to differentiate between async and sync calls, but without reflection I guess it's hard to make this happen. The API feels like extra complexity to add to a user because they'll often end up duplicating their implementation (it's a 99 percentile case IMO to have to change interceptor behaviour between sync and async)

Yeah, this is a good point...

I think the challenge I'm grappling with is ensuring that users don't perform blocking work in the async path. i.e. if we provide an API where you can "write it once" in a synchronous manner, then users may perform blocking work, which would be used for the async path too.

I think maybe I'm a little less concerned about the "double API" personally because most users are only using the async client or only using the sync client, so if they just care about one, then they can use Interceptor.syncOnly or Interceptor.asyncOnly.

---

Looking forward to your follow-up thoughts!

[AbhiPrasad]

For request vs response errors, do you mean 4xx errors vs 5xx errors? If so, then yeah, that should be handled by the exception hierarchy linked above.

yup, perfect that sounds good to me!

I'd add a quick note about general exception behaviour to the docs.

I think the challenge I'm grappling with is ensuring that users don't perform blocking work in the async path

Great point, translating that is not easy in Java or even .NET. I guess I'm stuck in dynamic language world where this is a bit easier to massage, with some performance caveats you need to present to users.

I think the single API for interceptors is a nice to have, but I wouldn't block this PR on those changes. We've done the dual async/sync split in many of the Sentry SDK APIs and you just end up having the burden of answering some support GitHub issues every couple of months.