Bring beforeSend-like capability to transactions

[lobsterkatie]

Proposal to modify SDKs to allow transactions to run through beforeSend or equivalent method

(Originally specifically a proposal for adding beforeSendTransaction, but revised after initial discussion)

Rendered RFC

[marandaneto]

People still use event processors for different reasons on Mobile SDKs, not sure if removing them is the right thing to do, I find it useful in some use cases otherwise your beforeSend just gets bloated with so many things.

[lobsterkatie]

I also think event processors can be useful. I included this question because long ago there was talk of deprecating the method (which is why it's undocumented). We backed off when it became necessary to use it for transaction processing, but our solution here would solve that. @mitsuhiko, I think you had thoughts on this at the time?

[marandaneto]

yeah, I recall event processors being for internal usage only, and never a public API, but they turned out to be very useful in some cases.

[marandaneto]

Fine for me, but I still have the feeling that this leads to the same problem as sampleRate for errors and tracesSampleRate for transactions.
beforeSend for errors, beforeSendTransaction for transactions.
If that's not a concern, all good.

[k-fish]

I also somewhat have a feeling this will add confusion, having more functions on the sdk is just more surface area for people to miss.

[marandaneto]

IIRC, there are more SDKs that run beforeSend for transactions.
Cocoa is one of them getsentry/sentry-cocoa#1730

[AbhiPrasad]

For this, I have a slightly alternative proposal, where in I would like us to adopt more granular span processing hooks, specifically onSpanStart, and onSpanFinish. This is more complicated in the short-term, but in the long term will help because we probably have to have similar APIs around Replays and Profiles.

To accomplish this, we can adopt a new top level option users can configure, a Tracer. This also means users operate on transactions/spans directly instead of an event (which means they don't have to figure out schema differences, like transaction.name === event.transaction etc.).

interface SentryOptions: {
  // ...

  tracer: Tracer

  // ... 
}

interface Tracer {
  onSpanStart?: (span: Span) => Span | null;

  onSpanFinish?: (span: Span) => Span | null;

  // did not include on transaction start since that's handled by sampling options

  onTransactionFinish?: (transaction: Transaction) => Transaction | null;
}

I'm not totally attached to the tracer, but I think it would be a nice way to encapsulate the logic. This is also similar to the open telemetry API. In languages where possible the tracer can just be a plain dictionary, otherwise it's a class you must construct.

[marandaneto]

onSpanFinish

We have something similar for a few integrations, similar to onSpanFinish.
Usually called BeforeSpanCallback.
The signature varies, but usually takes:
fun execute(span: ISpan, request: Request, response: Response?): ISpan?
So you can either drop or mutate the span data based on the request or response.

[sl0thentr0py]

@AbhiPrasad I like the fine-grained lifecycle hooks as well, but they don't solve the problem of 'run some user code at the very end of all other sentry processing'.

In WSGI, we use event_processors to add request stuff to the event and afaict onTransactionFinish would run before those event_processors, thus still not solving the request header PII stripping case.

We should definitely expand on lifecycle events / hooks throughout the SDK, but this beforeSendTransaction is still necessary imo. This is a client/transport lifecycle hook and the Tracer stuff would be a Transaction lifecycle hook.

[AbhiPrasad]

I like the fine-grained lifecycle hooks as well, but they don't solve the problem of 'run some user code at the very end of all other sentry processing'.

Great point. Perhaps then we chose to somehow introduce a new beforeX method (idk what it's called), that is called for all events being sent.

type EventType = 'error' | 'transaction' | 'profile' | 'replay' | 'whatever-comes-next';

// name pending - hardest problem in computer science...
beforeWhatever: (type: EventType, event: Event, hint?: Hint) => Event | null;

Then we have something that'll affect every event type, replay, profile, transaction, etc - making it fully forwards compatible.

[sl0thentr0py]

Yeah, I also prefer a unified method, and that seems to be a fairly general sentiment. Just that the ideal name is already taken so we'll have to be creative. :)

[k-fish]

onSpanFinish

Will I agree we should move in this direction, until we have an exact use / need for span hooks I think that can be a separate discussion / rfc.

Then we have something that'll affect every event type, replay, profile, transaction, etc - making it fully forwards compatible.

Are you proposing just a single function which we can split based on a passed prop? I think that works better 🤔

[marandaneto]

Are you proposing just a single function which we can split based on a passed prop? I think that works better 🤔

The problem with this approach is that some SDKs auto instrument and users don't have a chance to pass a prop unless they do it directly in the SDK init via options.

[lforst]

I like the fine-grained lifecycle hooks as well, but they don't solve the problem of 'run some user code at the very end of all other sentry processing'.

Great point. Perhaps then we chose to somehow introduce a new beforeX method (idk what it's called), that is called for all events being sent.

type EventType = 'error' | 'transaction' | 'profile' | 'replay' | 'whatever-comes-next';

// name pending - hardest problem in computer science...
beforeWhatever: (type: EventType, event: Event, hint?: Hint) => Event | null;

Then we have something that'll affect every event type, replay, profile, transaction, etc - making it fully forwards compatible.

Just for TypeScript type inference this particular API is problematic because the type argument is technically completely independent from the event argument - which makes it impossible for users to narrow down the type of the event based on the type argument.

Ideally, we have a type field directly in the Event type. But this can be an implementation detail that each SDK decides on by itself.

[AbhiPrasad]

Ideally, we have a type field directly in the Event type. But this can be an implementation detail that each SDK decides on by itself.

We do have, https://develop.sentry.dev/sdk/event-payloads/types/#typedef-EventType, but this doesn't cover profiling, which just use envelope item headers to determine the type of the payload (doesn't follow the event format).

profile - https://github.com/getsentry/relay/blob/4894c1d3bab6111585c29c9ea3b55e1511faca60/relay-profiling/src/lib.rs

Replay does use the headers, but it's a different format, different schema.

replay - https://github.com/getsentry/relay/blob/a705412815a96f92eeb88df5ba553644c8f70ade/relay-replays/src/lib.rs#L14

Perhaps we need to standardize this better on ingestion?

[AbhiPrasad]

Actually after writing out the above, maybe there is no world where we can standardize this, and we should just have the explicit methods. Overloaded methods is always a pain, better to be explicit!

[stefanosiano]

Just throwing an idea:
make the Sentry events implement a common interface that allows to "register" a callback to.
In other words, have Errors and Transactions (and later maybe even Profiles and Replays) provide callbacks when they are created, making sampling decision, before sent, before processors, etc.
This way we may have methods on sdk init where the user implemets what they want, like registerErrorCallback and registerTransactionCallback, passing to each of them an object with different methods that can be implemented or ignored, like onEventCreated, onEventSampling, onEventBeforeSent, etc.
So we provide only 1 method to the user per event, instead of having to dig across several methods, but also enough granularity to do whatever they want with that same event.
I come from Android, so it looks fine for me (and that's what the Android sdk already does for its classes), but don't know how much sense it makes on dynamic languages

[lobsterkatie]

methods on sdk init where the user implemets what they want, like registerErrorCallback and registerTransactionCallback, passing to each of them an object with different methods that can be implemented or ignored, like onEventCreated, onEventSampling, onEventBeforeSent, etc.

This sounds like a different implementation of the hooks Abhi was talking about. Here I agree with Neel and Kevan - not a bad idea at all, but probably a separate conversation from this one, which is specifically "how do we give users the same swiss army knife for transactions which they have for errors?"

[lobsterkatie]

So far it seems like people are favoring the single-method option. I actually prefer that option as well, for many of the reasons outlined, so maybe the question is actually a slightly different one: Assuming we go with a single method, should we have it be beforeSend and do it as a breaking change in our respective next major versions, or should we introduce a new method with a new name and eventually deprecate beforeSend?

I will update the RFC a bit to reflect the conversation so far.

[vladanpaunovic]

My vote goes to expand the usage of beforeSend() by passing transactions and later possibly more event types trough it.

I want to echo what I already said in a meeting yesterday:
I believe that beforeSend() should act as an inverted webhook, where instead of receiving various events declared by event.type we are sending events and allowing users to distinguish between them using even.type (or a similar signature).
Users can always use switch/case or if/elseif statements to conditionally massage different data in the isolation.

So far it seems like people are favoring the single-method option. I actually prefer that option as well, for many of the reasons outlined, so maybe the question is actually a slightly different one: Assuming we go with a single method, should we have it be beforeSend and do it as a breaking change in our respective next major versions, or should we introduce a new method with a new name and eventually deprecate beforeSend?

I will update the RFC a bit to reflect the conversation so far.

I would continue using beforeSend() method. Adding more data flow trough this method would not be considered as a breaking change.
The beforeSend() method is semantically correct, it is widely used already and does not require any user actions.

[marandaneto]

Users can always use switch/case or if/elseif statements to conditionally massage different data in the isolation.

@vladanpaunovic one problem with this solution is that some languages don't have Inheritance so you can't have easily 2 types going thru the same callback if strongly typed.

[antonpirker]

So after reading this are we back on square one? The one beforeSend to handle all kind of "events" (errors, transactions, profiles, replay) does not work on strongly typed languages because there is no base class "Event" that all the other classes "Transaction", "Profile", "Replay" inherit from?

Do we need to get our data structures in order first, before we can deal with all the different kind of events in a unified way?

[antonpirker]

I guess also just piping all events/transactions/whatever into beforeSend from now on, can break a lot of users code because if they access a property of an error that does not exist in an transaction than we will break their code.

So I guess we have to create a beforeFinalized (beforeStored, beforeSaved, beforeAccepted, beforeDone, afterBeforeSend(!)) that has the same signature as beforeSend and gets all the errors and additionally all the transactions/profiles/etc.

After 12 months we remove beforeSend and after 12 more months we rename the new beforeX to beforeSend and have a nice API again.

[antonpirker]

To "Event processors happen in an unspecified order": This was also one thing we observed in Python. The user wanted to add an event processor to filter transactions based on something in the request. But our internal integration added this request to the event also with an event processor.
The users event processor ran before the integrations event processor so there was no request in the event (or the hint? cant remember) so it was impossible to filter the transactions in an event processor.

Proposal:

We could add special user_event_processors (or whatever the name) and those event processors are run AFTER all the currently available event processors. To make sure all the SDK internal processing has been done and now the user can do stuff with the event.

(One down side: "all the SDK internal processing" also means the event could have been dropped/sampled already)

[philipphofmann]

That is a good point, @antonpirker, but I think that is a different discussion.

[stefanosiano]

So I guess we have to create a beforeFinalized (beforeStored, beforeSaved, beforeAccepted, beforeDone) that has the same signature as beforeSend and gets all the errors and additionally all the transactions/profiles/etc.

After 12 months we remove beforeSend and after 12 more months we rename the new beforeX to beforeSend and have a nice API again.

We could have a simple beforeSendEvent, deprecate the old beforeSend and create a common event type, so we don't have to name and rename the method.

The question to me is: do we want to keep this beforeSend or similar method in the future?
If we are going to change how the events can be processed, then we may just go with a beforeTransactionSend so we don't break anything, since we would (breaking) change it anyway in the new major version. For example we could give a new method (one for all the events or one for each type of events) to give more granular control, or a new method to replace multiple "old" methods, like a callback that also gives the "status" of the event (created, processed, toBeSampled, etc.)
If we don't want to change how events are going to be processed, then the single method may be better, given it's possible to do in all of the languages.

@vladanpaunovic one problem with this solution is that some languages don't have Inheritance so you can't have easily 2 types going thru the same callback if strongly typed.

@marandaneto Can you give an example of an sdk in a language without inheritance? We could check other ways, like composition or delegation, but if it's not possible then we'd have to separate the methods.

[cleptric]

Using beforeSend for transactions also creates a lot of work, as we have to release new majors for all our SDKs and have our users update their code.
Going forward with beforeSendTransactions feels like it has way less friction.

[lobsterkatie]

I believe that beforeSend() should act as an inverted webhook, where instead of receiving various events declared by event.type we are sending events and allowing users to distinguish between them using even.type (or a similar signature).

I'm not sure what you mean, @vladanpaunovic, because those sound like the same thing to me. beforeSend receives events, those events have a type property, and the user then distinguishes between them based on that property. Am I misunderstanding?

[vladanpaunovic]

I believe that beforeSend() should act as an inverted webhook, where instead of receiving various events declared by event.type we are sending events and allowing users to distinguish between them using even.type (or a similar signature).

I'm not sure what you mean, @vladanpaunovic, because those sound like the same thing to me. beforeSend receives events, those events have a type property, and the user then distinguishes between them based on that property. Am I misunderstanding?

No, you got it right. All good. My explanation wasn't great.

[marandaneto]

@lobsterkatie option 2 sounds good, an alternative for statically typed languages:

public interface BeforeSendCallback {
    // already exists
    @Nullable
    SentryEvent execute(@NotNull SentryEvent event, @NotNull Hint hint);
    
    // adds new method for transaction.
    // sets method as `default` method for transaction, so theres no need to check the type of the event,
    // the method is called accordingly with the class type.
    // no need to use `SentryBaseEvent` nor `event instanceof SentryEvent`
    // friendlier for statically typed languages
    @Nullable 
    default SentryTransaction execute(@NotNull SentryTransaction transaction, @NotNull Hint hint) {
      return transaction;
    }
  }

Would that be ok?
That would not work on Dart tho because there are no overloads, it'd on Java.

@philipphofmann @brustolin would that work on iOS?

[adinauer]

In Java we have SentryBaseEvent which is a super class of SentryEvent and SentryTransaction.

At the moment the signature of beforeSend in Java is:

SentryEvent execute(@NotNull SentryEvent event, @NotNull Hint hint);

Assuming we change the parameter to type SentryBaseEvent it would cause people to have to cast it to SentryEvent to access things like setLevel or isCrashed.

An alternative would be to have two parameters that are @Nullable.
I assume this also applies to other SDKs (.NET).

[philipphofmann]

That is a good point, @antonpirker, but I think that is a different discussion.

[philipphofmann]

If I remember correctly, there we good reasons for not calling beforeSend for transactions, but I can't recall them. @mitsuhiko or @jan-auer could know.

[lobsterkatie]

If I remember correctly, there we good reasons for not calling beforeSend for transactions

Yes - from what I recall, it was exactly the reason I listed there, that the schema mis-match had the potential to break people's beforeSend functions. Or do you mean there was a reason for not having a transaction-specific beforeSend-like option?

[philipphofmann]

It's unclear whether a transaction-specific beforeSend-type hook was discussed at the time.

I think this sentence is inaccurate. We discussed it and most likely the outcome was what you just pointed out: the schema mis-match had the potential to break people's beforeSend functions

Or do you mean there was a reason for not having a transaction-specific beforeSend-like option

Could also be, but I can't recall that.

[lobsterkatie]

Sorry, I think we're miscommunicating here, because I believe we actually agree on the history. Let me try to rephrase:

I know that back in the day, when tracing was brand new, we discussed sending transactions through beforeSend itself, and decided against it because of the schema issue potentially creating a breaking change. What I don't know, and you're saying you can't recall either, is if a separate beforeSend-like option (beforeSendTransaction or similar) was considered, and if it was, why it wasn't implemented. With regard to that question schemas/breaking changes are irrelevant, because no one would have had any existing code to break.

Does that make sense?

[philipphofmann]

if a separate beforeSend-like option (beforeSendTransaction or similar) was considered, and if it was, why it wasn't implemented. With regard to that question schemas/breaking changes are irrelevant, because no one would have had any existing code to break.

Yep, that makes sense 👍

[philipphofmann]

IMO another con is that users might end up checking the type of the payload and then casting it to different payload types.

[lobsterkatie]

If they did this, would it break downstream processing in the SDK?

[philipphofmann]

I don't think it would, but IMO this is an unappealing API.

[philipphofmann]

I think these are not cons; these are problems to solve. I think having one method called before sending anything makes sense. Alongside adding the universal method, we could also have fine-grained methods. That would be Option 5, then.

Option 5

Add a new universal beforeSend method, deprecate beforeSend, and add fine-grained beforeSend for events and transactions.

• beforeSend deprecated
• beforeSendAll (name can be discussed)
• beforeSendEvent
• beforeSendTransaction

Option 6

Make beforeSend the new universal beforeSend method, deprecate, and add fine-grained beforeSend for events and transactions.

• beforeSend
• beforeSendEvent
• beforeSendTransaction

[philipphofmann]

@philipphofmann @brustolin would that work on iOS?

@marandaneto, on Cocoa we don't use an interface, we use a typedef. So what you pointed out wouldn't work.

/**
 * Block can be used to mutate event before its send.
 * To avoid sending the event altogether, return nil instead.
 */
typedef SentryEvent *_Nullable (^SentryBeforeSendEventCallback)(SentryEvent *_Nonnull event);

[mydea]

I am obviously late to the party, just want to add on a potential downside of option 2, which is that it could potentially break user code if they don't write defensive hooks.

E.g. say a user has a hook like this:

beforeSend(type, event) {
  event.user.email = 'my@email.com';
  return event;
}

And this works currently because all events that can, as of now, be passed in there have/support this shape.
But if in the future a different type of event can be passed in which, for whatever reason, has a different shape, this could break, or have unexpected consequences, I guess?

Obviously ideally users would write this defensively with e.g. switch on a type argument or whatever, but that would be up to user code then. Doesn't mean this is not a good idea, just to keep this potential issue in mind, as it could lead to rather subtle issues (that would also probably not be caught by tests in user-code, especially when using a non-typed language like JS).

[lobsterkatie]

@mydea, I believe what you're talking about is exactly what's covered in the first sentence of the Background section, no?

When transactions were introduced, the decision was made not to run them through beforeSend because they follow a slightly different schema, and therefore had the potential to break any existing beforeSend which relies on its input being a certain shape.

That's why we're talking about this as a change which would ship with the next major of each SDK, precisely because it would be a breaking change.

[lobsterkatie]

Update after IRL meeting:

Many of the points touched on above were discussed, but in the end there wasn't broad consensus on whether to go with a beforeSendTransaction or pipe everything through beforeSend. The idea of lifecycle hooks also came up again, and they are starting to seem more likely to happen, so as an in-between, let's-fix-things-for-now option, we decided that SDKs that want to can implement beforeSendTransaction, even if we do deprecate it later (at least in SDKs which switch to either a universal beforeSend or lifecycle hooks when a new major comes around).

I'm therefore going to close this for now. Once it's clear whether hooks are going to happen, and/or when this comes up again for other events types (profiles, replays), we can revisit the topic.