From 67ad7ea0da3f58a3bdc2450a4b18d2a650ecab69 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 3 Nov 2022 16:50:15 +0100 Subject: [PATCH 1/5] SDK Lifecycle Hooks --- text/XXXX-sdk-lifecycle-hooks.md | 34 ++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 text/XXXX-sdk-lifecycle-hooks.md diff --git a/text/XXXX-sdk-lifecycle-hooks.md b/text/XXXX-sdk-lifecycle-hooks.md new file mode 100644 index 00000000..a99c7892 --- /dev/null +++ b/text/XXXX-sdk-lifecycle-hooks.md @@ -0,0 +1,34 @@ +* Start Date: YYYY-MM-DD +* RFC Type: feature / decision / informational +* RFC PR: + +# 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 4ae43a7d7376abe894df7a2b9eea3e5a75510778 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 3 Nov 2022 16:57:39 +0100 Subject: [PATCH 2/5] update with PR description --- README.md | 1 + text/0034-sdk-lifecycle-hooks.md | 36 ++++++++++++++++++++++++++++++++ text/XXXX-sdk-lifecycle-hooks.md | 34 ------------------------------ 3 files changed, 37 insertions(+), 34 deletions(-) create mode 100644 text/0034-sdk-lifecycle-hooks.md delete mode 100644 text/XXXX-sdk-lifecycle-hooks.md diff --git a/README.md b/README.md index 301a7ca5..6a626341 100644 --- a/README.md +++ b/README.md @@ -20,6 +20,7 @@ This repository contains RFCs and DACIs. Lost? - [0022-response-context](text/0022-response-context.md): Response context - [0033-view-hierarchy](text/0033-view-hierarchy.md): View Hierarchy - [0027-manual-disabling-of-flaky-tests](text/0027-manual-disabling-of-flaky-tests.md): Processes for manually disabling flaky tests in `sentry` and `getsentry` +- [00034-sdk-lifecycle](text/0034-sdk-lifecycle-hooks.md): SDK Lifecycle hooks - [0036-auto-instrumentation-ui-thread](text/0036-auto-instrumentation-ui-thread.md): auto-instrumentation UI thread - [0037-anr-rates](text/0037-anr-rates.md): Calculating accurate ANR rates - [0038-scrubbing-sensitive-data](text/0038-scrubbing-sensitive-data.md): Scrubbing sensitive data - how to improve diff --git a/text/0034-sdk-lifecycle-hooks.md b/text/0034-sdk-lifecycle-hooks.md new file mode 100644 index 00000000..26dfdc62 --- /dev/null +++ b/text/0034-sdk-lifecycle-hooks.md @@ -0,0 +1,36 @@ +- Start Date: 2022-11-03 +- RFC Type: feature +- RFC PR: [https://github.com/getsentry/rfcs/pull/34](https://github.com/getsentry/rfcs/pull/34) +- RFC Status: approved +- RFC Driver: [Abhijeet Prasad](https://github.com/AbhiPrasad) + +# 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? diff --git a/text/XXXX-sdk-lifecycle-hooks.md b/text/XXXX-sdk-lifecycle-hooks.md deleted file mode 100644 index a99c7892..00000000 --- a/text/XXXX-sdk-lifecycle-hooks.md +++ /dev/null @@ -1,34 +0,0 @@ -* Start Date: YYYY-MM-DD -* RFC Type: feature / decision / informational -* RFC PR: - -# 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 a6b7b99190b50ce1712632a6c0d0883c36615517 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Fri, 11 Nov 2022 16:52:13 +0100 Subject: [PATCH 3/5] initial material --- text/0034-sdk-lifecycle-hooks.md | 88 ++++++++++++++++++++++++++++---- 1 file changed, 79 insertions(+), 9 deletions(-) diff --git a/text/0034-sdk-lifecycle-hooks.md b/text/0034-sdk-lifecycle-hooks.md index 26dfdc62..7348cedf 100644 --- a/text/0034-sdk-lifecycle-hooks.md +++ b/text/0034-sdk-lifecycle-hooks.md @@ -6,24 +6,94 @@ # Summary -One paragraph explanation of the feature or document purpose. +This PR proposes the introduction of lifecycle hooks in the SDK, improving extensibility and usability of SDKs at Sentry. + +Lifecyle hooks can be registered as a top level method, and allow for integrations/sdk users to have finely grained control over the event lifecycle in the SDK. + +```ts +Sentry.on("hook-name", (...args) => { + someLogic(); +}); +``` # Motivation -Why are we doing this? What use cases does it support? What is the expected outcome? +There are three main ways users can extend functionality in the SDKs right now. + +At it's current form, the SDK is an event processing pipeline. It takes in some data (an error/message, a span, a profile), turns it into the event, attaches useful context to that event based on the current scope, and then sends that event to Sentry. + +``` +| Error | ---> | Event | ---> | EventWithContext | ---> | Envelope | ---> | Transport | ---> | Sentry | +``` + +``` +| TransactionStart | ---> | SpanStart | ---> | SpanFinish | ---> | TransactionFinish | --> | Event | ---> | EventWithContext | ---> | Envelope | ---> | Transport | ---> | Sentry | +``` + +``` +| Session | ---> | Envelope | ---> | Transport | ---> | Sentry | +``` + +The SDKs provide a few ways to extend this pipeline: + +1. Event Processors (what Integrations use) +2. `beforeSend` callback +3. `beforeBreadcrumb` callback + +But these are all top level options in someway, and are part of the unified API as a result. This means that in certain scenarios, they are not granular enough as extension points. + +# Proposal + +SDK hooks live on the client, and are **stateless**. They are called in the order they are registered. SDKs can opt-in to whatever hooks they use, and there can be hooks unique to an SDK. + +```ts +class Client { + hooks: { + [hookName: string]: HookCallback[]; + }; + + on(hookName: string, callback: HookCallback) { + this.hooks[hookName].push(callback); + } +} +``` + +## Hooks + +Hooks can return `null` to short-circuit the pipeline. + +- `captureException` -# Background +```ts +type onCaptureException = ( + exception: T, + hint?: EventHint, + scope?: Scope +) => T | null; +``` -The reason this decision or document is required. This section might not always exist. +- `captureMessage` -# Supporting Data +```ts +type onCaptureMessage = ( + message: string, + level?: Severity, + hint?: EventHint, + scope?: Scope +) => string | null; +``` -[Metrics to help support your decision (if applicable).] +- `captureEvent` -# Options Considered +```ts +type onCaptureEvent = ( + event: Event, + hint?: EventHint, + scope?: Scope +) => string | null; +``` -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. +- `capture # Drawbacks From a2c88e13c4ca797a912346ead781568531076fda Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 16 Mar 2023 15:37:38 +0100 Subject: [PATCH 4/5] simplify hooks proposal --- text/0034-sdk-lifecycle-hooks.md | 85 +++++++++++++++++++------------- 1 file changed, 52 insertions(+), 33 deletions(-) diff --git a/text/0034-sdk-lifecycle-hooks.md b/text/0034-sdk-lifecycle-hooks.md index 7348cedf..7f27ace6 100644 --- a/text/0034-sdk-lifecycle-hooks.md +++ b/text/0034-sdk-lifecycle-hooks.md @@ -8,12 +8,20 @@ This PR proposes the introduction of lifecycle hooks in the SDK, improving extensibility and usability of SDKs at Sentry. -Lifecyle hooks can be registered as a top level method, and allow for integrations/sdk users to have finely grained control over the event lifecycle in the SDK. +To test out this RFC, we implemented it in the [JavaScript SDK](https://github.com/getsentry/sentry-javascript/blob/7aa20d04a3d61f30600ed6367ca7151d183a8fc9/packages/types/src/client.ts#L153) with great success, so now we are looking to propose and implement it in the other SDKs. + +Lifecyle hooks can be registered on a Sentry client, and allow for integrations/sdk users to have finely grained control over the event lifecycle in the SDK. ```ts -Sentry.on("hook-name", (...args) => { - someLogic(); -}); +interface Client { + // ... + + on(hookName: string, callback: (...args: unknown) => void) => void; + + emit(hookName: string, ...args: unknown[]) => void; + + // ... +} ``` # Motivation @@ -42,65 +50,76 @@ The SDKs provide a few ways to extend this pipeline: But these are all top level options in someway, and are part of the unified API as a result. This means that in certain scenarios, they are not granular enough as extension points. +The following are some examples of how sdk hooks can unblock new features and integrations, but not a definitive list: + +- Integrations want to add information to spans when they start or finish. This is what [RFC #75 is running into](https://github.com/getsentry/rfcs/pull/75), where they want to add thread information to each span. +- Integrations want to add information to envelopes before they are sent to Sentry. +- Integrations want to run code on transaction/span finish (to add additional spans to the transaction, for example). +- Integrations want to mutate an error on `captureException` +- Integrations want to override propagation behaviour (extracing/injecting outgoing headers) + # Proposal SDK hooks live on the client, and are **stateless**. They are called in the order they are registered. SDKs can opt-in to whatever hooks they use, and there can be hooks unique to an SDK. +Hooks are meant to be mostly internal APIs for integration authors, but we can also expose them to SDK users if there is a use case for it. + +As hook callbacks are not processed by the client, they can be async functions. + ```ts +// Example implementation in JavaScript + +type HookCallback = (...args: unknown[]): void; + class Client { hooks: { [hookName: string]: HookCallback[]; }; - on(hookName: string, callback: HookCallback) { + on(hookName: string, callback: HookCallback): void { this.hooks[hookName].push(callback); } + + emit(hookName: string, ...args: unknown[]): void { + this.hooks[hookName].forEach(callback => callback(...args)); + } } ``` +SDKs are expected to have a common set of hooks, but can also have SDK specific hooks. + ## Hooks -Hooks can return `null` to short-circuit the pipeline. +These following are a set of example hooks that would unblock some use cases listed above. The names/schema of the hooks are not final, and are meant to be used as a starting point for discussion. + +To document and approve new hooks, we will create a new page in the develop docs that lists all the hooks, and what they are used for. -- `captureException` +`startTransaction`: ```ts -type onCaptureException = ( - exception: T, - hint?: EventHint, - scope?: Scope -) => T | null; +on('startTransaction', callback: (transaction: Transaction) => void) => void; ``` -- `captureMessage` +`finishTransaction`: ```ts -type onCaptureMessage = ( - message: string, - level?: Severity, - hint?: EventHint, - scope?: Scope -) => string | null; +on('finishTransaction', callback: (transaction: Transaction) => void) => void; ``` -- `captureEvent` +`startSpan`: ```ts -type onCaptureEvent = ( - event: Event, - hint?: EventHint, - scope?: Scope -) => string | null; +on('startSpan', callback: (span: Span) => void) => void; ``` -- `capture +`finishSpan`: -# Drawbacks - -Why should we not do this? What are the drawbacks of this RFC or a particular option if -multiple options are presented. +```ts +on('finishSpan', callback: (span: Span) => void) => void; +``` -# Unresolved questions +`beforeEnvelope`: -- 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? +```ts +on('beforeEnvelope', callback: (envelope: Envelope) => void) => void; +``` From 18d4e7a892a7b022d8414c8a55cace3ff6fd6288 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 27 Mar 2023 10:50:00 +0200 Subject: [PATCH 5/5] PR review notes --- text/0034-sdk-lifecycle-hooks.md | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/text/0034-sdk-lifecycle-hooks.md b/text/0034-sdk-lifecycle-hooks.md index 7f27ace6..99517e3a 100644 --- a/text/0034-sdk-lifecycle-hooks.md +++ b/text/0034-sdk-lifecycle-hooks.md @@ -10,7 +10,9 @@ This PR proposes the introduction of lifecycle hooks in the SDK, improving exten To test out this RFC, we implemented it in the [JavaScript SDK](https://github.com/getsentry/sentry-javascript/blob/7aa20d04a3d61f30600ed6367ca7151d183a8fc9/packages/types/src/client.ts#L153) with great success, so now we are looking to propose and implement it in the other SDKs. -Lifecyle hooks can be registered on a Sentry client, and allow for integrations/sdk users to have finely grained control over the event lifecycle in the SDK. +Lifecyle hooks can be registered on a Sentry client, and allow for integrations to have finely grained control over the event lifecycle in the SDK. + +Currently hooks are meant to be completely internal to SDK implementors - but we can re-evaluate this in the future. ```ts interface Client { @@ -64,7 +66,7 @@ SDK hooks live on the client, and are **stateless**. They are called in the orde Hooks are meant to be mostly internal APIs for integration authors, but we can also expose them to SDK users if there is a use case for it. -As hook callbacks are not processed by the client, they can be async functions. +As hook callbacks are not processed by the client. Data passed into can be mutated, which can have side effects on the event pipeline, and consquences for hooks with async callbacks. As such, we recommend using hooks for synchronous operations only, but SDK authors can choose to implement them as async if they need to. ```ts // Example implementation in JavaScript @@ -86,6 +88,12 @@ class Client { } ``` +For languages that cannot use dynamic strings as hook names, alternate options should be considered. For example, we could use a `Hook` enum, or a `Hook` class with static properties. + +The primary hook functions named `on` and `emit` can also change based on implementor SDK. For example, using `addObserver` and `notifyObserver` may work better in the Java and Apple SDKs (since those are the names of the observer pattern in those languages). + +Hook callbacks are recommended to be forwards compatible, and forward any arguments they don't use. This allows us to add new arguments to hooks without breaking integrations. For example in Python we should catch and forward extra keyword arguments to the callback. + SDKs are expected to have a common set of hooks, but can also have SDK specific hooks. ## Hooks @@ -123,3 +131,5 @@ on('finishSpan', callback: (span: Span) => void) => void; ```ts on('beforeEnvelope', callback: (envelope: Envelope) => void) => void; ``` + +Other possible hooks inlcude `beforeBreadcrumb`, `beforeSend`, `startSession` and `endSession`.