rfc: SDKs report file I/O on the main thread #40
Conversation
The aim of this RFC is to revisit the decision that Ingest creates performance issues, not SDKs.
Co-authored-by: Adam McKerlie <adammckerlie@gmail.com>
- Address some of Adam's comments
| 2. Double dipping quotas, sending the transaction and the error created within that transaction. | ||
| 3. Not able to use dynamic thresholds and configurations, code changes would be required to update settings. | ||
| 4. Stack traces are inherently expensive to process | ||
| 5. Detection is mixed between ingest and SDK |
There was a problem hiding this comment.
My gut feeling is detection is going to happen in many different ways over time (starting as soon as we want to detect something that doesn't fit the span detector model) - this might be inevitable (and therefore not a large concern).
|
|
||
| 1. Users have to enable performance monitoring. | ||
| 2. A transaction is required to be running while a performance issue is happening. | ||
| 3. Transactions are high volume which means we may send too many stack traces, so we can't have stacktraces only the span that caused the issue. Which makes it harder to identify the root cause. |
There was a problem hiding this comment.
We don't have stacktraces at all in transactions, right?
There was a problem hiding this comment.
The protocol would support it, though, as transactions are events.
|
I changed the RFC quite a bit by adding an extra option. I have a hard time finding pros for option 2. @mjq-sentry, as you advocated for that (#40 (comment)), could you please point out some pros? |
|
|
||
| ### Cons <a name="option-2-cons"></a> | ||
|
|
||
| 1. Need for per-SDK rollout. |
There was a problem hiding this comment.
In this case is that just 2 SDKs (Android and Cocoa) or would it be more? (don't know if e.g. react-native would need its own implementation as well)
There was a problem hiding this comment.
That applies to every mobile or desktop SDK. As far as I know, RN, Flutter, and .NET would also need implementations.
|
So, there are a few things that were outlined in the original decision that never made it to long-term storage that I think is necessary for this discussion.
While we are thinking of FIOMT primarily on mobile it's also possible on desktop applications and javascript (node). Building the detector at the Ingest layer means it could detect any SDK that sends a span with |
|
Thanks, @silent1mezzo, I added your input to the RFC with 75962ad |
|
After reading through the RFC and discussion with a number of engineers the best option for File I/O on the main thread performance issues is Option 3: Ingest reports FIOMT as performance issues. Option 1 was ruled out as this is not an error issue as per the definition I mentioned. Option 3 was chosen over Option 2 because we can easily detect it via spans (PR is already complete) and the problem exists across multiple SDKs (iOS, Android, RN, Flutter, .NET and possibly more). There's also a likely future where FIOMT is detected via profiles and running detection in the ingestion pipeline will help combine Performance and Profiling issues. |
|
@silent1mezzo, please review the documented decision and either give feedback or approve this PR. Thanks 🙏 |
This RFC aims to revisit the decision that Ingest creates file I/O on the main thread performance issues and not the SDKs.
Rendered RFC