rfc(decision): mobile-transactions-and-spans #118
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HazAT
approved these changes
Oct 24, 2023
| In Sentry, we need pick apart these special transactions (that we shouldn’t consider as such anymore, maybe use `transaction->transaction_info->source` to identify carrier transaction) and only use their content, which are the spans we measured. | ||
|
|
||
| TBD: | ||
| * How can we ensure that spans are packaged up efficiently, ultimately creating not to little and not too many web requests. |
There was a problem hiding this comment.
I think not a high prio thing to figure out, since eventually it will be Span Ingest.
philipphofmann
approved these changes
Oct 24, 2023
| - Any manually added / background work spans (e.g. a long running sync) could extend the transaction lifetime, which could result in overlapping transactions | ||
| - We need new (manual) APIs to track the current screen | ||
|
|
||
| ### 3. Leave it as-is |
There was a problem hiding this comment.
IMO, this is not an option cause we miss adding auto-instrumented spans to transactions.
|
|
||
| - The transaction duration still makes no sense, as it’s length is determined by the time the screen was visible. Also probably needs a max timeout again too | ||
| - Any manually added / background work spans (e.g. a long running sync) could extend the transaction lifetime, which could result in overlapping transactions | ||
| - We need new (manual) APIs to track the current screen |
There was a problem hiding this comment.
This won't work with multiple screens. Furthermore, this most likely won't work properly for declarative UI frameworks such as Compose or SwiftUI, where identifying the current screen is a challenge.
kahest
approved these changes
Oct 24, 2023
| TBD: | ||
| * How can we ensure that spans are packaged up efficiently, ultimately creating not to little and not too many web requests. | ||
| * As of now, some performance grouping is done based on transactions op and description. With this change they will simply act as carieers, so we need to ensure the span context has enough information that aggregation (e.g. by screen) can still be performed. | ||
| * Profiles are right now captured based on transaction start signals, we need to find a reliable way to keep the remaining functionality. |
There was a problem hiding this comment.
this is a useful callout about Profiling - note that this discussion already happens internally and is not part of the RFC
AbhiPrasad
approved these changes
Oct 24, 2023
shruthilayaj
approved these changes
Oct 24, 2023
Co-authored-by: Philipp Hofmann <ph.hofmann@pm.me> Co-authored-by: Daniel Griesser <daniel.griesser.86@gmail.com> Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This RFC proposes how transaction and spans are used on mobile platforms.
Rendered RFC