Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref: Amend transaction and spans RFC 118 #125

Merged
merged 2 commits into from
Nov 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,5 +58,5 @@ This repository contains RFCs and DACIs. Lost?
- [0101-revamping-the-sdk-performance-api](text/0101-revamping-the-sdk-performance-api.md): Revamping the SDK Performance API
- [0106-artifact-indices](text/0106-artifact-indices.md): Improvements to Source Maps Processing
- [0116-sentry-semantic-conventions](text/0116-sentry-semantic-conventions.md): Sentry Semantic Conventions
- [0117-mobile-transactions-and-spans](text/0117-mobile-transactions-and-spans.md): Transactions and Spans for Mobile Platforms
- [0118-mobile-transactions-and-spans](text/0118-mobile-transactions-and-spans.md): Transactions and Spans for Mobile Platforms
- [0123-metrics-correlation](text/0123-metrics-correlation.md): This RFC addresses the high level metrics to span correlation system
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,20 @@ As of now the way transactions are created has several drawbacks:

4. Transaction durations are sometimes pointless: As they’re based on idle transaction. Any relevant or even non-relevant child span can influence the transaction duration.

## Option Chosen

On 2023-11-21, we decided to move forward with [4. Single Span Ingestion](#option-4), because single span ingestion will be available soon, and we strongly believe it's the future how SDKs will send spans to Sentry. Participants in the decision:

- Philipp Hofmann
- Karl Heinz Struggl
- Markus Hintersteiner
- Nar Saynorath
- Shruthilaya Jaganathan
- Pierre Massat

# Options Considered

### 1. Transactions as Carriers for Spans (preferred approach)
### 1. Transactions as Carriers for Spans

The idea is to use transactions as carriers for measurements on mobile instead of trying to make the backend heavy concept work on mobile. There is no active "open" transaction, but we rather create transactions on-demand, whenever there is span data to send.
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. In short, we only use transactions to understand which Spans (measurements) should be aggregated.
Expand All @@ -35,28 +46,27 @@ Future considerations:
* As of now, some performance grouping is done based on transactions op and description. With this change, transactions will simply act as carriers, thus we need to ensure the span context has enough information that aggregation (e.g. by screen) can still be performed.
* Profiles are bound to transactions via a 1:1 mapping right now, we'd need to move towards a "continous profiling" model.


**Pros**
#### Pros

- We always get what we want when we want it
- Once we can ingest spans, it’ll be easy to switch.

**Cons**
#### Cons

- We need to change how we interact with transactions in the SDK and in the product.

### 2. One Transaction per Screen

Have one transaction per screen. This automatic transaction can be fully managed within the Sentry SDK. If no spans are generated an empty transaction would still be sent, as it contains slow/frozen frame metrics and ttid/ttfd data.

**Pros**
#### Pros

- There’s always a transaction running, which spans can be attached to
- There is no need for idle transactions anymore, as the lifetime matches a screen lifetime (a max deadline maybe still makes sense), making the overall behavior more predictable
- User interactions are modeled as spans instead of transactions, so they don’t interfere with transactions
- Slow and frozen frames could be added as child spans to the running transaction

**Cons**
#### Cons

- 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
Expand All @@ -65,3 +75,18 @@ Have one transaction per screen. This automatic transaction can be fully managed
### 3. Leave it as-is

Whilst being the least effort, this option doesn't add any value and we remain with all the drawbacks as outlined in the [background section](#background).

### 4. Single Span Ingestion <a name="option-4"></a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sidenote/question: why is it "single" span ingestion? I know we didn't invent this here, maybe you have a source

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's just the wording people always brought up. I don't have any source.


Keep screen load transactions, and use single-span ingestion ([PR](https://github.com/getsentry/relay/pull/2620)) whenever the SDK creates an auto-generated span and sends it stand-alone to Sentry. To avoid multiple network requests, SDKs need to batch spans together; for example, send an envelope for every ten spans. The batch logic is still up for definition and is not the goal for this RFC. Instead, we are going to define this in an extra RFC.
We want to keep the screen load transactions as Mobile Starfish already relies on them, and we want to be backward compatible.

#### Pros

- Spans without transactions will not appear in the v1 performance product but only in Starfish. Therefore, no work on the v1 performance product is required.
- No idle timeout or wait-for-children logic is required.
- This solution is future-proof.

#### Cons

- Profiling won't work out of the box because profiles are bound to transactions.