getsentry / getsentry/sentry-java

Make EventProcessors async

Đang mở
#5,478 4 bình luận 0 reaction 0 người được giao Xem trên GitHub
Improvement Java Platform: Java
Ngôn ngữ chính
Kotlin
Star
1.4k
Fork
478
Merge trung bình
2 ngày 23 giờ
Pull request đã merge (30 ngày)
67

Mô tả

I investigated how we could approach this and ended up with two variants:

* A. Moving work to background thread
* B. Adding EventProcessor.processAsync methods

## A. Moving work to background thread

Problems:

* Some EventProcessor work can't be moved to background since we need the current state at time of capture. Adding a queue and delaying things like screen shot capture and view hierarchy would potentially show the wrong data.
* It's hard to find a good spot to move work to background since we currently use scope very far into SentryClient. While we could work around this, that also comes with risks. Cloning scopes to snapshot them could introduce more overhead than we are removing. Handpicking values we need could turn into a maintenance nightmare and cause lots of bugs.
* Using scope async might cause problems due to customers removing things from scope right after capture went through when async processing hasn't run yet. When async processing finally runs, the info is already gone or has been updated to different state.
* beforeSend callbacks are moved to background thread, we'd need an option to allow customers to opt-in until the next major and then flip to default on

I abandoned this idea due to some work that we can't push to background without risking a broken SDK.

## B. Adding EventProcessor processAsync variants

Problems:

* Introduces another Queue we have to maintain including overflow and it creates a new dedicated thread.
* beforeSend callbacks are moved to background thread
* We can add an option isEnableAsyncProcessing which we'll default to false until the next major, then we can flip it and document the breaking change. Customers should be able to move some of their beforeSend code to an EventProcessor so they can still access ThreadLocal state.

How it works:

* One `processAsync` method mirroring each existing `process` method on `EventProcessor`, so 5 new methods in total.
* A new option `enableAsyncProcessing` that defaults to `false` until the next major. We can then document the breaking change, flip to `true` and add guidance on how to migrate existing use cases from `beforeSend` to sync `EventProcessor` methods + `beforeSend` combo.
* If `isEnableAsyncProcessing` is `true` we use a Queue backed by a single thread to move processing to background right before where we currently call `beforeSend`. We first call `processAsync` on all event processors, then `beforeSend`, both from the background thread.
* If `isEnableAsyncProcessing` is `false` we still call `processAsync` before `beforeSend` but do so without moving to a background thread. There is no Queue and no extra thread.

What doesn't make sense:

* Adding a `supportsAsync` method to `EventProcessor` interface and caching the result. The default behaviour of simply returning the incoming event is faster. I tested and it slowed down the SDK. There's also a risk of leaking memory via the cache if customers keep adding short lived event processors.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.