From d4ec7946d9aef3082db6ae1588c1aaf01d04b459 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 12 Jun 2023 08:14:55 -0400 Subject: [PATCH 01/13] rfc(decision): Revamping the SDK Performance API --- README.md | 4 +-- .../0101-revamping-the-sdk-performance-api.md | 35 +++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) create mode 100644 text/0101-revamping-the-sdk-performance-api.md diff --git a/README.md b/README.md index 49b5ac59..5821190a 100644 --- a/README.md +++ b/README.md @@ -49,7 +49,7 @@ This repository contains RFCs and DACIs. Lost? - [0087-graphql-errors](text/0087-graphql-errors.md): Request and Response bodies for GraphQL errors - [0088-fix-memory-limitiations-in-session-replays-access-pattern](text/0088-fix-memory-limitiations-in-session-replays-access-pattern.md): Fix Memory Limitiations in Session Replay's Access Pattern - [0091-ci-upload-tokens](text/0091-ci-upload-tokens.md): This RFC Proposes an improved CI experience for uploading source maps, debug symbols, - and potentially other CI based operations by proposing a new way to get and manage - access tokens specifically for this environment + and potentially other CI based operations by proposing a new way to get and manage + access tokens specifically for this environment - [0092-replay-issue-creation](text/0092-replay-issue-creation.md): Replay Issue Creation - [0095-escalating-forecasts-merged-issues](text/0095-escalating-forecasts-merged-issues.md): Issue States and Escalating Forecasts for Merged issues diff --git a/text/0101-revamping-the-sdk-performance-api.md b/text/0101-revamping-the-sdk-performance-api.md new file mode 100644 index 00000000..ece6fa2c --- /dev/null +++ b/text/0101-revamping-the-sdk-performance-api.md @@ -0,0 +1,35 @@ +- Start Date: 2023-06-12 +- RFC Type: decision +- RFC PR: https://github.com/getsentry/rfcs/pull/101 +- RFC Status: draft + +# Summary + +One paragraph explanation of the feature or document purpose. + +# Motivation + +Why are we doing this? What use cases does it support? What is the expected outcome? + +# Background + +The reason this decision or document is required. This section might not always exist. + +# Supporting Data + +[Metrics to help support your decision (if applicable).] + +# Options Considered + +If an RFC does not know yet what the options are, it can propose multiple options. The +preferred model is to propose one option and to provide alternatives. + +# Drawbacks + +Why should we not do this? What are the drawbacks of this RFC or a particular option if +multiple options are presented. + +# Unresolved questions + +- What parts of the design do you expect to resolve through this RFC? +- What issues are out of scope for this RFC but are known? From baec7995cdd9d5914b65a7535890d04434d08cd0 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 13 Jun 2023 12:10:20 -0400 Subject: [PATCH 02/13] first draft of rfc --- .../0101-revamping-the-sdk-performance-api.md | 295 +++++++++++++++++- 1 file changed, 279 insertions(+), 16 deletions(-) diff --git a/text/0101-revamping-the-sdk-performance-api.md b/text/0101-revamping-the-sdk-performance-api.md index ece6fa2c..0c3ae537 100644 --- a/text/0101-revamping-the-sdk-performance-api.md +++ b/text/0101-revamping-the-sdk-performance-api.md @@ -5,31 +5,294 @@ # Summary -One paragraph explanation of the feature or document purpose. +This RFC proposes a new RFC to revamp the performance API in the SDKs. The new API aims to accomplish the following. -# Motivation - -Why are we doing this? What use cases does it support? What is the expected outcome? +- Align both the internal schemas and top level public API with OpenTelemetry and their SDKs. +- De-emphasize the concept of transactions from users using a Sentry performance monitoring SDK. +- Optimize for making sure performance data is always sent to Sentry. +- Open the SDK up for future work where we support batch span ingestion instead of relying on a transaction solely. # Background -The reason this decision or document is required. This section might not always exist. +Right now every SDK has both the concept of transactions and spans - and to a user they both exist as vehicles of performance data. In addition, the transaction exists as the carrier of distributed tracing information ([dynamic sampling context](https://develop.sentry.dev/sdk/performance/dynamic-sampling-context/) and [sentry-trace info](https://develop.sentry.dev/sdk/performance/#header-sentry-trace)), although this is going to change with the advent of tracing without performance support in the SDKs. + +Below is JavaScript example of how to think about performance instrumentation in the JavaScript SDKs (browser/node) + +```jsx +// op is defined by https://develop.sentry.dev/sdk/performance/span-operations/ +// name has no specs but is expected to be low cardinality +const transaction = Sentry.startTransaction({ + op: "http.server", + name: "GET /", +}); + +// Need to set transaction on span so that integrations +// can attach spans (I/O operations or framework-specific spans) +// to the correct parent. +Sentry.getCurrentHub().getScope().setSpan(transaction); + +// spans have a description, while transactions have names +// op is an optional attribute, but a lot of the product relies on it +// existing. +// description technically should be low cardinality, but we have +// no explicit documentation to say that it should (since spans +// were not indexed at all for a while). +const span = transaction.startChild({ description: "Long Task" }); +expensiveAction(); +span.finish(); + +anotherAction(); + +const secondSpan = transaction.startChild({ description: "Another Task" }); + +// transaction + all child spans sent to Sentry only when `finish()` is called. +transaction.finish(); +Sentry.getCurrentHub().getScope().setSpan(undefined); + +// second span info is not sent to Sentry because transaction is already finished. +secondSpan.finish(); +``` + +In our integrations that add automatic instrumentation, things look something like so: + +```jsx +const parentSpan = Sentry.getCurrentHub().getSpan(); + +// parentSpan can be undefined if no span is on the scope, this leads to +// child span just being lost +const span = parentSpan?.startChild({ description: "something" }); + +Sentry.getCurrentHub().getScope().setSpan(span); + +work(); + +span.finish(); + +// span is finished, so parent is put back onto scope +Sentry.getCurrentHub().getScope().setSpan(parentSpan); +``` + +Most users do the above also when nested within their application, as often you assume that a transaction is defined that you can attach (very common in a web server context). + +To add instrumentation to their applications, users have to know concepts about hubs/scopes/transactions/spans and understand all the different nuances and use cases. This can be difficult, and presents a big barrier to entry for new users. + +Also, since transactions/spans are different classes (span is a parent class of transaction), users have to understand the impacts that the same method will have on both the transaction/span. For example, currently calling `setTag` on a transaction will add a tag to the transaction event (which is searchable in Sentry), while `setTag` on a span just adds it to the span, and the field is not searchable. `setData` on a span adds it to `span.data`, while `setData` on a transaction is undefined behaviour (some SDKs throw away the data, others add it to `event.extras`). + +Summarizing, here are the core Issues in SDK Performance API: + +1. Users have to understand the difference between spans/transactions and their schema differences. +2. Users have to set/get transactions/spans from a scope (meaning they also have to understand what a scope/hub means). +3. Nesting transactions within each other is undefined behaviour - no obvious way to make a transaction a child of another. +4. If a transaction finishes before it’s child span finishes, that child span gets orphaned and the data is never sent to Sentry. This is most apparent if you have transactions that automatically finish (like on browser/mobile). +5. Transactions have a max child span count of 1000 which means that eventually data is lost if you keep adding child spans to a transaction. + +# Improvements + +## Span Schema + +The current transaction schema inherits from the error event schema, with a few fields that are specific to transactions. + +```rs +// https://github.com/getsentry/relay/blob/2ad761f64db3df9b4d42f2c0896e1f6d99c16f49/relay-general/src/protocol/event.rs +pub struct Event { + /// Transaction name of the event. + /// In the SDK this is set and referenced as the `name` field. + pub transaction: Annotated, + + /// Timestamp when the event was created. + pub timestamp: Annotated, + + /// Timestamp when the event has started (relevant for event type = "transaction") + pub start_timestamp: Annotated, + + /// Custom tags for this event. + pub tags: Annotated, + + /// Spans for tracing. + pub spans: Annotated>, +} +``` + +The transaction also has a trace context, which contains additional fields about the transaction. + +```rs +// https://github.com/getsentry/relay/blob/2ad761f64db3df9b4d42f2c0896e1f6d99c16f49/relay-general/src/protocol/contexts/trace.rs +pub struct TraceContext { + /// The trace ID. + #[metastructure(required = "true")] + pub trace_id: Annotated, + + /// The ID of the span. + #[metastructure(required = "true")] + pub span_id: Annotated, + + /// The ID of the span enclosing this span. + pub parent_span_id: Annotated, + + /// Span type (see `OperationType` docs). + pub op: Annotated, + + /// Whether the trace failed or succeeded. Currently only used to indicate status of individual + /// transactions. + pub status: Annotated, +} +``` + +The current span schema is as follows: + +```rs +// https://github.com/getsentry/relay/blob/2ad761f64db3df9b4d42f2c0896e1f6d99c16f49/relay-general/src/protocol/span.rs +pub struct Span { + /// Timestamp when the span was ended. + #[metastructure(required = "true")] + pub timestamp: Annotated, + + /// Timestamp when the span started. + #[metastructure(required = "true")] + pub start_timestamp: Annotated, + + /// Human readable description of a span (e.g. method URL). + pub description: Annotated, + + /// Span type (see `OperationType` docs). + pub op: Annotated, + + /// The Span id. + #[metastructure(required = "true")] + pub span_id: Annotated, + + /// The ID of the span enclosing this span. + pub parent_span_id: Annotated, + + /// The ID of the trace the span belongs to. + #[metastructure(required = "true")] + pub trace_id: Annotated, + + /// The status of a span + pub status: Annotated, + + /// Arbitrary tags on a span, like on the top-level event. + pub tags: Annotated>, + + /// The origin of the span indicates what created the span (see [OriginType] docs). + pub origin: Annotated, + + /// Arbitrary additional data on a span, like `extra` on the top-level event. + pub data: Annotated>, +} +``` + +As you can see, the fields on the transaction/span differ in a few ways. This means that spans and transactions are not interchangeable, and users have to know the difference between the two. In addition, the wire format that is sent to Sentry is different for spans and transactions, which means that the SDKs have to do some work to convert between the two. + +### New Span Schema + +To simplify how performance data is consumed and understood, we are proposing a new span schema. The new span schema aims to be a superset of the [OpenTelemetry span schema](https://github.com/open-telemetry/opentelemetry-proto/blob/4967b51b5cb29f725978362b9d413bae1dbb641c/opentelemetry/proto/trace/v1/trace.proto) and have a minimal top level API surface. This also means that spans can be easily converted to OpenTelemetry spans and vice versa. + +The new span schema is as follows: + +```rs +pub struct Span { + /// Indicates to Sentry the version of the span schema. This will be set to 2 for + /// this version of the schema proposed by this RFC. + pub version: Annotated, + + /// A unique identifier for a trace. A 16 byte array. + /// Identical to the trace_id in the OpenTelemetry schema. + #[metastructure(required = "true")] + pub trace_id: Annotated, + + /// A unique identifier for a span within a trace, a 8 byte array. + /// Identical to the span_id in the OpenTelemetry schema. + #[metastructure(required = "true")] + pub span_id: Annotated, + + /// A unique identifier for the parent of this span within a trace, a 8 byte array. + /// If this is a root span this is empty. + /// Identical to the parent_span_id in the OpenTelemetry schema. + pub parent_span_id: Annotated, + + /// Span type (see `OperationType` docs). + /// No equivalent in the OpenTelemetry schema + /// but identical to the op field in the Sentry span/transaction schema. + pub op: Annotated, + + /// The name of the span. Should be low cardinality. + /// Maps to name in the OpenTelemetry schema. + #[metastructure(required = "true")] + pub name: Annotated, + + /// A set of attributes on the span. This maps to `span.data` in the current schema for spans. + /// There is no existing mapping for this in the current transaction schema. + /// + /// The keys of attributes are well known values, and defined by a combination of OpenTelemtry's and Sentry's + /// semantic conventions. + pub attributes: Annotated>, + + /// Timestamp when the span was ended. + /// Maps to end_time_unix_nano in the OpenTelemetry schema. + #[metastructure(required = "true")] + pub end_timestamp: Annotated, + + /// Timestamp when the span started. + /// Maps to start_time_unix_nano in the OpenTelemetry schema. + #[metastructure(required = "true")] + pub start_timestamp: Annotated, + + /// An optional final status for this span. Can have three possible values: 'ok', 'error', 'unset'. + /// Maps to status in the OpenTelemetry schema. + /// 'unset' is set by default. + pub status: Annotated, +} +``` + +TODO: Walk through new mapping process. + +## New SDK API + +The new SDK API should be as minimal as possible. The goal is to make it easy for users to instrument their code without having to know the difference between a span and a transaction. The new API should also be as similar to the OpenTelemetry SDK API as possible. Since we do not yet support single span ingestion, under the hood the new API should create spans/transactions as needed, but this should not be exposed to users unless they want to see it. + +Here are the requirements: + +1. Newly created spans must have the correct trace and parent/child relationship +2. Users shouldn’t be burdened with knowing if something is a span/transaction +3. Spans only need a name to identify themselves, everything else is optional. +4. The new top level APIs should be as similar to the OpenTelemetry SDK public API as possible. + +There are two top level methods we'll be introducing to achieve this: `Sentry.startActiveSpan` and `Sentry.startSpan`. `Sentry.startActiveSpan` will take a callback and start/stop a span automatically. In addition, it'll also set the span as the active span in the current scope. Under the hood, the SDK will create a transaction or span based on if there is already an existing span on the scope. `Sentry.startSpan` will create a span, but not set it as the active span in the current scope. + +```js +// span that is created is provided to callback in case additional +// attributes have to be added. +// ideally callback can be async/sync +Sentry.startActiveSpan({}, (_span) => expensiveCalc()); + +// If the SDK needs async/sync typed different we can expose this +// declare function Sentry.startActiveSpanAsync(spanCtx, asyncCallback); +``` + +```jsx +// does not get put on scope +const span = Sentry.startSpan({ name: "expensiveCalc" }); + +expensiveCalc(); -# Supporting Data +span.finish(); +``` -[Metrics to help support your decision (if applicable).] +The span instances returned from the `Sentry.startSpan` or exposed in the `Sentry.startActiveSpan` callback should match the schema outlined above. -# Options Considered +The only methods that all SDKs are required to implement are `Sentry.startActiveSpan` and `Sentry.startSpan`. For languages that need it, they add an additional method for async callbacks: `Sentry.startActiveSpanAsync`. Other languages can also attach a suffix to the methods to indicate that the spans are being started from different sources, but these are language/framework/sdk dependent. -If an RFC does not know yet what the options are, it can propose multiple options. The -preferred model is to propose one option and to provide alternatives. +For example with go: -# Drawbacks +```go +sentry.StartSpanFromContext(ctx, spanCtx) +``` -Why should we not do this? What are the drawbacks of this RFC or a particular option if -multiple options are presented. +Or when continuing from headers in javascript: -# Unresolved questions +```js +Sentry.startSpanFromHeaders(spanCtx, headers); +``` -- What parts of the design do you expect to resolve through this RFC? -- What issues are out of scope for this RFC but are known? +TODO: Discuss drawbacks of this approach. From d226b134a36d833a39822b3d75e8983ca7241b32 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 14 Jun 2023 08:58:57 -0400 Subject: [PATCH 03/13] add more details about span schema --- .../0101-revamping-the-sdk-performance-api.md | 35 ++++++++++++------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/text/0101-revamping-the-sdk-performance-api.md b/text/0101-revamping-the-sdk-performance-api.md index 0c3ae537..e0ab5c28 100644 --- a/text/0101-revamping-the-sdk-performance-api.md +++ b/text/0101-revamping-the-sdk-performance-api.md @@ -90,19 +90,22 @@ Summarizing, here are the core Issues in SDK Performance API: ## Span Schema +To remove the overhead of understanding transactions/spans and their differences, we propose to simplify the span schema to have a minimal set of required fields. + The current transaction schema inherits from the error event schema, with a few fields that are specific to transactions. ```rs -// https://github.com/getsentry/relay/blob/2ad761f64db3df9b4d42f2c0896e1f6d99c16f49/relay-general/src/protocol/event.rs +// Based on https://github.com/getsentry/relay/blob/2ad761f64db3df9b4d42f2c0896e1f6d99c16f49/relay-general/src/protocol/event.rs pub struct Event { /// Transaction name of the event. - /// In the SDK this is set and referenced as the `name` field. + /// This field is called `name` by most SDKs and is accessed by `transaction.setName()` and `transaction.getName()` pub transaction: Annotated, - /// Timestamp when the event was created. + /// Timestamp when the event was created. Some SDKs alias this to `end_timestamp` but convert + /// it to `timestamp` when serializing to send to Sentry. pub timestamp: Annotated, - /// Timestamp when the event has started (relevant for event type = "transaction") + /// Timestamp when the event has started. pub start_timestamp: Annotated, /// Custom tags for this event. @@ -174,26 +177,26 @@ pub struct Span { /// Arbitrary tags on a span, like on the top-level event. pub tags: Annotated>, - /// The origin of the span indicates what created the span (see [OriginType] docs). - pub origin: Annotated, - /// Arbitrary additional data on a span, like `extra` on the top-level event. pub data: Annotated>, } ``` -As you can see, the fields on the transaction/span differ in a few ways. This means that spans and transactions are not interchangeable, and users have to know the difference between the two. In addition, the wire format that is sent to Sentry is different for spans and transactions, which means that the SDKs have to do some work to convert between the two. +As you can see, the fields on the transaction/span differ in a few ways, the most noteable of which is that transactions have `name` while spans have `description`. This means that spans and transactions are not interchangeable, and users have to know the difference between the two. + +In addition, user's have the burden to understand the differences between `name`/`description`/`operation`. `operation` in particular can be confusing, as it overlaps with transaction `name` and span `description`. In addition, `operation` is not a required field, which means that it is not clear what the default value should be. + +Transactions also have no mechanism for arbitrary additional data like spans do with `data`. Users can choose to add arbitrary data to transactions by adding it to the `contexts` field (as transactions extend the event schema), but this is not obvious and not exposed in every SDK. Since contexts are already well defined in their own way, there is no way of using [Sentry's semantic conventions for span data](https://develop.sentry.dev/sdk/performance/span-data-conventions/) for transactions. ### New Span Schema -To simplify how performance data is consumed and understood, we are proposing a new span schema. The new span schema aims to be a superset of the [OpenTelemetry span schema](https://github.com/open-telemetry/opentelemetry-proto/blob/4967b51b5cb29f725978362b9d413bae1dbb641c/opentelemetry/proto/trace/v1/trace.proto) and have a minimal top level API surface. This also means that spans can be easily converted to OpenTelemetry spans and vice versa. +To simplify how performance data is consumed and understood, we are proposing a new span schema that the SDKs send to Sentry. The new span schema aims to be a superset of the [OpenTelemetry span schema](https://github.com/open-telemetry/opentelemetry-proto/blob/4967b51b5cb29f725978362b9d413bae1dbb641c/opentelemetry/proto/trace/v1/trace.proto) and have a minimal top level API surface. This also means that spans can be easily converted to OpenTelemetry spans and vice versa. The new span schema is as follows: ```rs pub struct Span { - /// Indicates to Sentry the version of the span schema. This will be set to 2 for - /// this version of the schema proposed by this RFC. + /// Indicates to Sentry the version of the span schema. pub version: Annotated, /// A unique identifier for a trace. A 16 byte array. @@ -245,11 +248,17 @@ pub struct Span { } ``` -TODO: Walk through new mapping process. +For the purposes of this RFC, the version on the span schema will be set to 2. This will indicate to all consumers that the new span schema is being used. + +Just like both the old Sentry schema and the OpenTelemetry schema, we keep the same fields for `span_id`, `trace_id`, `parent_span_id`, `start_timestamp`, and `end_timestamp`. We also choose to rename `description` to `name` to match the OpenTelemetry schema. + +Having both the `name` and `op` fields is redundant, but we choose to keep both for backwards compatibility. There are many features in the product that are powered by having a meaningful operation, more details about this can be found in the documentation around [Span operations](https://develop.sentry.dev/sdk/performance/span-operations/). In the future, we can choose to deprecate `op` and remove it from the schema. + +The most notable change here is to formally introduce the `attributes` field, and remove the `span.data` field. This is a breaking change, but worth it in the long-term. If we start accepting `attributes` on transactions as well, we more closely align with the OpenTelemetry schema, and can use the same conventions for both spans and transactions. ## New SDK API -The new SDK API should be as minimal as possible. The goal is to make it easy for users to instrument their code without having to know the difference between a span and a transaction. The new API should also be as similar to the OpenTelemetry SDK API as possible. Since we do not yet support single span ingestion, under the hood the new API should create spans/transactions as needed, but this should not be exposed to users unless they want to see it. +The new SDK API should be as minimal as possible. The goal is to make it easy for users to instrument their code without having to know the difference between a span and a transaction. The new API should also be as similar to the OpenTelemetry SDK API as possible. Since we do not yet support single span ingestion, under the hood the new API should create spans/transactions as needed, but this should not be exposed to users unless they need to for their use case. Here are the requirements: From 5bc801e5cecbf1e16623a79227c2479d4cb13feb Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 14 Jun 2023 14:19:06 -0400 Subject: [PATCH 04/13] add brief plan for RFC --- .../0101-revamping-the-sdk-performance-api.md | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/text/0101-revamping-the-sdk-performance-api.md b/text/0101-revamping-the-sdk-performance-api.md index e0ab5c28..b253ef47 100644 --- a/text/0101-revamping-the-sdk-performance-api.md +++ b/text/0101-revamping-the-sdk-performance-api.md @@ -12,6 +12,18 @@ This RFC proposes a new RFC to revamp the performance API in the SDKs. The new A - Optimize for making sure performance data is always sent to Sentry. - Open the SDK up for future work where we support batch span ingestion instead of relying on a transaction solely. +# Planned Work + +The RFC proposal comes in two steps. + +1. Introduce three new top level methods, `Sentry.startActiveSpan` and `Sentry.startSpan` and their language specific variants as well as `Sentry.setMeasurement`. + +This allows us to de-emphasize the concept of transactions from users, and instead have them think about spans. This also removes the overhead of thinking about hubs/scopes and instead introduces a new concept of an active span. Under the hood, `Sentry.startActiveSpan` and `Sentry.startSpan` should create transactions/spans as appropriate. `Sentry.setMeasurement` is used to abstract away `transaction.setMeasurement` and similar. + +2. Introduce a new span schema that is aligned with OpenTelemetry. + +We change the data model that is referenced and used internally inside the SDK to better reflect OpenTelemetry. This involves adding shims for backwards compatibility, and removing redundant fields. This must be done in a backwards compatible way. + # Background Right now every SDK has both the concept of transactions and spans - and to a user they both exist as vehicles of performance data. In addition, the transaction exists as the carrier of distributed tracing information ([dynamic sampling context](https://develop.sentry.dev/sdk/performance/dynamic-sampling-context/) and [sentry-trace info](https://develop.sentry.dev/sdk/performance/#header-sentry-trace)), although this is going to change with the advent of tracing without performance support in the SDKs. @@ -92,6 +104,8 @@ Summarizing, here are the core Issues in SDK Performance API: To remove the overhead of understanding transactions/spans and their differences, we propose to simplify the span schema to have a minimal set of required fields. +### Existing Span Schema + The current transaction schema inherits from the error event schema, with a few fields that are specific to transactions. ```rs @@ -113,6 +127,9 @@ pub struct Event { /// Spans for tracing. pub spans: Annotated>, + + /// Measurements which holds observed values such as web vitals. + pub measurements: Annotated, } ``` @@ -231,6 +248,9 @@ pub struct Span { /// semantic conventions. pub attributes: Annotated>, + /// Measurements which holds observed values such as web vitals. + pub measurements: Annotated, + /// Timestamp when the span was ended. /// Maps to end_time_unix_nano in the OpenTelemetry schema. #[metastructure(required = "true")] @@ -269,6 +289,8 @@ Here are the requirements: There are two top level methods we'll be introducing to achieve this: `Sentry.startActiveSpan` and `Sentry.startSpan`. `Sentry.startActiveSpan` will take a callback and start/stop a span automatically. In addition, it'll also set the span as the active span in the current scope. Under the hood, the SDK will create a transaction or span based on if there is already an existing span on the scope. `Sentry.startSpan` will create a span, but not set it as the active span in the current scope. +There are two things to take away from the implementation of these two new APIs. First we should only be referencing spans and returning span references. This is to make it easier to switch between spans and transactions in the future. Second, we should be denotating the difference between an active span and a span. This is to indicate to users that there is a behaviour difference between the two. + ```js // span that is created is provided to callback in case additional // attributes have to be added. @@ -303,5 +325,3 @@ Or when continuing from headers in javascript: ```js Sentry.startSpanFromHeaders(spanCtx, headers); ``` - -TODO: Discuss drawbacks of this approach. From 67d248c30281cf61c8e5fcc679e9015a70556aeb Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 14 Jun 2023 14:19:47 -0400 Subject: [PATCH 05/13] re-arrange order --- .../0101-revamping-the-sdk-performance-api.md | 100 +++++++++--------- 1 file changed, 52 insertions(+), 48 deletions(-) diff --git a/text/0101-revamping-the-sdk-performance-api.md b/text/0101-revamping-the-sdk-performance-api.md index b253ef47..82f9ce98 100644 --- a/text/0101-revamping-the-sdk-performance-api.md +++ b/text/0101-revamping-the-sdk-performance-api.md @@ -100,6 +100,56 @@ Summarizing, here are the core Issues in SDK Performance API: # Improvements +## New SDK API + +The new SDK API has the following requirements: + +1. Newly created spans must have the correct trace and parent/child relationship +2. Users shouldn’t be burdened with knowing if something is a span/transaction +3. Spans only need a name to identify themselves, everything else is optional. +4. The new top level APIs should be as similar to the OpenTelemetry SDK public API as possible. + +There are two top level methods we'll be introducing to achieve this: `Sentry.startActiveSpan` and `Sentry.startSpan`. `Sentry.startActiveSpan` will take a callback and start/stop a span automatically. In addition, it'll also set the span as the active span in the current scope. Under the hood, the SDK will create a transaction or span based on if there is already an existing span on the scope. `Sentry.startSpan` will create a span, but not set it as the active span in the current scope. + +There are two things to take away from the implementation of these two new APIs. First we should only be referencing spans and returning span references. This is to make it easier to switch between spans and transactions in the future. Second, we should be denotating the difference between an active span and a span. This is to indicate to users that there is a behaviour difference between the two. + +```js +// span that is created is provided to callback in case additional +// attributes have to be added. +// ideally callback can be async/sync +Sentry.startActiveSpan({}, (_span) => expensiveCalc()); + +// If the SDK needs async/sync typed different we can expose this +// declare function Sentry.startActiveSpanAsync(spanCtx, asyncCallback); +``` + +```jsx +// does not get put on scope +const span = Sentry.startSpan({ name: "expensiveCalc" }); + +expensiveCalc(); + +span.finish(); +``` + +The span instances returned from the `Sentry.startSpan` or exposed in the `Sentry.startActiveSpan` callback should match the schema outlined above. + +The only methods that all SDKs are required to implement are `Sentry.startActiveSpan` and `Sentry.startSpan`. For languages that need it, they add an additional method for async callbacks: `Sentry.startActiveSpanAsync`. Other languages can also attach a suffix to the methods to indicate that the spans are being started from different sources, but these are language/framework/sdk dependent. + +For example with go: + +```go +sentry.StartSpanFromContext(ctx, spanCtx) +``` + +Or when continuing from headers in javascript: + +```js +Sentry.startSpanFromHeaders(spanCtx, headers); +``` + +Since we want to discourage accessing the transaction object directly, the `Sentry.setMeasurement` top level method will also be introduced. This will set a custom performance metric if a transaction exists. + ## Span Schema To remove the overhead of understanding transactions/spans and their differences, we propose to simplify the span schema to have a minimal set of required fields. @@ -276,52 +326,6 @@ Having both the `name` and `op` fields is redundant, but we choose to keep both The most notable change here is to formally introduce the `attributes` field, and remove the `span.data` field. This is a breaking change, but worth it in the long-term. If we start accepting `attributes` on transactions as well, we more closely align with the OpenTelemetry schema, and can use the same conventions for both spans and transactions. -## New SDK API - -The new SDK API should be as minimal as possible. The goal is to make it easy for users to instrument their code without having to know the difference between a span and a transaction. The new API should also be as similar to the OpenTelemetry SDK API as possible. Since we do not yet support single span ingestion, under the hood the new API should create spans/transactions as needed, but this should not be exposed to users unless they need to for their use case. - -Here are the requirements: - -1. Newly created spans must have the correct trace and parent/child relationship -2. Users shouldn’t be burdened with knowing if something is a span/transaction -3. Spans only need a name to identify themselves, everything else is optional. -4. The new top level APIs should be as similar to the OpenTelemetry SDK public API as possible. - -There are two top level methods we'll be introducing to achieve this: `Sentry.startActiveSpan` and `Sentry.startSpan`. `Sentry.startActiveSpan` will take a callback and start/stop a span automatically. In addition, it'll also set the span as the active span in the current scope. Under the hood, the SDK will create a transaction or span based on if there is already an existing span on the scope. `Sentry.startSpan` will create a span, but not set it as the active span in the current scope. - -There are two things to take away from the implementation of these two new APIs. First we should only be referencing spans and returning span references. This is to make it easier to switch between spans and transactions in the future. Second, we should be denotating the difference between an active span and a span. This is to indicate to users that there is a behaviour difference between the two. - -```js -// span that is created is provided to callback in case additional -// attributes have to be added. -// ideally callback can be async/sync -Sentry.startActiveSpan({}, (_span) => expensiveCalc()); - -// If the SDK needs async/sync typed different we can expose this -// declare function Sentry.startActiveSpanAsync(spanCtx, asyncCallback); -``` +## Next Steps -```jsx -// does not get put on scope -const span = Sentry.startSpan({ name: "expensiveCalc" }); - -expensiveCalc(); - -span.finish(); -``` - -The span instances returned from the `Sentry.startSpan` or exposed in the `Sentry.startActiveSpan` callback should match the schema outlined above. - -The only methods that all SDKs are required to implement are `Sentry.startActiveSpan` and `Sentry.startSpan`. For languages that need it, they add an additional method for async callbacks: `Sentry.startActiveSpanAsync`. Other languages can also attach a suffix to the methods to indicate that the spans are being started from different sources, but these are language/framework/sdk dependent. - -For example with go: - -```go -sentry.StartSpanFromContext(ctx, spanCtx) -``` - -Or when continuing from headers in javascript: - -```js -Sentry.startSpanFromHeaders(spanCtx, headers); -``` +Since we only have `beforeSend` hooks for a transaction, we should look toward building similar hooks for span start and finish as well. This can be done after the span schema has been changed in the SDKs. From 175fb6e4d37c85632065ab354c1812a81bc43de8 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 14 Jun 2023 16:32:20 -0400 Subject: [PATCH 06/13] be more specific about startActiveSpan impl --- .../0101-revamping-the-sdk-performance-api.md | 48 +++++++++++++++---- 1 file changed, 38 insertions(+), 10 deletions(-) diff --git a/text/0101-revamping-the-sdk-performance-api.md b/text/0101-revamping-the-sdk-performance-api.md index 82f9ce98..d4ca168f 100644 --- a/text/0101-revamping-the-sdk-performance-api.md +++ b/text/0101-revamping-the-sdk-performance-api.md @@ -18,11 +18,11 @@ The RFC proposal comes in two steps. 1. Introduce three new top level methods, `Sentry.startActiveSpan` and `Sentry.startSpan` and their language specific variants as well as `Sentry.setMeasurement`. -This allows us to de-emphasize the concept of transactions from users, and instead have them think about spans. This also removes the overhead of thinking about hubs/scopes and instead introduces a new concept of an active span. Under the hood, `Sentry.startActiveSpan` and `Sentry.startSpan` should create transactions/spans as appropriate. `Sentry.setMeasurement` is used to abstract away `transaction.setMeasurement` and similar. +This allows us to de-emphasize the concept of hubs, scopes and transactions from users, and instead have them think about just spans. Under the hood, `Sentry.startActiveSpan` and `Sentry.startSpan` should create transactions/spans as appropriate. `Sentry.setMeasurement` is used to abstract away `transaction.setMeasurement` and similar. -2. Introduce a new span schema that is aligned with OpenTelemetry. +1. Introduce a new span schema that is aligned with OpenTelemetry. -We change the data model that is referenced and used internally inside the SDK to better reflect OpenTelemetry. This involves adding shims for backwards compatibility, and removing redundant fields. This must be done in a backwards compatible way. +We change the data model that is referenced and used internally inside the SDK to better reflect OpenTelemetry. This involves adding shims for backwards compatibility and removing redundant fields. # Background @@ -109,21 +109,45 @@ The new SDK API has the following requirements: 3. Spans only need a name to identify themselves, everything else is optional. 4. The new top level APIs should be as similar to the OpenTelemetry SDK public API as possible. -There are two top level methods we'll be introducing to achieve this: `Sentry.startActiveSpan` and `Sentry.startSpan`. `Sentry.startActiveSpan` will take a callback and start/stop a span automatically. In addition, it'll also set the span as the active span in the current scope. Under the hood, the SDK will create a transaction or span based on if there is already an existing span on the scope. `Sentry.startSpan` will create a span, but not set it as the active span in the current scope. +There are two top level methods we'll be introducing to achieve this: `Sentry.startActiveSpan` and `Sentry.startSpan`. `Sentry.startActiveSpan` will take a callback and start/stop a span automatically. In addition, it'll also set the span on the current scope. Under the hood, the SDK will create a transaction or span based on if there is already an existing span on the scope. -There are two things to take away from the implementation of these two new APIs. First we should only be referencing spans and returning span references. This is to make it easier to switch between spans and transactions in the future. Second, we should be denotating the difference between an active span and a span. This is to indicate to users that there is a behaviour difference between the two. +```ts +namespace Sentry { + declare function startActiveSpan( + spanContext: SpanContext, + callback: (span: Span) => void + ): void; +} -```js // span that is created is provided to callback in case additional // attributes have to be added. // ideally callback can be async/sync Sentry.startActiveSpan({}, (_span) => expensiveCalc()); // If the SDK needs async/sync typed different we can expose this -// declare function Sentry.startActiveSpanAsync(spanCtx, asyncCallback); +// declare function startActiveSpanAsync( +// spanContext: SpanContext, +// callback: (span: Span) => Promise, +// ): void; ``` -```jsx +In the ideal case, `startActiveSpan` should generally follow this code path. + +1. Get the active span from the current scope +2. If the active span is defined, create a child span of that active span based on the provided `spanContext`, otherwise create a transaction based on the provided `spanContext`. +3. Run the provided callback +4. Finish the child span/transaction created in step 2 +5. Remove the child span/transaction from the current scope and if it exists, set the previous active span as the active span in the current scope. + +If the provided callback throws an exception, the span/transaction created in step 2 should be marked as errored. This error should not be swallowed by `startActiveSpan`. + +`Sentry.startSpan` will create a span, but not set it as the active span in the current scope. 90% of our documentation and code examples will be referencing `Sentry.startActiveSpan` - `Sentry.startSpan` is just there for edge case users of the API. + +```ts +namespace Sentry { + declare function startSpan(spanContext: SpanContext): Span; +} + // does not get put on scope const span = Sentry.startSpan({ name: "expensiveCalc" }); @@ -132,8 +156,6 @@ expensiveCalc(); span.finish(); ``` -The span instances returned from the `Sentry.startSpan` or exposed in the `Sentry.startActiveSpan` callback should match the schema outlined above. - The only methods that all SDKs are required to implement are `Sentry.startActiveSpan` and `Sentry.startSpan`. For languages that need it, they add an additional method for async callbacks: `Sentry.startActiveSpanAsync`. Other languages can also attach a suffix to the methods to indicate that the spans are being started from different sources, but these are language/framework/sdk dependent. For example with go: @@ -150,6 +172,12 @@ Sentry.startSpanFromHeaders(spanCtx, headers); Since we want to discourage accessing the transaction object directly, the `Sentry.setMeasurement` top level method will also be introduced. This will set a custom performance metric if a transaction exists. +```ts +namespace Sentry { + declare function setMeasurement(key: string, value: number): void; +} +``` + ## Span Schema To remove the overhead of understanding transactions/spans and their differences, we propose to simplify the span schema to have a minimal set of required fields. From 8130f2655cec0257f5ef49a477e6c367b252ab50 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 14 Jun 2023 16:50:50 -0400 Subject: [PATCH 07/13] remove code snippets in favour of tables --- .../0101-revamping-the-sdk-performance-api.md | 182 ++++-------------- 1 file changed, 42 insertions(+), 140 deletions(-) diff --git a/text/0101-revamping-the-sdk-performance-api.md b/text/0101-revamping-the-sdk-performance-api.md index d4ca168f..5480e59d 100644 --- a/text/0101-revamping-the-sdk-performance-api.md +++ b/text/0101-revamping-the-sdk-performance-api.md @@ -186,96 +186,42 @@ To remove the overhead of understanding transactions/spans and their differences The current transaction schema inherits from the error event schema, with a few fields that are specific to transactions. -```rs -// Based on https://github.com/getsentry/relay/blob/2ad761f64db3df9b4d42f2c0896e1f6d99c16f49/relay-general/src/protocol/event.rs -pub struct Event { - /// Transaction name of the event. - /// This field is called `name` by most SDKs and is accessed by `transaction.setName()` and `transaction.getName()` - pub transaction: Annotated, - - /// Timestamp when the event was created. Some SDKs alias this to `end_timestamp` but convert - /// it to `timestamp` when serializing to send to Sentry. - pub timestamp: Annotated, - - /// Timestamp when the event has started. - pub start_timestamp: Annotated, - - /// Custom tags for this event. - pub tags: Annotated, - - /// Spans for tracing. - pub spans: Annotated>, - - /// Measurements which holds observed values such as web vitals. - pub measurements: Annotated, -} -``` +A full version of this protocol can be seen in [Relay](https://github.com/getsentry/relay/blob/2ad761f64db3df9b4d42f2c0896e1f6d99c16f49/relay-general/src/protocol/event.rs), but here are some of the fields that are important: + +| Transaction Field | Description | Type | +| ----------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------- | +| `name` | Name of the transaction. In ingest and storage this field is called `transaction` | String | +| `end_timestamp` | Timestamp when transaction was finished. Some SDKs alias this to `end_timestamp` but convert it to `timestamp` when serializing to send to Sentry. | String \| Float | +| `start_timestamp` | Timestamp when transaction was created. | String \| Float | +| `tags` | Custom tags for this event. Identical in behaviour to tags on error events. | Object | +| `spans` | A list of child spans to this transaction. | Span[] | +| `measurements` | Measurements which holds observed values such as web vitals. | Object | +| `contexts` | Contexts which holds additional information about the transaction. In particular, `contexts.trace` has additional information about the transaction "span" | Object | The transaction also has a trace context, which contains additional fields about the transaction. -```rs -// https://github.com/getsentry/relay/blob/2ad761f64db3df9b4d42f2c0896e1f6d99c16f49/relay-general/src/protocol/contexts/trace.rs -pub struct TraceContext { - /// The trace ID. - #[metastructure(required = "true")] - pub trace_id: Annotated, - - /// The ID of the span. - #[metastructure(required = "true")] - pub span_id: Annotated, - - /// The ID of the span enclosing this span. - pub parent_span_id: Annotated, - - /// Span type (see `OperationType` docs). - pub op: Annotated, - - /// Whether the trace failed or succeeded. Currently only used to indicate status of individual - /// transactions. - pub status: Annotated, -} -``` +| Transaction Field | Description | Type | +| ----------------- | ----------------------------------------------------------------------------------------------------------------------------- | ------ | +| `trace_id` | Trace ID of the transaction. Format is identical between Sentry and OpenTelemetry | String | +| `span_id` | Span ID of the transaction. Format is identical between Sentry and OpenTelemetry | String | +| `parent_span_id` | Parent span ID of the transaction. Format is identical between Sentry and OpenTelemetry | String | +| `op` | Operation type of the transaction. [Standardized by Sentry spec](https://develop.sentry.dev/sdk/performance/span-operations/) | String | +| `status` | Status of the transaction. Sentry maps status to HTTP status codes, while OpenTelemetry has a fixed set of status' | String | The current span schema is as follows: -```rs -// https://github.com/getsentry/relay/blob/2ad761f64db3df9b4d42f2c0896e1f6d99c16f49/relay-general/src/protocol/span.rs -pub struct Span { - /// Timestamp when the span was ended. - #[metastructure(required = "true")] - pub timestamp: Annotated, - - /// Timestamp when the span started. - #[metastructure(required = "true")] - pub start_timestamp: Annotated, - - /// Human readable description of a span (e.g. method URL). - pub description: Annotated, - - /// Span type (see `OperationType` docs). - pub op: Annotated, - - /// The Span id. - #[metastructure(required = "true")] - pub span_id: Annotated, - - /// The ID of the span enclosing this span. - pub parent_span_id: Annotated, - - /// The ID of the trace the span belongs to. - #[metastructure(required = "true")] - pub trace_id: Annotated, - - /// The status of a span - pub status: Annotated, - - /// Arbitrary tags on a span, like on the top-level event. - pub tags: Annotated>, - - /// Arbitrary additional data on a span, like `extra` on the top-level event. - pub data: Annotated>, -} -``` +| Span Field | Description | Type | +| ----------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------- | +| `description` | Description of the span. Same purpose as transaction `name` | +| `trace_id` | Trace ID of the span. Format is identical between Sentry and OpenTelemetry | String | +| `span_id` | Span ID of the span. Format is identical between Sentry and OpenTelemetry | String | +| `parent_span_id` | Parent span ID of the span. Format is identical between Sentry and OpenTelemetry | String | +| `end_timestamp` | Timestamp when span was finished. Some SDKs alias this to `end_timestamp` but convert it to `timestamp` when serializing to send to Sentry. | String \| Float | +| `start_timestamp` | Timestamp when span was created. | String \| Float | +| `op` | Operation type of the span. [Standardized by Sentry spec](https://develop.sentry.dev/sdk/performance/span-operations/) | String | +| `status` | Status of the span. Sentry maps status to HTTP status codes, while OpenTelemetry has a fixed set of status' | String | +| `tags` | Custom tags for this span. | Object | +| `data` | Arbitrary additional data on a span, like `extra` on the top-level event. We maintain [conventions for span data keys and values](https://develop.sentry.dev/sdk/performance/span-data-conventions/). | Object | As you can see, the fields on the transaction/span differ in a few ways, the most noteable of which is that transactions have `name` while spans have `description`. This means that spans and transactions are not interchangeable, and users have to know the difference between the two. @@ -289,62 +235,18 @@ To simplify how performance data is consumed and understood, we are proposing a The new span schema is as follows: -```rs -pub struct Span { - /// Indicates to Sentry the version of the span schema. - pub version: Annotated, - - /// A unique identifier for a trace. A 16 byte array. - /// Identical to the trace_id in the OpenTelemetry schema. - #[metastructure(required = "true")] - pub trace_id: Annotated, - - /// A unique identifier for a span within a trace, a 8 byte array. - /// Identical to the span_id in the OpenTelemetry schema. - #[metastructure(required = "true")] - pub span_id: Annotated, - - /// A unique identifier for the parent of this span within a trace, a 8 byte array. - /// If this is a root span this is empty. - /// Identical to the parent_span_id in the OpenTelemetry schema. - pub parent_span_id: Annotated, - - /// Span type (see `OperationType` docs). - /// No equivalent in the OpenTelemetry schema - /// but identical to the op field in the Sentry span/transaction schema. - pub op: Annotated, - - /// The name of the span. Should be low cardinality. - /// Maps to name in the OpenTelemetry schema. - #[metastructure(required = "true")] - pub name: Annotated, - - /// A set of attributes on the span. This maps to `span.data` in the current schema for spans. - /// There is no existing mapping for this in the current transaction schema. - /// - /// The keys of attributes are well known values, and defined by a combination of OpenTelemtry's and Sentry's - /// semantic conventions. - pub attributes: Annotated>, - - /// Measurements which holds observed values such as web vitals. - pub measurements: Annotated, - - /// Timestamp when the span was ended. - /// Maps to end_time_unix_nano in the OpenTelemetry schema. - #[metastructure(required = "true")] - pub end_timestamp: Annotated, - - /// Timestamp when the span started. - /// Maps to start_time_unix_nano in the OpenTelemetry schema. - #[metastructure(required = "true")] - pub start_timestamp: Annotated, - - /// An optional final status for this span. Can have three possible values: 'ok', 'error', 'unset'. - /// Maps to status in the OpenTelemetry schema. - /// 'unset' is set by default. - pub status: Annotated, -} -``` +| Span Field | Description | Type | Notes | +| ----------------- | ------------------------------------------------------------ | --------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `name` | The name of the span | String | Should be low cardinality. Replacing span `description` | +| `trace_id` | Trace ID of the span | String | Format is identical between Sentry and OpenTelemetry | +| `span_id` | Span ID of the span | String | Format is identical between Sentry and OpenTelemetry | +| `parent_span_id` | Parent span ID of the span | String | Format is identical between Sentry and OpenTelemetry. If empty this is a root span (transaction). | +| `end_timestamp` | Timestamp when span was finished | String \| Float | | +| `start_timestamp` | Timestamp when span was finished | String \| Float | | +| `op` | Operation type of the span | String | Use is discouraged but kept for backwards compatibility for product features | +| `status` | Status of the span | String | An optional final status for this span. Can have three possible values: 'ok', 'error', 'unset'. Same as OpenTelemetry's Span Status | +| `attributes` | A set of attributes on the span. | Object | This maps to `span.data` in the current schema for spans. There is no existing mapping for this in the current transaction schema. The keys of attributes are well known values, and defined by a combination of OpenTelemtry's and Sentry's semantic conventions. | +| `measurements` | Measurements which holds observed values such as web vitals. | Object | | For the purposes of this RFC, the version on the span schema will be set to 2. This will indicate to all consumers that the new span schema is being used. From bf6ebddc9aa3c5b19674f6645cea942ee700f0bf Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 14 Jun 2023 16:56:58 -0400 Subject: [PATCH 08/13] add section around shimming the old schema --- text/0101-revamping-the-sdk-performance-api.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/text/0101-revamping-the-sdk-performance-api.md b/text/0101-revamping-the-sdk-performance-api.md index 5480e59d..c7e947e2 100644 --- a/text/0101-revamping-the-sdk-performance-api.md +++ b/text/0101-revamping-the-sdk-performance-api.md @@ -256,6 +256,10 @@ Having both the `name` and `op` fields is redundant, but we choose to keep both The most notable change here is to formally introduce the `attributes` field, and remove the `span.data` field. This is a breaking change, but worth it in the long-term. If we start accepting `attributes` on transactions as well, we more closely align with the OpenTelemetry schema, and can use the same conventions for both spans and transactions. +### Shimming the old schema + +TODO + ## Next Steps Since we only have `beforeSend` hooks for a transaction, we should look toward building similar hooks for span start and finish as well. This can be done after the span schema has been changed in the SDKs. From 5d496a719ece7034a7cea59f3307ed8092d1947a Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 14 Jun 2023 16:58:09 -0400 Subject: [PATCH 09/13] fix grammer --- text/0101-revamping-the-sdk-performance-api.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0101-revamping-the-sdk-performance-api.md b/text/0101-revamping-the-sdk-performance-api.md index c7e947e2..b1cc0383 100644 --- a/text/0101-revamping-the-sdk-performance-api.md +++ b/text/0101-revamping-the-sdk-performance-api.md @@ -5,7 +5,7 @@ # Summary -This RFC proposes a new RFC to revamp the performance API in the SDKs. The new API aims to accomplish the following. +This RFC proposes to revamp the performance API in the SDKs. The new API aims to accomplish the following: - Align both the internal schemas and top level public API with OpenTelemetry and their SDKs. - De-emphasize the concept of transactions from users using a Sentry performance monitoring SDK. From 6fa96d25bddc94a2d8da48c121e5bfed723a8def Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 14 Jun 2023 17:02:35 -0400 Subject: [PATCH 10/13] fix type def --- text/0101-revamping-the-sdk-performance-api.md | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/text/0101-revamping-the-sdk-performance-api.md b/text/0101-revamping-the-sdk-performance-api.md index b1cc0383..99b0fa5f 100644 --- a/text/0101-revamping-the-sdk-performance-api.md +++ b/text/0101-revamping-the-sdk-performance-api.md @@ -113,22 +113,25 @@ There are two top level methods we'll be introducing to achieve this: `Sentry.st ```ts namespace Sentry { - declare function startActiveSpan( + declare function startActiveSpan( spanContext: SpanContext, - callback: (span: Span) => void - ): void; + callback: (span: Span) => T + ): T; } // span that is created is provided to callback in case additional // attributes have to be added. // ideally callback can be async/sync -Sentry.startActiveSpan({}, (_span) => expensiveCalc()); +const returnVal = Sentry.startActiveSpan( + { name: "expensiveCalc" }, + (span: Span) => expensiveCalc() +); // If the SDK needs async/sync typed different we can expose this -// declare function startActiveSpanAsync( +// declare function startActiveSpanAsync( // spanContext: SpanContext, -// callback: (span: Span) => Promise, -// ): void; +// callback: (span: Span) => Promise, +// ): Promise; ``` In the ideal case, `startActiveSpan` should generally follow this code path. From 4031acf9bfe8bf045ab7afd4e4ffa90b482c1a77 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 15 Jun 2023 17:15:02 -0400 Subject: [PATCH 11/13] add snippet around migrating span schema --- text/0101-revamping-the-sdk-performance-api.md | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/text/0101-revamping-the-sdk-performance-api.md b/text/0101-revamping-the-sdk-performance-api.md index 99b0fa5f..593973fb 100644 --- a/text/0101-revamping-the-sdk-performance-api.md +++ b/text/0101-revamping-the-sdk-performance-api.md @@ -2,6 +2,7 @@ - RFC Type: decision - RFC PR: https://github.com/getsentry/rfcs/pull/101 - RFC Status: draft +- RFC Driver: [Abhijeet Prasad](https://github.com/abhiprasad) # Summary @@ -144,7 +145,7 @@ In the ideal case, `startActiveSpan` should generally follow this code path. If the provided callback throws an exception, the span/transaction created in step 2 should be marked as errored. This error should not be swallowed by `startActiveSpan`. -`Sentry.startSpan` will create a span, but not set it as the active span in the current scope. 90% of our documentation and code examples will be referencing `Sentry.startActiveSpan` - `Sentry.startSpan` is just there for edge case users of the API. +`Sentry.startSpan` will create a span, but not set it as the active span in the current scope. ```ts namespace Sentry { @@ -173,7 +174,7 @@ Or when continuing from headers in javascript: Sentry.startSpanFromHeaders(spanCtx, headers); ``` -Since we want to discourage accessing the transaction object directly, the `Sentry.setMeasurement` top level method will also be introduced. This will set a custom performance metric if a transaction exists. +Since we want to discourage accessing the transaction object directly, the `Sentry.setMeasurement` top level method will also be introduced. This will set a custom performance metric if a transaction exists. If a transaction doesn't exist, this method will do nothing. In the future, this method will attach the measurement to the span on the scope, but for now it'll only attach it to the transaction. ```ts namespace Sentry { @@ -261,7 +262,13 @@ The most notable change here is to formally introduce the `attributes` field, an ### Shimming the old schema -TODO +To ensure that we have backwards compatibility, we will shim the old schema to the new schema. This has to be done for both transactions and spans. + +For transactions, we need to start adding the `attributes` field to the trace context, the same way we do for spans. This will allow us to use the same conventions for both transactions and spans. For spans, we can keep and deprecate the `span.data` field, and forward it to `span.attributes` internally. For example, `span.setData()` would just call `span.setAttribute()` internally. + +Since status is changing to be an enum of 3 values from something that previously mapped to http status code, we need to migrate away from it in two steps. First, we'll be marking http status of spans in `span.attributes`, using our [span data semantic conventions](https://develop.sentry.dev/sdk/performance/span-data-conventions). For example, `http.request.status_code` can record the request status code. Next, we'll introduce a mapping where 2xx status codes map to `ok`, 4xx and 5xx status codes map to `error`, and all other status codes map to `unset`. + +Similar to `span.data`, we can keep and deprecate the `span.description` field and forward it to `span.name` internally. For example, `span.setDescription()` would just call `span.setName()` internally. ## Next Steps From 16d2dd1b7dc4cbe32910560c7420cd4302d52f5c Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 15 Jun 2023 17:18:55 -0400 Subject: [PATCH 12/13] add a paragraph around SDKs with single scopes --- README.md | 1 + text/0101-revamping-the-sdk-performance-api.md | 8 +++----- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 5821190a..72bc7366 100644 --- a/README.md +++ b/README.md @@ -53,3 +53,4 @@ This repository contains RFCs and DACIs. Lost? access tokens specifically for this environment - [0092-replay-issue-creation](text/0092-replay-issue-creation.md): Replay Issue Creation - [0095-escalating-forecasts-merged-issues](text/0095-escalating-forecasts-merged-issues.md): Issue States and Escalating Forecasts for Merged issues +- [0101-revamping-the-sdk-performance-api](text/0101-revamping-the-sdk-performance-api.md): Revamping the SDK Performance API diff --git a/text/0101-revamping-the-sdk-performance-api.md b/text/0101-revamping-the-sdk-performance-api.md index 593973fb..0f5a92fc 100644 --- a/text/0101-revamping-the-sdk-performance-api.md +++ b/text/0101-revamping-the-sdk-performance-api.md @@ -145,6 +145,8 @@ In the ideal case, `startActiveSpan` should generally follow this code path. If the provided callback throws an exception, the span/transaction created in step 2 should be marked as errored. This error should not be swallowed by `startActiveSpan`. +`startActiveSpan` only provides the correct parent-child relationship if your platform has proper support for forking scopes. For platforms have a single hub/scope (like the mobile SDKs), this method will not lead to the correct parent-child relationship. The SDK will have to provide a different method for these platforms. One option is for `startActiveSpan` to always attach the span it creates to the root span (the transaction), which means users don't get exact parent-child relationships, but they do get relative relationships between spans using relative durations. + `Sentry.startSpan` will create a span, but not set it as the active span in the current scope. ```ts @@ -168,11 +170,7 @@ For example with go: sentry.StartSpanFromContext(ctx, spanCtx) ``` -Or when continuing from headers in javascript: - -```js -Sentry.startSpanFromHeaders(spanCtx, headers); -``` +Other SDKs can also add a `startSpanFromContext` method where they can make context a variable databag that accepts headers, environment variables, etc. This is useful for easy trace propagation. Since we want to discourage accessing the transaction object directly, the `Sentry.setMeasurement` top level method will also be introduced. This will set a custom performance metric if a transaction exists. If a transaction doesn't exist, this method will do nothing. In the future, this method will attach the measurement to the span on the scope, but for now it'll only attach it to the transaction. From db965103c1a10235d0bbced10eb934ed76529a8f Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 1 Aug 2023 22:10:41 -0400 Subject: [PATCH 13/13] update RFC to move schema changes out of scope --- .../0101-revamping-the-sdk-performance-api.md | 101 +++++++++++------- 1 file changed, 62 insertions(+), 39 deletions(-) diff --git a/text/0101-revamping-the-sdk-performance-api.md b/text/0101-revamping-the-sdk-performance-api.md index 0f5a92fc..14b7bf1b 100644 --- a/text/0101-revamping-the-sdk-performance-api.md +++ b/text/0101-revamping-the-sdk-performance-api.md @@ -1,35 +1,34 @@ - Start Date: 2023-06-12 - RFC Type: decision - RFC PR: https://github.com/getsentry/rfcs/pull/101 -- RFC Status: draft +- RFC Status: approved - RFC Driver: [Abhijeet Prasad](https://github.com/abhiprasad) # Summary This RFC proposes to revamp the performance API in the SDKs. The new API aims to accomplish the following: -- Align both the internal schemas and top level public API with OpenTelemetry and their SDKs. - De-emphasize the concept of transactions from users using a Sentry performance monitoring SDK. - Optimize for making sure performance data is always sent to Sentry. -- Open the SDK up for future work where we support batch span ingestion instead of relying on a transaction solely. +- Open the SDK up for future work where we support batch span ingestion. -# Planned Work +Note: In a previous version of this RFC there was a focus on updating the span schema. This is no longer in scope for this RFC and will be addressed by future work. This section has been moved to an Appendix at the end of the RFC. This is what the following item refers to. -The RFC proposal comes in two steps. +- Align both the internal schemas and top-level public API with OpenTelemetry and their SDKs. [Moved to Appendix](#appendix) -1. Introduce three new top level methods, `Sentry.startActiveSpan` and `Sentry.startSpan` and their language specific variants as well as `Sentry.setMeasurement`. +# Planned Work -This allows us to de-emphasize the concept of hubs, scopes and transactions from users, and instead have them think about just spans. Under the hood, `Sentry.startActiveSpan` and `Sentry.startSpan` should create transactions/spans as appropriate. `Sentry.setMeasurement` is used to abstract away `transaction.setMeasurement` and similar. +The primary planned work in this RFC is to introduce three new top-level methods, `Sentry.startActiveSpan` and `Sentry.startSpan` and their language-specific variants as well as `Sentry.setMeasurement`. -1. Introduce a new span schema that is aligned with OpenTelemetry. +This allows us to de-emphasize the concept of hubs, scopes, and transactions from users, and instead have them think about just spans. Under the hood, `Sentry.startActiveSpan` and `Sentry.startSpan` should create transactions/spans as appropriate. `Sentry.setMeasurement` is used to abstract away `transaction.setMeasurement` and similar. -We change the data model that is referenced and used internally inside the SDK to better reflect OpenTelemetry. This involves adding shims for backwards compatibility and removing redundant fields. +In a previous version of the RFC, there was a follow-up step to update the span schema. This will be focused on later and for now, is not in scope for performance API improvements. # Background Right now every SDK has both the concept of transactions and spans - and to a user they both exist as vehicles of performance data. In addition, the transaction exists as the carrier of distributed tracing information ([dynamic sampling context](https://develop.sentry.dev/sdk/performance/dynamic-sampling-context/) and [sentry-trace info](https://develop.sentry.dev/sdk/performance/#header-sentry-trace)), although this is going to change with the advent of tracing without performance support in the SDKs. -Below is JavaScript example of how to think about performance instrumentation in the JavaScript SDKs (browser/node) +Below is a JavaScript example of how to think about performance instrumentation in the JavaScript SDKs (browser/node) ```jsx // op is defined by https://develop.sentry.dev/sdk/performance/span-operations/ @@ -62,7 +61,7 @@ const secondSpan = transaction.startChild({ description: "Another Task" }); transaction.finish(); Sentry.getCurrentHub().getScope().setSpan(undefined); -// second span info is not sent to Sentry because transaction is already finished. +// second span info is not sent to Sentry because the transaction is already finished. secondSpan.finish(); ``` @@ -81,22 +80,22 @@ work(); span.finish(); -// span is finished, so parent is put back onto scope +// span is finished, so the parent is put back onto the scope Sentry.getCurrentHub().getScope().setSpan(parentSpan); ``` Most users do the above also when nested within their application, as often you assume that a transaction is defined that you can attach (very common in a web server context). -To add instrumentation to their applications, users have to know concepts about hubs/scopes/transactions/spans and understand all the different nuances and use cases. This can be difficult, and presents a big barrier to entry for new users. +To add instrumentation to their applications, users have to know concepts about hubs/scopes/transactions/spans and understand all the different nuances and use cases. This can be difficult and presents a big barrier to entry for new users. -Also, since transactions/spans are different classes (span is a parent class of transaction), users have to understand the impacts that the same method will have on both the transaction/span. For example, currently calling `setTag` on a transaction will add a tag to the transaction event (which is searchable in Sentry), while `setTag` on a span just adds it to the span, and the field is not searchable. `setData` on a span adds it to `span.data`, while `setData` on a transaction is undefined behaviour (some SDKs throw away the data, others add it to `event.extras`). +Also, since transactions/spans are different classes (span is a parent class of transaction), users have to understand the impacts that the same method will have on both transactions/spans. For example, currently calling `setTag` on a transaction will add a tag to the transaction event (which is searchable in Sentry), while `setTag` on a span just adds it to the span, and the field is not searchable. `setData` on a span adds it to `span.data`, while `setData` on a transaction is undefined behaviour (some SDKs throw away the data, others add it to `event.extras`). Summarizing, here are the core Issues in SDK Performance API: 1. Users have to understand the difference between spans/transactions and their schema differences. 2. Users have to set/get transactions/spans from a scope (meaning they also have to understand what a scope/hub means). 3. Nesting transactions within each other is undefined behaviour - no obvious way to make a transaction a child of another. -4. If a transaction finishes before it’s child span finishes, that child span gets orphaned and the data is never sent to Sentry. This is most apparent if you have transactions that automatically finish (like on browser/mobile). +4. If a transaction finishes before its child span finishes, that child span gets orphaned and the data is never sent to Sentry. This is most apparent if you have transactions that automatically finish (like on browser/mobile). 5. Transactions have a max child span count of 1000 which means that eventually data is lost if you keep adding child spans to a transaction. # Improvements @@ -108,9 +107,13 @@ The new SDK API has the following requirements: 1. Newly created spans must have the correct trace and parent/child relationship 2. Users shouldn’t be burdened with knowing if something is a span/transaction 3. Spans only need a name to identify themselves, everything else is optional. -4. The new top level APIs should be as similar to the OpenTelemetry SDK public API as possible. +4. The new top-level APIs should be as similar to the OpenTelemetry SDK public API as possible. + +### Span Creators -There are two top level methods we'll be introducing to achieve this: `Sentry.startActiveSpan` and `Sentry.startSpan`. `Sentry.startActiveSpan` will take a callback and start/stop a span automatically. In addition, it'll also set the span on the current scope. Under the hood, the SDK will create a transaction or span based on if there is already an existing span on the scope. +There are two top-level methods we'll be introducing to achieve this: `Sentry.startActiveSpan` and `Sentry.startSpan`. `Sentry.startActiveSpan` will take a callback and start/stop a span automatically. In addition, it'll also set the span on the current scope. Under the hood, the SDK will create a transaction or span based on if there is already an existing span on the scope. + +The reason for electing to have `startActiveSpan` and `startSpan` as separate top-level methods is to not burden the user with having to know about scope propagation, since the concept of a scope might change in future versions of the unified API. If this is not viable at all for a language or certain frameworks, SDK authors can opt to include the `onScope` parameter to the `startSpan` call that will automatically set the span on the current scope, but this is not recommended. ```ts namespace Sentry { @@ -128,7 +131,7 @@ const returnVal = Sentry.startActiveSpan( (span: Span) => expensiveCalc() ); -// If the SDK needs async/sync typed different we can expose this +// If the SDK needs async/sync typed differently it can be exposed as `startActiveSpanAsync` // declare function startActiveSpanAsync( // spanContext: SpanContext, // callback: (span: Span) => Promise, @@ -140,12 +143,12 @@ In the ideal case, `startActiveSpan` should generally follow this code path. 1. Get the active span from the current scope 2. If the active span is defined, create a child span of that active span based on the provided `spanContext`, otherwise create a transaction based on the provided `spanContext`. 3. Run the provided callback -4. Finish the child span/transaction created in step 2 +4. Finish the child span/transaction created in Step 2 5. Remove the child span/transaction from the current scope and if it exists, set the previous active span as the active span in the current scope. -If the provided callback throws an exception, the span/transaction created in step 2 should be marked as errored. This error should not be swallowed by `startActiveSpan`. +If the provided callback throws an exception, the span/transaction created in Step 2 should be marked as errored. This error should not be swallowed by `startActiveSpan`. -`startActiveSpan` only provides the correct parent-child relationship if your platform has proper support for forking scopes. For platforms have a single hub/scope (like the mobile SDKs), this method will not lead to the correct parent-child relationship. The SDK will have to provide a different method for these platforms. One option is for `startActiveSpan` to always attach the span it creates to the root span (the transaction), which means users don't get exact parent-child relationships, but they do get relative relationships between spans using relative durations. +`startActiveSpan` only provides the correct parent-child relationship if your platform has proper support for forking scopes. For platforms that have a single hub/scope (like the mobile SDKs), this method will not lead to the correct parent-child relationship. The SDK will have to provide a different method for these platforms. The recommended option here is for `startActiveSpan` to always attach the span it creates to the root span (the transaction), which means users don't get exact parent-child relationships, but they do get relative relationships between spans using relative durations. `Sentry.startSpan` will create a span, but not set it as the active span in the current scope. @@ -154,7 +157,7 @@ namespace Sentry { declare function startSpan(spanContext: SpanContext): Span; } -// does not get put on scope +// does not get put on the scope const span = Sentry.startSpan({ name: "expensiveCalc" }); expensiveCalc(); @@ -162,17 +165,31 @@ expensiveCalc(); span.finish(); ``` -The only methods that all SDKs are required to implement are `Sentry.startActiveSpan` and `Sentry.startSpan`. For languages that need it, they add an additional method for async callbacks: `Sentry.startActiveSpanAsync`. Other languages can also attach a suffix to the methods to indicate that the spans are being started from different sources, but these are language/framework/sdk dependent. +The goal here is to make span creation consistent across all SDKs, but we also want to make sure that the SDKs are idiomatic for their language/runtime. SDK authors are free to add additional methods to start spans if they feel that `startActiveSpan` and `startSpan` are not appropriate for their language/framework/runtime. -For example with go: +For example, with go we could have a method that starts a span from a go context: ```go sentry.StartSpanFromContext(ctx, spanCtx) ``` -Other SDKs can also add a `startSpanFromContext` method where they can make context a variable databag that accepts headers, environment variables, etc. This is useful for easy trace propagation. +SDK authors can also change the behaviour of `startActiveSpan` and `startSpan` if they feel that the outlined behaviour is not idiomatic for their language/framework/runtime, but this is not recommended and should be discussed with the greater SDK team before being implemented. + +### Span Name + +To accommodate the inclusion of `Sentry.startActiveSpan` and `Sentry.startSpan`, the `span.name` field should be used and is an alias for `span.description`. Span name should become required for the span context argument that is accepted by `Sentry.startActiveSpan` and `Sentry.startSpan`. + +This means methods for setting a span name should be added to the span interface. + +```ts +span.setName(name: string): void; +``` + +Under the hood, calling `span.setName` should set the `span.description` field for backward compatibility reasons. + +### Setting measurements -Since we want to discourage accessing the transaction object directly, the `Sentry.setMeasurement` top level method will also be introduced. This will set a custom performance metric if a transaction exists. If a transaction doesn't exist, this method will do nothing. In the future, this method will attach the measurement to the span on the scope, but for now it'll only attach it to the transaction. +Since we want to discourage accessing the transaction object directly, the `Sentry.setMeasurement` top-level method will also be introduced. This will set a custom performance metric if a transaction exists. If a transaction doesn't exist, this method will do nothing. In the future, this method will attach the measurement to the span on the scope, but for now, it'll only attach it to the transaction. ```ts namespace Sentry { @@ -180,6 +197,16 @@ namespace Sentry { } ``` +## Next Steps + +Since we only have `beforeSend` hooks for a transaction, we should look toward building similar hooks for span start and finish as well. This can be done after the span schema has been changed in the SDKs. + +# Appendix + +This RFC previously contained information for changing the span schema. This is no longer in the scope of the RFC but is documented here for posterity's sake. + +Step 2 of the RFC was: Introduce a new span schema that is aligned with OpenTelemetry where we would change the data model that is referenced and used internally inside the SDK to better reflect OpenTelemetry. This involves adding shims for backward compatibility and removing redundant fields. + ## Span Schema To remove the overhead of understanding transactions/spans and their differences, we propose to simplify the span schema to have a minimal set of required fields. @@ -188,7 +215,7 @@ To remove the overhead of understanding transactions/spans and their differences The current transaction schema inherits from the error event schema, with a few fields that are specific to transactions. -A full version of this protocol can be seen in [Relay](https://github.com/getsentry/relay/blob/2ad761f64db3df9b4d42f2c0896e1f6d99c16f49/relay-general/src/protocol/event.rs), but here are some of the fields that are important: +A full version of this protocol can be seen in [Relay](https://github.com/getsentry/relay/blob/2ad761f64db3df9b4d42f2c0896e1f6d99c16f49/relay-general/src/protocol/event.rs), but here are some of the important fields: | Transaction Field | Description | Type | | ----------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------- | @@ -225,15 +252,15 @@ The current span schema is as follows: | `tags` | Custom tags for this span. | Object | | `data` | Arbitrary additional data on a span, like `extra` on the top-level event. We maintain [conventions for span data keys and values](https://develop.sentry.dev/sdk/performance/span-data-conventions/). | Object | -As you can see, the fields on the transaction/span differ in a few ways, the most noteable of which is that transactions have `name` while spans have `description`. This means that spans and transactions are not interchangeable, and users have to know the difference between the two. +As you can see, the fields on the transaction/span differ in a few ways, the most notable of which is that transactions have `name` while spans have `description`. This means that spans and transactions are not interchangeable, and users have to know the difference between the two. -In addition, user's have the burden to understand the differences between `name`/`description`/`operation`. `operation` in particular can be confusing, as it overlaps with transaction `name` and span `description`. In addition, `operation` is not a required field, which means that it is not clear what the default value should be. +In addition, users have the burden to understand the differences between `name`/`description`/`operation`. `operation` in particular can be confusing, as it overlaps with transaction `name` and span `description`. In addition, `operation` is not a required field, which means that it is not clear what the default value should be. Transactions also have no mechanism for arbitrary additional data like spans do with `data`. Users can choose to add arbitrary data to transactions by adding it to the `contexts` field (as transactions extend the event schema), but this is not obvious and not exposed in every SDK. Since contexts are already well defined in their own way, there is no way of using [Sentry's semantic conventions for span data](https://develop.sentry.dev/sdk/performance/span-data-conventions/) for transactions. ### New Span Schema -To simplify how performance data is consumed and understood, we are proposing a new span schema that the SDKs send to Sentry. The new span schema aims to be a superset of the [OpenTelemetry span schema](https://github.com/open-telemetry/opentelemetry-proto/blob/4967b51b5cb29f725978362b9d413bae1dbb641c/opentelemetry/proto/trace/v1/trace.proto) and have a minimal top level API surface. This also means that spans can be easily converted to OpenTelemetry spans and vice versa. +To simplify how performance data is consumed and understood, we are proposing a new span schema that the SDKs send to Sentry. The new span schema aims to be a superset of the [OpenTelemetry span schema](https://github.com/open-telemetry/opentelemetry-proto/blob/4967b51b5cb29f725978362b9d413bae1dbb641c/opentelemetry/proto/trace/v1/trace.proto) and have a minimal top-level API surface. This also means that spans can be easily converted to OpenTelemetry spans and vice versa. The new span schema is as follows: @@ -250,24 +277,20 @@ The new span schema is as follows: | `attributes` | A set of attributes on the span. | Object | This maps to `span.data` in the current schema for spans. There is no existing mapping for this in the current transaction schema. The keys of attributes are well known values, and defined by a combination of OpenTelemtry's and Sentry's semantic conventions. | | `measurements` | Measurements which holds observed values such as web vitals. | Object | | -For the purposes of this RFC, the version on the span schema will be set to 2. This will indicate to all consumers that the new span schema is being used. +For this RFC, the version on the span schema will be set to 2. This will indicate to all consumers that the new span schema is being used. Just like both the old Sentry schema and the OpenTelemetry schema, we keep the same fields for `span_id`, `trace_id`, `parent_span_id`, `start_timestamp`, and `end_timestamp`. We also choose to rename `description` to `name` to match the OpenTelemetry schema. -Having both the `name` and `op` fields is redundant, but we choose to keep both for backwards compatibility. There are many features in the product that are powered by having a meaningful operation, more details about this can be found in the documentation around [Span operations](https://develop.sentry.dev/sdk/performance/span-operations/). In the future, we can choose to deprecate `op` and remove it from the schema. +Having both the `name` and `op` fields is redundant, but we choose to keep both for backward compatibility. There are many features in the product that are powered by having a meaningful operation, more details about this can be found in the documentation around [Span operations](https://develop.sentry.dev/sdk/performance/span-operations/). In the future, we can choose to deprecate `op` and remove it from the schema. -The most notable change here is to formally introduce the `attributes` field, and remove the `span.data` field. This is a breaking change, but worth it in the long-term. If we start accepting `attributes` on transactions as well, we more closely align with the OpenTelemetry schema, and can use the same conventions for both spans and transactions. +The most notable change here is to formally introduce the `attributes` field, and remove the `span.data` field. This is a breaking change, but worth it in the long term. If we start accepting `attributes` on transactions as well, we more closely align with the OpenTelemetry schema and can use the same conventions for both spans and transactions. ### Shimming the old schema -To ensure that we have backwards compatibility, we will shim the old schema to the new schema. This has to be done for both transactions and spans. +To ensure that we have backward compatibility, we will shim the old schema to the new schema. This has to be done for both transactions and spans. For transactions, we need to start adding the `attributes` field to the trace context, the same way we do for spans. This will allow us to use the same conventions for both transactions and spans. For spans, we can keep and deprecate the `span.data` field, and forward it to `span.attributes` internally. For example, `span.setData()` would just call `span.setAttribute()` internally. -Since status is changing to be an enum of 3 values from something that previously mapped to http status code, we need to migrate away from it in two steps. First, we'll be marking http status of spans in `span.attributes`, using our [span data semantic conventions](https://develop.sentry.dev/sdk/performance/span-data-conventions). For example, `http.request.status_code` can record the request status code. Next, we'll introduce a mapping where 2xx status codes map to `ok`, 4xx and 5xx status codes map to `error`, and all other status codes map to `unset`. +Since status is changing to be an enum of 3 values from something that was previously mapped to http status code, we need to migrate away from it in two steps. First, we'll be marking http status of spans in `span.attributes`, using our [span data semantic conventions](https://develop.sentry.dev/sdk/performance/span-data-conventions). For example, `http.request.status_code` can record the request status code. Next, we'll introduce a mapping where 2xx status codes map to `ok`, 4xx and 5xx status codes map to `error`, and all other status codes map to `unset`. Similar to `span.data`, we can keep and deprecate the `span.description` field and forward it to `span.name` internally. For example, `span.setDescription()` would just call `span.setName()` internally. - -## Next Steps - -Since we only have `beforeSend` hooks for a transaction, we should look toward building similar hooks for span start and finish as well. This can be done after the span schema has been changed in the SDKs.