sentry-dotnet icon indicating copy to clipboard operation
sentry-dotnet copied to clipboard

Add `Hint` support to beforeSend and Event processors

Open lawrence-laz opened this issue 2 years ago • 6 comments

Hi, Sentry is great! I am however missing a way to add an attachment to an outgoing event in the BeforeSend callback. For example:

using var host = Host.CreateDefaultBuilder()
    .ConfigureLogging((context, builder) =>
    {
        builder.AddSentry(options =>
        {
            options.BeforeSend = sentryEvent =>
            {
                sentryEvent.AddBreadcrumb("Hello"); // I can do this
                sentryEvent.AddAttachment("Foo.txt"); // But I cannot do this
                return sentryEvent;
            };
        };
    })
    .Build();

Am I missing something, or this is a missing feature?

lawrence-laz avatar Feb 04 '22 14:02 lawrence-laz

One solution to this is adding Hint which is part of the unified API spec: https://develop.sentry.dev/sdk/unified-api/#hints Recently the Java SDK for hint as Map<string,object>: https://github.com/getsentry/sentry-java/pull/1929

bruno-garcia avatar Mar 10 '22 20:03 bruno-garcia

It would be great to have this

pistoleta avatar Mar 28 '22 17:03 pistoleta

Blocks https://github.com/getsentry/sentry-unity/issues/492

bruno-garcia avatar Mar 30 '22 21:03 bruno-garcia

@bruno-garcia can u share an example how u expect the api usage to look if we use the hint approach

SimonCropp avatar May 06 '22 06:05 SimonCropp

Some user-doc info on Hint: https://docs.sentry.io/platforms/java/configuration/filtering/

How it's used to add screenshot attachments in sentry-java: https://github.com/getsentry/sentry-java/blob/6270232e48719ccbba243a02a6107413fcdbd6df/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java#L95

And also how users can remove the screenshot in BeforeSend: https://github.com/getsentry/sentry-java/pull/2046/files#diff-f2e9a27fd40ccf0ccec3a5f45afc8d2375d5d72826bd39f38bb4c3f58b988603R1875

vaind avatar May 24 '22 14:05 vaind

Note, this also applies to BeforeBreadcrumb, and our docs already (incorrectly) state that hints are supported.

https://docs.sentry.io/platforms/dotnet/configuration/options/#before-breadcrumb

mattjohnsonpint avatar Jun 05 '22 02:06 mattjohnsonpint

Because we've currently implemented BeforeSend as a property that accepts a delegate function, there's no clean way to add a Hint parameter to the function without making a breaking change that would require a major version bump. Methods are overloadable, but properties are not.

I'd like to handle this as follows:

public class SentryOptions
{
    // ...

    internal Func<SentryEvent, Hint, SentryEvent?>? _beforeSend;

    public void SetBeforeSend(Func<SentryEvent, SentryEvent?> beforeSend)
    {
        _beforeSend = (e, _) => beforeSend(e);
    }

    public void SetBeforeSend(Func<SentryEvent, Hint, SentryEvent?> beforeSend)
    {
        _beforeSend = beforeSend;
    }

    [Obsolete("This property will be removed in a future version. Use SetBeforeSend instead.")]
    public Func<SentryEvent, SentryEvent?>? BeforeSend
    {
        get => null;
        set => _beforeSend = value is null ? null : (e, _) => value(e);
    }

    // ...
}

The usage would then change from:

options.BeforeSend = @event => { ... };

To the following:

options.SetBeforeSend(@event => { ... });

... or when hints are desired:

options.SetBeforeSend((@event, hint) => { ... });

mattjohnsonpint avatar Apr 03 '23 23:04 mattjohnsonpint

FYI - Hints should also be used for purposes of providing additional context to the BeforeSend (and similar) methods that are not going to be part of the sent event at all. For example, #2217 could add the HTTP request and response of a failed HTTP request as hints. Java does it like this.

mattjohnsonpint avatar Apr 22 '23 04:04 mattjohnsonpint

So looking into this at the moment. It looks mostly straight forward... the bits that are still foggy are how/why the Sentry.NET SDK interacts with the Java SDK on Android.

In Sentry.Android.Callbacks.BeforeSendCallback I see the following method:

    public JavaSdk.SentryEvent? Execute(JavaSdk.SentryEvent e, JavaSdk.Hint h)
    {
        // Note: Hint is unused due to:
        // https://github.com/getsentry/sentry-dotnet/issues/1469

        var evnt = e.ToSentryEvent(_javaOptions);
        var result = _beforeSend.Invoke(evnt);
        return result?.ToJavaSentryEvent(_options, _javaOptions);
    }

So it takes a Java SDK event and hint, should eventually convert both of these to Sentry .NET equivalents, invoke the .NET implementation of _beforeSend and then convert the resulting event back to a Java SDK equivalent to be returned.

Why do we do this? As I understand it, the Java SDK has already implemented hints and already has its own implementation of BeforeSend. What's the scenario in which events and hints that get captured in Java code need to be handled by a .NET implementation of a BeforeSend callback prior to being eventually sent to Sentry.io (seemingly by the Java SDK)?

jamescrosswell avatar May 01 '23 08:05 jamescrosswell

That code is related to .NET Android (e.g. MAUI), which we bundle the native Sentry Android SDK. It applies when there's a native crash in Java code that would otherwise be unable to be handled in .NET. The callback you highlighted is so that a .NET developer can write a BeforeSend implementation once that applies both to events coming from .NET and to these native Java events. The same applies for Cocoa on iOS.

For now, please focus on the .NET side only. We can leave this mobile parts unimplemented and create a new work item for bridging hints to BeforeSend on Android and Cocoa events later. Thanks.

mattjohnsonpint avatar May 01 '23 16:05 mattjohnsonpint