Skip to content

Add Hint support to beforeSend and Event processors #1469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lawrence-laz opened this issue Feb 4, 2022 · 10 comments · Fixed by #2351
Closed

Add Hint support to beforeSend and Event processors #1469

lawrence-laz opened this issue Feb 4, 2022 · 10 comments · Fixed by #2351
Assignees
Labels
Feature New feature or request

Comments

@lawrence-laz
Copy link

lawrence-laz commented Feb 4, 2022

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 lawrence-laz changed the title Allow adding attachment in BeforeSend callback Allow adding attachments in BeforeSend callback Feb 4, 2022
@lucas-zimerman lucas-zimerman added Attachments Feature New feature or request labels Feb 7, 2022
@bruno-garcia bruno-garcia moved this to Needs Discussion in [DEPRECATED] Mobile SDKs Feb 16, 2022
@bruno-garcia
Copy link
Member

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>: getsentry/sentry-java#1929

@pistoleta
Copy link

It would be great to have this

@bruno-garcia
Copy link
Member

Blocks getsentry/sentry-unity#492

@SimonCropp
Copy link
Contributor

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

@vaind vaind changed the title Allow adding attachments in BeforeSend callback Add Hint support to beforeSend and Event processors May 24, 2022
@mattjohnsonpint
Copy link
Contributor

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
Copy link
Contributor

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 mattjohnsonpint moved this from In Progress to Backlog in [DEPRECATED] Mobile SDKs Apr 22, 2023
@mattjohnsonpint
Copy link
Contributor

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.

@jamescrosswell
Copy link
Collaborator

jamescrosswell commented May 1, 2023

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

@mattjohnsonpint
Copy link
Contributor

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.

@jamescrosswell jamescrosswell linked a pull request May 2, 2023 that will close this issue
@mattjohnsonpint mattjohnsonpint moved this from Backlog to In Progress in [DEPRECATED] Mobile SDKs May 10, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in [DEPRECATED] Mobile SDKs May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature or request
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

10 participants