From ddd8afbba31d82ce01abbc74a509504c849bfe3c Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Wed, 22 Nov 2023 12:24:51 +0100 Subject: [PATCH 1/2] ref: Amend transaction and spans RFC 118 Amend decision of RFC 118 to use single span ingestion. --- README.md | 2 +- ... => 0118-mobile-transactions-and-spans.md} | 37 ++++++++++++++++--- 2 files changed, 32 insertions(+), 7 deletions(-) rename text/{0117-mobile-transactions-and-spans.md => 0118-mobile-transactions-and-spans.md} (75%) diff --git a/README.md b/README.md index 2a8b65b5..ddce0f50 100644 --- a/README.md +++ b/README.md @@ -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 diff --git a/text/0117-mobile-transactions-and-spans.md b/text/0118-mobile-transactions-and-spans.md similarity index 75% rename from text/0117-mobile-transactions-and-spans.md rename to text/0118-mobile-transactions-and-spans.md index 3dfd8445..b0227d19 100644 --- a/text/0117-mobile-transactions-and-spans.md +++ b/text/0118-mobile-transactions-and-spans.md @@ -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. @@ -35,13 +46,12 @@ 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. @@ -49,14 +59,14 @@ Future considerations: 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 @@ -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 + +Use single-span ingestion (PR) and keep screen load transactions. Whenever the SDK creates an auto-generated span, it 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. From 791d63cbb00cb148d4d395946e3e12cbe81af022 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Wed, 22 Nov 2023 16:18:44 +0100 Subject: [PATCH 2/2] Update text/0118-mobile-transactions-and-spans.md Co-authored-by: Karl Heinz Struggl --- text/0118-mobile-transactions-and-spans.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0118-mobile-transactions-and-spans.md b/text/0118-mobile-transactions-and-spans.md index b0227d19..2c56101d 100644 --- a/text/0118-mobile-transactions-and-spans.md +++ b/text/0118-mobile-transactions-and-spans.md @@ -78,7 +78,7 @@ Whilst being the least effort, this option doesn't add any value and we remain w ### 4. Single Span Ingestion -Use single-span ingestion (PR) and keep screen load transactions. Whenever the SDK creates an auto-generated span, it 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. +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