WIP: Sentry Logs #4158

Flash0ver · 2025-04-30T16:09:31Z

Draft PR: Work in progress

Info

Developer Documentation
See also: Sentry Structured Logging for .NET #4132

TODO

Complete public API surface area
Batching, via log Envelope Item Payload
- see https://develop.sentry.dev/sdk/telemetry/logs/#log-envelope-item

Follow-ups

Add non object[] but generic <TArg> overloads per Log-Severity method for the most common scenarios
- to avoid both boxing allocations and array allocations in case options.EnableLogs is false
- our public IDiagnosticLogger.Log{Level}() extension methods have overloads with up to 3 generic type parameters
- .NET's String.Format has 1-3 object arguments
  - and 1-3 generic arguments since net8.0 when used with CompositeFormat
Consider a custom [InterpolatedStringHandler] to support interpolated strings and ensure zero-alloc if options.EnableLogs is false
- available for TFMs compatible with net6.0

Early feedback appreciated on

public APIs

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Sentry Logs ([#4158](https://github.com/getsentry/sentry-dotnet/pull/4158))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against db7d558

bruno-garcia · 2025-04-30T17:21:52Z

src/Sentry/Experimental/SentrySeverity.cs

+namespace Sentry.Experimental;
+
+//TODO: QUESTION: not sure about the name
+// this is a bit different to Sentry.SentryLevel and Sentry.BreadcrumbLevel


Does it need to be different? I wonder if we can use SentryLevel instead?
Because we don't have Trace there? :~

We could add it as -1 😅

No, not necessarily.
And adding it as -1 would not break any existing behavior - but may warrant some documentation - like:

sentry-dotnet/src/Sentry.Extensions.Logging/LogLevelExtensions.cs

Line 21 in 8978bc4

public static LogLevel ToMicrosoft(this SentryLevel level)

But reusing Sentry.SentryLevel could be confusing:

sentry-dotnet/src/Sentry/SentryLevel.cs

Line 6 in 8978bc4

public enum SentryLevel : short

uses the EnumMemberAttribute, with e.g. [EnumMember(Value = "warning")] Warning,. But the OTEL SeverityText field and our Protocol expects "warn" instead. Well, we would still have the separate ToSentryProtocolLevel (or similar) extension method that does the right thing, but the re-use may cause confusion.

Also, this would have a side effect onto existing usages, viaSentryOptions.DiagnosticLevel:

sentry-dotnet/src/Sentry/Infrastructure/DiagnosticLogger.cs

Line 8 in 8978bc4

public abstract class DiagnosticLogger : IDiagnosticLogger

Do we want to extend the "Log-Level" for existing internal/diagnostic logging?

On the other hand, adding yet another "Log-Level" enum, that is similar, but not quite the same, is similarly confusing.

And now I am thinking if we could get away with making this new enum type internal instead, since we do have separate Log{Level}() methods, and application code could still change the severity via the int-based SeverityNumber through the BeforeSendLog-callback. (see Log Severity Number)
To, for now, reduce the public surface area, and we could think about which approach to chose when/if the issue arises later.

bruno-garcia · 2025-04-30T17:23:21Z

src/Sentry/Experimental/System.Diagnostics.CodeAnalysis.ExperimentalAttribute.cs

+                AttributeTargets.Event |
+                AttributeTargets.Interface |
+                AttributeTargets.Delegate, Inherited = false)]
+internal sealed class ExperimentalAttribute : Attribute


@SimonCropp has a shimming library that might have this, no?

It is indeed!
I only checked the README where I didn't find it, but since v1.32.0 it is included (I'll PR Polyfill's docs later).

Now I got two questions:

Should we update to v1.32.0, or the latest version of Polyfill (v7.27.0).

Depending on the version we chose: should we update this in a separate PR?

e.g. v1.34.0 adds the System.Runtime.CompilerServices.DefaultInterpolatedStringHandler which the compiler then chooses for interpolated strings

this should have a positive effect on performance when using interpolated strings in our codebase targeting TFMs less than net6.0

bruno-garcia · 2025-04-30T17:25:10Z

src/Sentry/SentryLogger.cs

+{
+    //TODO: QUESTION: Trace vs LogTrace
+    // Trace() is from the Sentry Logs feature specs. LogTrace() would be more .NET idiomatic
+    public void Trace(string template, object[]? parameters = null, Action<SentryLog>? configureLog = null)


Hm, could we have overloads with T instead to avoid boxing cost when logging is disabled?

That's the approach taken on the diagnostic logger extensions

Yes, it's also on my bucket list ... I updated the description to reflect required work, too.
I'd not only like to add up to 3 TArg overloads per severity, but also - in a follow-up PR - add a custom [InterpolatedStringHandler], which I'm going to explore over the weekend.

src/Sentry/SentryLogger.cs

bruno-garcia · 2025-04-30T17:26:17Z

src/Sentry/SentryLogger.cs

+    //      but we could have an Analyzer for that, indicating that Sentry does not support other types if used in the interpolated string
+    //  or: utilize a SourceGen, similar to the Microsoft.Extensions.Logging [LoggerMessage]
+    //      with which we could enforce on compile-time to only support string, boolean, integer, double
+    private static void CaptureLog(SentrySeverity level, string template, object[]? parameters, Action<SentryLog>? configureLog)


Are we going to do batching here?

CaptureLogs as well?

The spec allows for multiple envelope items... which would be more efficient in apps that are emitting loads of logs. We'd need to build some fancy logic to queue/flush logs in that case. Probably worth it from a performance point of view though since logs are the kind of thing people might send oodles of.

Yes, batching is amongst my TODOs ... I updated the description with open work during the draft status.

bruno-garcia · 2025-04-30T17:26:37Z

Exciting first step in our logging story :)

jamescrosswell · 2025-04-30T21:28:52Z

@Flash0ver can we reference some developer docs and/or an issue in the PR description to help reviewers get an overview of the change before looking at code?

jamescrosswell · 2025-04-30T21:38:02Z

src/Sentry/Infrastructure/DiagnosticId.cs

@@ -8,4 +8,13 @@ internal static class DiagnosticId
    /// </summary>
    internal const string ExperimentalFeature = "SENTRY0001";
 #endif
+
+    //TODO: QUESTION: Should we re-use the above for all [Experimental] features or have one ID per experimental feature?
+    internal const string ExperimentalSentryLogs = "SENTRY0002";


I think it's OK to just have one... the main purpose here is to make sure people are aware/deliberate about opting into our experimental APIs. They'll get a separate warning in their code each time they use one/any of them.

Potentially they could be disabling those in a directory.build.props file for their solution... in which case separate diagnostic ids would mean they'd need to disable warnings for our Heap Dumps feature and the Logs feature separately... seems like overkill.

SGTM
I am now thinking about adding a single UrlFormat string, used by both/all experimental features, linking to a dedicated yet small "Experimental Features" page in our docs. But will follow up with that in another PR.

jamescrosswell · 2025-04-30T21:41:26Z

src/Sentry/Internal/Extensions/JsonExtensions.cs

@@ -472,6 +472,17 @@ public static void WriteSerializable(
        writer.WriteSerializableValue(value, logger);
    }

+    public static void WriteSerializable<TValue>(


How is this different to the overload defined on line 465? The only difference I can see is the struct constraint in the where clause (but is this important or used for anything)?

The overload above

sentry-dotnet/src/Sentry/Internal/Extensions/JsonExtensions.cs

Lines 465 to 469 in 8978bc4

public static void WriteSerializable(

this Utf8JsonWriter writer,

string propertyName,

ISentryJsonSerializable value,

IDiagnosticLogger? logger)

takes an ISentryJsonSerializable, and is boxing struct-based ISentryJsonSerializable to the interface type.

My original motivation was to add a new overload that avoids boxing for struct-based ISentryJsonSerializable to not binary-break existing call sites, but just realize that we have 11 existing call sites binding to the new overload.

IIRC we're not too concerned about binary compatibility, but definitely about source compat, since user's usually only consume one Sentry-package, or at least keep the Sentry-Package-Versions in sync, so I'm thinking about changing the existing WriteSerializable to be generic.

Also, I just realize that the only usages of the method is within the Sentry assembly, so there are not even binary breaking concerns at all.

TL;DR
I'd like to change the existing WriteSerializable to be generic in order to avoid boxing allocations for struct-based ISentryJsonSerializable.
But now I believe I changed my mind and I'd like to propose this change in a separate PR, to not mix too many concerns in a single changeset.

jamescrosswell · 2025-04-30T21:49:33Z

src/Sentry/SentryLogger.cs

+        CaptureLog(SentrySeverity.Trace, template, parameters, configureLog);
+    }
+
+    //TODO: QUESTION: parameter name "template" vs "format"


I feel like it's less mental gymnastics for us if we go with the sentry naming conventions (when we're reading code from other eepos etc. to see how they make changes that we need to mimic). So I'd probably go with template personally.

jamescrosswell · 2025-04-30T22:09:14Z

test/Sentry.DiagnosticSource.IntegrationTests/SqlListenerTests.verify.cs

@@ -155,7 +155,7 @@ public void ShouldIgnoreAllErrorAndExceptionIds()
        foreach (var field in eventIds)
        {
            var eventId = (EventId)field.GetValue(null)!;
-            var isEfExceptionMessage = SentryLogger.IsEfExceptionMessage(eventId);
+            var isEfExceptionMessage = Sentry.Extensions.Logging.SentryLogger.IsEfExceptionMessage(eventId);


Hm, we've got two types called SentryLogger now? Sentry.SentryLogger and also Sentry.Extensions.Logging.SentryLogger? I can see that being a pain over time. I reckon we rename one of them.

Sentry.Extensions.Logging.SentryLogger is internal sealed so no issues renaming that if we have to. SentryMelLogger maybe?

More painful would be things like SentryLoggingOptions, since that's public. If we need some dedicated options for these new features, we could call them SentryStructuredLoggingOptions maybe... In which case possibly we call the new logger the SentryStructuredLogger (for consistency)?

Are all of these public? Sentry.Extensions.Logging.SentryLogger

sentry-dotnet/src/Sentry.Extensions.Logging/SentryLogger.cs

Line 6 in f765f99

internal sealed class SentryLogger : ILogger

This is internal so less of a concern

jamescrosswell · 2025-04-30T22:13:34Z

Looking good so far 👍🏻

Flash0ver added 2 commits April 30, 2025 13:09

feat(logs): initial experiment

0add668

feat(logs): basic Logger Module API shape

db7d558

Flash0ver self-assigned this Apr 30, 2025

bruno-garcia reviewed Apr 30, 2025

View reviewed changes

jamescrosswell reviewed Apr 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Sentry Logs #4158

WIP: Sentry Logs #4158

Flash0ver commented Apr 30, 2025 •

edited

Loading

github-actions bot commented Apr 30, 2025

bruno-garcia Apr 30, 2025

Flash0ver May 2, 2025

bruno-garcia Apr 30, 2025

Flash0ver May 2, 2025

bruno-garcia Apr 30, 2025

Flash0ver May 2, 2025

bruno-garcia Apr 30, 2025

jamescrosswell Apr 30, 2025

Flash0ver May 2, 2025

bruno-garcia commented Apr 30, 2025

jamescrosswell commented Apr 30, 2025

jamescrosswell Apr 30, 2025

Flash0ver May 2, 2025 •

edited

Loading

jamescrosswell Apr 30, 2025

Flash0ver May 2, 2025 •

edited

Loading

jamescrosswell Apr 30, 2025

jamescrosswell Apr 30, 2025 •

edited

Loading

bruno-garcia May 5, 2025

jamescrosswell commented Apr 30, 2025

	public static void WriteSerializable(
	this Utf8JsonWriter writer,
	string propertyName,
	ISentryJsonSerializable value,
	IDiagnosticLogger? logger)

WIP: Sentry Logs #4158

Are you sure you want to change the base?

WIP: Sentry Logs #4158

Conversation

Flash0ver commented Apr 30, 2025 • edited Loading

github-actions bot commented Apr 30, 2025

Instructions and example for changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruno-garcia commented Apr 30, 2025

jamescrosswell commented Apr 30, 2025

Choose a reason for hiding this comment

Flash0ver May 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flash0ver May 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamescrosswell Apr 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamescrosswell commented Apr 30, 2025

Flash0ver commented Apr 30, 2025 •

edited

Loading

Flash0ver May 2, 2025 •

edited

Loading

Flash0ver May 2, 2025 •

edited

Loading

jamescrosswell Apr 30, 2025 •

edited

Loading