rfc(feature): SDK Lifecycle Hooks #34

AbhiPrasad · 2022-11-03T15:53:08Z

This PR proposes the introduction of lifecycle hooks in the SDK, improving extensibility and usability of SDKs at Sentry.

To test out this RFC, we implemented it in the JavaScript SDK 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.

interface Client {
  // ...

  on(hookName: string, callback: (...args: unknown) => void) => void;

  emit(hookName: string, ...args: unknown[]) => void;

  // ...
}

JS SDK work: getsentry/sentry-javascript#7262

text/0034-sdk-lifecycle-hooks.md

AbhiPrasad · 2023-03-16T14:39:01Z

I simplified and re-wrote this proposal based on the experience we had adding and using hooks within the JavaScript SDK. It is now ready for another set of reviews (only took a billion months lol)

sl0thentr0py

shippppit
should we think of common hooks across SDKs or is this completely left to SDK authors?

AbhiPrasad · 2023-03-16T15:34:00Z

should we think of common hooks across SDKs or is this completely left to SDK authors?

We will define a set of initial ones in the develop docs, but the rest can be left to SDK authors to solve their platform specific problems.

Since for now hooks are meant to be used internally I don't think we worry about SDK user concerns yet, but maybe we can re-evaluate this in 2-3 months after more SDKs adopt it.

cleptric

This is a great example of finding a very concise and simple solution to a problem that might be super platform-specific, so being able to extend this on an SDK by SDK basis is 👌

yordis · 2023-03-16T21:50:27Z

text/0034-sdk-lifecycle-hooks.md

+
+type HookCallback = (...args: unknown[]): void;
+
+class Client {


I am a bit confused, who implements this Client? What exactly the end-user interacts with?

Thanks a lot for showing an interest in our RFC process! 🚀
This is meant to be the client implementation we have in each SDK, see https://develop.sentry.dev/sdk/unified-api/#client

I need to think a layer higher and across multiple languages and clients. Sorry for the confusion, and thank you so much for your response!

@cleptric, I also need some clarification. Does this mean I need to implement a client when I want to add lifecycle hooks? Can you share the code a user must write when implementing a lifecycle hook?

The on and emit hooks will be defined on the client interface (much like flush and capture_event are at the moment).

Usually users (if writing custom clients) are inheriting from a base client implementation anyway, so they should get the hooks functionality out of the box.

Thanks for the clarification, @AbhiPrasad 😃 .

On mobile, hardly anybody implements a custom client. Most users interact with the static API. Implementing a custom client and binding this to the hub, then binding the hub to the static API, will not be the cleanest API on Mobile. Our users usually only call SentrySDK.start with the options, and that's it.

marandaneto · 2023-03-17T09:06:10Z

text/0034-sdk-lifecycle-hooks.md

+`beforeEnvelope`:
+
+```ts
+on('beforeEnvelope', callback: (envelope: Envelope) => void) => void;
+```


Why do we need a hook for beforeEnvelope? Can we make it internal at least? AFAIK this is not exposed for clients and the whole envelope impl. is marked as internal in Mobile SDKs at least.

Yes we can make some hooks internal. I will clarify this in the RFC.

marandaneto · 2023-03-17T09:07:57Z

text/0034-sdk-lifecycle-hooks.md

+
+SDKs are expected to have a common set of hooks, but can also have SDK specific hooks.
+
+## Hooks


What's about hooks for startSession and endSession?

This was not an exhaustive list, but I will add those.

Lms24

Great RFC and big +1 on the "implementing it as needed per SDK" approach!

Lms24 · 2023-03-17T09:37:36Z

text/0034-sdk-lifecycle-hooks.md

+
+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.


I'm wondering if we should clarify the mutability of the data (events, hints, breadcrumbs, etc) passed to the callbacks. If we allow callbacks to be async, we need to be aware that mutations made to these objects might come in too late, especially for hooks around processing and sending events.

Then again, I feel like we shouldn't prohibit mutation, as outlined by the use cases listed in this RFC (also this practical example).

When we write the dev spec for hooks, we should add that implementers need to think about mutability and async characteristics of the hook(s) they're implementing.

Good point, I will add notes around this.

mitsuhiko · 2023-03-20T14:53:28Z

I would like to explicitly document the requirements for callbacks with regards to forward compatibility. In particular in Python for instance we should expect them to always catch extra keyword arguments so that additional data can be passed in the future.

yordis · 2023-03-20T15:40:40Z

@mitsuhiko, your concern is why I pointed out that having a single argument object is much better, so you avoid breaking changes. You can extend API without requiring different function definitions (to some extent)

philipphofmann

Thanks for doing this @AbhiPrasad 👏 .

text/0034-sdk-lifecycle-hooks.md

philipphofmann · 2023-03-21T14:40:56Z

text/0034-sdk-lifecycle-hooks.md

+
+# 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.


@AbhiPrasad, why did we put the hooks on the client and not the options as our existing hooks beforeSend, beforeBreadcrumb, etc.?

These are not meant to be replacements for the top level hooks we have for beforeSend or beforeBreadcrumb for the time being, since the hook names are dynamic strings.

The hooks are on the client so they can be registered dynamically. This is basically the event emitter pattern.

It would be great to put this info in the RFC or the develop docs, as this was unclear to me when reading the RFC.

I didn't write JavaScript for 6 years or so, but event emitter still rings a bell now. For statically typed languages, we might have to adapt them to the language. For Swift and Java, it would maybe be something like addObserver and notifyObserver, or maybe we just stick to the JavaScript syntax. Thanks for explaining.

philipphofmann · 2023-03-21T14:43:31Z

text/0034-sdk-lifecycle-hooks.md

+
+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.
+
+`startTransaction`:


I think we should also add our existing hooks as beforeSend, beforeTransaction, beforeBreadcrumb, etc. to have one way to define all hooks, and we should keep the existing hooks for backward compatibility. Otherwise, users need to use two different APIs for a similar use case.

Yeah that makes sense to me - can add some notes

text/0034-sdk-lifecycle-hooks.md

philipphofmann

If the answer to my question #34 (comment) is yes, then LGTM. Great work @AbhiPrasad 👏

philipphofmann · 2023-03-23T20:02:06Z

text/0034-sdk-lifecycle-hooks.md

+
+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.


integrations/sdk users

That confused me a bit. I thought this API also targets our users, but as you @AbhiPrasad, explained in the TSC, this API is mostly internal. This information is essential to me as it completely changes how I evaluate this RFC.

This was a remnant from before, forgot to change this!

philipphofmann · 2023-03-23T20:03:31Z

text/0034-sdk-lifecycle-hooks.md

+
+# 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.


It would be great to put this info in the RFC or the develop docs, as this was unclear to me when reading the RFC.

I didn't write JavaScript for 6 years or so, but event emitter still rings a bell now. For statically typed languages, we might have to adapt them to the language. For Swift and Java, it would maybe be something like addObserver and notifyObserver, or maybe we just stick to the JavaScript syntax. Thanks for explaining.

philipphofmann · 2023-03-23T20:12:37Z

text/0034-sdk-lifecycle-hooks.md

+- 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)


On Mobile, take a screenshot or attach the view hierarchy when capturing an exception. Could we replace our custom logic of attachment processors with that, @AbhiPrasad? We call some code that returns attachments and then bundle it into an envelope when capturing exceptions.

Yes! We could for sure replace that with this, will add your example to the list!

antonpirker reviewed Nov 22, 2022

View reviewed changes

text/0034-sdk-lifecycle-hooks.md Show resolved Hide resolved

text/0034-sdk-lifecycle-hooks.md Show resolved Hide resolved

text/0034-sdk-lifecycle-hooks.md Show resolved Hide resolved

text/0034-sdk-lifecycle-hooks.md Outdated Show resolved Hide resolved

smeubank mentioned this pull request Nov 22, 2022

Event processor misuse getsentry/sentry-python#1752

Open

3 tasks

AbhiPrasad mentioned this pull request Nov 24, 2022

Ensure replays do not have filtered out errors attached getsentry/sentry-javascript#6285

Closed

kahest mentioned this pull request Dec 7, 2022

Support beforeSendTransaction getsentry/team-mobile#72

Closed

philipphofmann self-requested a review December 7, 2022 16:05

AbhiPrasad mentioned this pull request Dec 12, 2022

Revamp the Integrations API getsentry/sentry-javascript#4790

Closed

yordis reviewed Dec 13, 2022

View reviewed changes

text/0034-sdk-lifecycle-hooks.md Show resolved Hide resolved

yordis reviewed Dec 13, 2022

View reviewed changes

text/0034-sdk-lifecycle-hooks.md Outdated Show resolved Hide resolved

AbhiPrasad mentioned this pull request Feb 23, 2023

SDK lifecycle hooks getsentry/sentry-javascript#7262

Closed

2 tasks

AbhiPrasad added 3 commits March 16, 2023 15:09

SDK Lifecycle Hooks

67ad7ea

update with PR description

4ae43a7

initial material

a6b7b99

AbhiPrasad force-pushed the rfc-sdk-lifecycle-hooks branch from 159635f to a6b7b99 Compare March 16, 2023 14:09

simplify hooks proposal

a2c88e1

AbhiPrasad marked this pull request as ready for review March 16, 2023 14:38

AbhiPrasad self-assigned this Mar 16, 2023

sl0thentr0py reviewed Mar 16, 2023

View reviewed changes

sl0thentr0py approved these changes Mar 16, 2023

View reviewed changes

cleptric approved these changes Mar 16, 2023

View reviewed changes

yordis reviewed Mar 16, 2023

View reviewed changes

marandaneto reviewed Mar 17, 2023

View reviewed changes

Lms24 approved these changes Mar 17, 2023

View reviewed changes

philipphofmann reviewed Mar 21, 2023

View reviewed changes

cleptric mentioned this pull request Mar 22, 2023

feat: Add ignore_exceptions & ignore_transactions getsentry/sentry-php#1503

Merged

philipphofmann reviewed Mar 23, 2023

View reviewed changes

marandaneto approved these changes Mar 24, 2023

View reviewed changes

PR review notes

18d4e7a

AbhiPrasad changed the title ~~SDK Lifecycle Hooks~~ rfc(feature): SDK Lifecycle Hooks Mar 27, 2023

AbhiPrasad merged commit e3d5692 into main Mar 27, 2023

AbhiPrasad deleted the rfc-sdk-lifecycle-hooks branch March 27, 2023 08:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rfc(feature): SDK Lifecycle Hooks #34

rfc(feature): SDK Lifecycle Hooks #34

AbhiPrasad commented Nov 3, 2022 •

edited

Loading

AbhiPrasad commented Mar 16, 2023

sl0thentr0py left a comment

AbhiPrasad commented Mar 16, 2023

cleptric left a comment •

edited

Loading

yordis Mar 16, 2023

cleptric Mar 16, 2023 •

edited

Loading

yordis Mar 17, 2023

philipphofmann Mar 21, 2023

AbhiPrasad Mar 21, 2023

philipphofmann Mar 21, 2023

marandaneto Mar 17, 2023

AbhiPrasad Mar 21, 2023

marandaneto Mar 17, 2023

AbhiPrasad Mar 21, 2023

Lms24 left a comment

Lms24 Mar 17, 2023

AbhiPrasad Mar 21, 2023

mitsuhiko commented Mar 20, 2023

yordis commented Mar 20, 2023 •

edited

Loading

philipphofmann left a comment

philipphofmann Mar 21, 2023

AbhiPrasad Mar 21, 2023

philipphofmann Mar 23, 2023

philipphofmann Mar 21, 2023

AbhiPrasad Mar 21, 2023

philipphofmann left a comment •

edited

Loading

philipphofmann Mar 23, 2023

AbhiPrasad Mar 27, 2023

philipphofmann Mar 23, 2023

philipphofmann Mar 23, 2023 •

edited

Loading

AbhiPrasad Mar 27, 2023


		type HookCallback = (...args: unknown[]): void;

		class Client {


		SDKs are expected to have a common set of hooks, but can also have SDK specific hooks.

		## Hooks


		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.


		# 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.


		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.

		`startTransaction`:

rfc(feature): SDK Lifecycle Hooks #34

rfc(feature): SDK Lifecycle Hooks #34

Conversation

AbhiPrasad commented Nov 3, 2022 • edited Loading

AbhiPrasad commented Mar 16, 2023

sl0thentr0py left a comment

Choose a reason for hiding this comment

AbhiPrasad commented Mar 16, 2023

cleptric left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cleptric Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lms24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitsuhiko commented Mar 20, 2023

yordis commented Mar 20, 2023 • edited Loading

philipphofmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipphofmann left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipphofmann Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhiPrasad commented Nov 3, 2022 •

edited

Loading

cleptric left a comment •

edited

Loading

cleptric Mar 16, 2023 •

edited

Loading

yordis commented Mar 20, 2023 •

edited

Loading

philipphofmann left a comment •

edited

Loading

philipphofmann Mar 23, 2023 •

edited

Loading