Add Serilog log's properties as Breadcrumb data

[ZivFarin]

This line shows that a serilog log's properties are set as 'extra' for the event, but once I log an event and add properties, it is nowhere to be found.
This does not fit so much with the behavior in other sinks I have seen so far.

Is there a reason for this?
Perhaps a function in the style of ExtractBreadCrumbDataFromLogEvent below would be better?

class SentrySink : ILogEventSink
{
    public void Emit(LogEvent logEvent)
    {
        Dictionary<string, string> breadcrumbData = ExtractBreadCrumbDataFromLogEvent(logEvent);


        SentrySdk.AddBreadcrumb(
            message: logEvent.RenderMessage(),
            category: "Log",
            type: "Default",
            data: breadcrumbData,
            level: MapLevel(logEvent.Level)
        );
    }

    private static Dictionary<string, string> ExtractBreadCrumbDataFromLogEvent(LogEvent logEvent)
    {
        var breadcrumbData = new Dictionary<string, string>();


        if (logEvent.Exception is not null)
            breadcrumbData["Exception"] = logEvent.Exception.ToString().Trim('"');

        foreach (var prop in logEvent.Properties)
            breadcrumbData[prop.Key] = prop.Value.ToString().Trim('"');


        return breadcrumbData;
    }

    private static BreadcrumbLevel MapLevel(LogEventLevel level) =>
        level switch
        {
            LogEventLevel.Verbose => BreadcrumbLevel.Debug,
            LogEventLevel.Debug => BreadcrumbLevel.Debug,
            LogEventLevel.Information => BreadcrumbLevel.Info,
            LogEventLevel.Warning => BreadcrumbLevel.Warning,
            LogEventLevel.Error => BreadcrumbLevel.Error,
            LogEventLevel.Fatal => BreadcrumbLevel.Critical,
            _ => BreadcrumbLevel.Info
        };
}

[bruno-garcia]

The breadcrumbs data is written here:

sentry-dotnet/src/Sentry.Serilog/SentrySink.cs

Lines 135 to 138 in 1bf3762

	data = new Dictionary<string, string>
	{
	{"exception_message", exception.Message}
	};

It's true we don't get the serilog contexts in there yet.
But the context should show up on Extra. It's read further up on that same file:

sentry-dotnet/src/Sentry.Serilog/SentrySink.cs

Line 114 in 1bf3762

evt.SetExtras(GetLoggingEventProperties(logEvent));

@Flash0ver verified the Extra bit works right now, could you share a repro where it doesn't work for you?

Regarding the serilog context on breadcrumb, would you be willing to contribute a PR for that?

[ZivFarin]

Just saw I've missused 'context'.
I meant 'log properties'. Edited my messages to fit that.

I saw the log's properties are being added to the event's "extra", but when properties are added to a log - they are usually attached to that specific log (in Sentry's case - a specific Breadcrumb).

If this is what you've intended -
Then no problem, I'll create a PR which "rewires" GetLoggingEventProperties to the breadcrumbs data.
Note that doing so might make attaching the log's properties to the event obsolete.
Would like me to remove that while I'm at it?

If we're on the same page: Can you refer me to your contribution standards?

[jamescrosswell]

I saw the log's properties are being added to the event's "extra", but when properties are added to a log - they are usually attached to that specific log (in Sentry's case - a specific Breadcrumb).

If this is what you've intended - Then no problem, I'll create a PR which "rewires" GetLoggingEventProperties to the breadcrumbs data.

Breadcrumbs have a Data property but this is a IReadOnlyDictionary<string, string>?. Events (which typically means errors) have an Extra property which is of type IReadOnlyDictionary<string, object?>... so there's no easy way to store the result of GetLoggingEventProperties on Breadcrumbs in the same way that they get stored on Events.

Note that doing so might make attaching the log's properties to the event obsolete.
Would like me to remove that while I'm at it?

Either the log gets captured as an Event (aka error) if (logEvent.Level >= _options.MinimumEventLevel) or it gets captured as a Breadcrumb... so the existing code storing serilog properties on the event should definitely be retained.