Adding a breadcrumbCallback for filtering and cleaning

[elicwhite]

Adding a way to filter breadcrumbs. This enables filtered breadcrumbs to not count against the breadcrumb limit.

Fixes #672 and #579

Based on the conversation in #579, I considered making this callback allow modification of the breadcrumb blob if it returns an object, and filtering if it returns false. I decided against that because I would prefer to keep the data modification in one place (dataCallback) instead of being spread over many functions.

I believe I followed the contributing guidelines, let me know if I missed something.

[benvinegar]

@TheSavior – thanks for the PR.

My only concern is this: we will now have dataCallback, shouldSendCallback, shouldSendBreadcrumbCallback, and potentially also breadcrumbCallback. I don't like that we have "should send" variants of callbacks when one callback could do both a) data mutation and b) data "cancellation". This bloats both the API and Raven's filesize.

My preference, I think – give me more time on this – is that we just call this breadcrumbCallback and move to make dataCallback work the same way in a future major version (e.g. 4.0).

Thoughts?

cc @LewisJEllis @MaxBittker @mattrobenolt

[LewisJEllis]

I definitely agree that we want to avoid ending up with four or five or six different callbacks to worry about. Talked to Ben about this and I think I agree that we can simplify the whole picture to just dataCallback and breadcrumbCallback in a reasonable way that doesn't nuke backwards-compatibility.

If we keep dataCallback's behavior mostly as-is but make it so an explicit false return value cancels sending (filters the exception), we can eliminate shouldSendCallback, but let's not worry about that change here. @TheSavior let's change shouldSendBreadcrumbCallback to just breadcrumbCallback. If breadcrumbCallback returns false filter it, otherwise breadcrumbCallback can mutate the breadcrumb object in-place or return a new value for it (sort of like dataCallback). This will help keep the number of callback hooks down.

[elicwhite]

So I was thinking about this stuff too when I was writing this PR. I actually switched between a few different approaches when thinking about what I would want if I was the maintainer of the project.

I originally started with breadcrumbCallback but decided that if I was maintaining this project, I wouldn't want to have an API where you could do the same thing in multiple places. Any cleaning of data that can be done in the breadcrumbCallback could be done in the dataCallback by looping over the breadcrumbs.

In that way, it feels like a callback to manage the breadcrumbs is a bit different. That's how I ended up landing on following the shouldSend approach that just lets the user filter.

I'm happy to switch the approach back to filter/mutate, I just want to make sure we get the reasoning right.

[benvinegar]

@TheSavior – my argument of having a proper breadcrumbCallback is that someone could augment the breadcrumb based on their current application state. That's something they couldn't do if the mutation was done "postmortem" once dataCallback hits.

For example, we have a pretty popular Redux plugin that captures Redux actions as breadcrumb events. A developer could use breadcrumbCallback to augment the breadcrumb with more detail drawn from their Redux application state.

Basically, I see some possibilities for usefulness, people will probably ask for it at some point, and given that it's really not really much extra trouble might as well.

[elicwhite]

That's a use I hadn't considered here.

However, does dataCallback not run in the same event loop as when it is fired? I've seen posts of people who put their application state into the extra interface in dataCallback.

[benvinegar]

Yeah, it does. But my argument is that, by that point – minutes or even (super rare edge case) hours after the breadcrumb fired – the application state has changed. What I'm saying is, at the point at which the breadcrumb is captured, mutate it.

(Presumably they wouldn't dump their entire app state into the breadcrumb, they can't because breadcrumb entries are particularly memory constrained, but they could pull some specific valuable pieces out.)

[elicwhite]

Got it, right. Breadcrumbs fire on actions and build up until an exception occurs. Obviously the dataCallback is called when the exception occurs, possibly well after the breadcrumb was captured. Okay, I understand what you are saying.

I'll switch the PR to breadcrumbCallback.

[benvinegar]

One last thing needs to be addressed, but all things considered this is a terrific contribution and we appreciate it.

[benvinegar]

Need to check for explicit false here. We don't want the case where a no-op callback returns undefined and so we throw away the payload.

[elicwhite]

What do we expect returning a falsey, non false value to do? Does it stick the falsey value into the breadcrumbs array? Would you expect a falsey value to use the original crumb that was passed into the callback?

I could also imagine throwing if we don't get an object or false since it means the callback isn't built correctly. However, throwing in raven is obviously problematic since that is what tracks exceptions. Throwing in the breadcrumb handler though seems theoretically more okay than other places.

[LewisJEllis]

Yes, it should just keep the original crumb value in that case. This is roughly consistent with dataCallback's behavior (see here), aside from the explicit false return case. The user can either return false to filter, return their new breadcrumb object value, or just modify it in-place and return nothing.

[elicwhite]

Perhaps another related question is that if we don't want to throw, that undefined would cause it to use the original data object. What should happen if the callback returns a string? What does dataCallback do?

[elicwhite]

dataCallback looks like it gets used like this:

        if (isFunction(globalOptions.dataCallback)) {
            data = globalOptions.dataCallback(data) || data;
        }

        // Why??????????
        if (!data || isEmptyObject(data)) {
            return;
        }

which means that returning a falsey value uses the original data object. If a string or other malformed object is returned, it will send that to sentry.

Is this the behavior we want for the breadcrumbCallback?

[elicwhite]

Great, I'll update.

[LewisJEllis]

Just to be clear, there's also an implicit third case to Ben's comment which is "if this is neither an object nor false/null, then just use the original crumb value".

[benvinegar]

Yes^

[LewisJEllis]

One other note since we brought null into the picture - since typeof null is object, we should be careful to do the "is it false/null" case first, so that in the second case when you do something like if typeof result === 'object', we won't mistakenly treat null as an object.

Edit: nevermind, the isObject helper takes care of this.

[elicwhite]

To be consistent with dataCallback, I'm also using the original value if the callback returns an empty object.

[benvinegar]

"replace" is a better descriptor than "mutate" – you can mutate the input crumb and not return anything, different from replacing the value

[elicwhite]

The test below this is for replacing, this test probably shouldn't return in the callback to actually test mutation.

[benvinegar]

@TheSavior – Thanks again. I'm going to release 3.9.0 with this today.