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.
I investigated how we could approach this and ended up with two variants:
A. Moving work to background thread
Problems:
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:
How it works:
processAsyncmethod mirroring each existingprocessmethod onEventProcessor, so 5 new methods in total.enableAsyncProcessingthat defaults tofalseuntil the next major. We can then document the breaking change, flip totrueand add guidance on how to migrate existing use cases frombeforeSendto syncEventProcessormethods +beforeSendcombo.isEnableAsyncProcessingistruewe use a Queue backed by a single thread to move processing to background right before where we currently callbeforeSend. We first callprocessAsyncon all event processors, thenbeforeSend, both from the background thread.isEnableAsyncProcessingisfalsewe still callprocessAsyncbeforebeforeSendbut do so without moving to a background thread. There is no Queue and no extra thread.What doesn't make sense:
supportsAsyncmethod toEventProcessorinterface 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.