feat(logs): initial API for 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
XML API docs
Add Tests

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
Batching, via log Envelope Item Payload
- see https://develop.sentry.dev/sdk/telemetry/logs/#log-envelope-item
Integrate into Sentry.Extensions.Logging
- and Sentry.AspNetCore transitively
- and Sentry.Maui transitively
Experimental docs

Early feedback appreciated on

public APIs

see also Question-Comments in the draft

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.

We currently still have a new enum.
Renamed to SentryLogLevel. Matches the sentry-java SDK.
Also moved to Sentry namespace.

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

src/Sentry/SentryLogger.cs

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?

src/Sentry/Infrastructure/DiagnosticId.cs

src/Sentry/Internal/Extensions/JsonExtensions.cs

src/Sentry/SentryLogger.cs

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

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

Looking good so far 👍🏻

…sentry-dotnet into feat/logs-initial-api

samples/Sentry.Samples.Console.Basic/Program.cs

src/Sentry/Extensibility/DisabledHub.cs

src/Sentry/Protocol/SentryLog.cs

bruno-garcia · 2025-05-14T14:31:51Z

src/Sentry/SentryStructuredLogger.cs

+
+    private void CaptureLog(SentryLogLevel level, string template, object[]? parameters, Action<SentryLog>? configureLog)
+    {
+        Debug.Assert(_options is not null);


why not IsEnabled check here? and by pass all the public method checks

I wanted to avoid the CaptureLog method invocation from the public Log{Level} methods if logging is not enabled.
This assert is to let null-flow analysis know that the options are not null here, because we already checked in the calling method.

Changed to Debug.Assert(IsEnabled); to be more explicit about the expected flow.

src/Sentry/SentryStructuredLogger.cs

…delegate exceptions

Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>

Co-Authored-By: Bruno Garcia <bruno-garcia@users.noreply.github.com>

…ework

github-actions · 2025-05-15T18:55:56Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 9a51033

jamescrosswell

Looking great.

The main suggestion I've left is about testability... which I think you'd bump into if you tried to create some unit tests for the new methods that have been added to theHub class.

jamescrosswell · 2025-05-15T22:55:12Z

samples/Sentry.Samples.Console.Basic/Program.cs

@@ -33,6 +33,19 @@

    // This option tells Sentry to capture 100% of traces. You still need to start transactions and spans.
    options.TracesSampleRate = 1.0;
+
+    // This option enables the (experimental) Sentry Logs.
+    options.EnableLogs = true;


If/while these are experimental, should we put them in an options.Experimental.EnableLogs section of the options (to make that unavoidably apparent to SDK users)?

I guess because they have [Experimental(Infrastructure.DiagnosticId.ExperimentalFeature)] it could be OK without it?

jamescrosswell · 2025-05-15T23:02:09Z

src/Sentry/Extensibility/DiagnosticLoggerExtensions.cs

It's almost worth writing a source generator to maintain this file... so much boilerplate. Not for this PR obviously.

jamescrosswell · 2025-05-15T23:08:37Z

src/Sentry/SentryStructuredLogger.cs

+    [Experimental(DiagnosticId.ExperimentalFeature)]
+    public void LogTrace(string template, object[]? parameters = null, Action<SentryLog>? configureLog = null)
+    {
+        if (IsEnabled)


It looks like all of the public methods have this IsEnabled check and then delegate all the work to the CaptureLog method.

We could centralise this logic by putting the check for IsEnabled at the top of the CaptureLog method instead.

Alternatively, if we did add an ISentryStructuredLogger interface, we could have two implementations of that interface: SentryStructuredLogger and DisabledSentryStructuredLogger... the implementation for the later would just be a bunch of no-ops.

One advantage of that approach is that we could mark the dependencies for this implementation as non nullable. I'm not sure if that would simplify things enough to be worth while though (it's a pretty simple class in the first place).

Your call I think.

jamescrosswell · 2025-05-15T23:15:55Z

src/Sentry/IHub.cs

+    /// </list>
+    /// </remarks>
+    [Experimental(Infrastructure.DiagnosticId.ExperimentalFeature)]
+    public SentryStructuredLogger Logger { get; }


Just thinking about testing of the Hub classes in general, the Logger is kind of a dependency of the hub and currently it's not very easy to inject/mock.

Maybe worth creating an ISentryStructuredLogger interface and having this member be of type ISentryStructuredLogger rather than SentryStructuredLogger. A mock Logger could then optionally be injected via the constructor for test purposes.

jamescrosswell · 2025-05-15T23:38:47Z

src/Sentry/SentryLog.cs

+namespace Sentry;
+
+/// <summary>
+/// Represents the Sentry Log protocol.


Is there any reason this isn't in the Sentry.Protocol namespace?

jamescrosswell · 2025-05-15T23:42:48Z

src/Sentry/SentryLog.cs

+    /// <remarks>
+    /// Sent as seconds since the Unix epoch.
+    /// </remarks>
+    [Experimental(DiagnosticId.ExperimentalFeature)]


Possibly overkill decorating every property with this... the class is already decorated with Experimental.

I think it's enough if we put the option to enable this feature under SentryOptions.Experimenta and then decorate any public methods in the API (the stuff on IHub I'm guessing).

Potentially we could decorate public classes like this as well, although I can't see anybody using these classes independently of the experimental APIs they support.

jamescrosswell · 2025-05-15T23:46:49Z

src/Sentry/SentryLogLevel.cs

+/// The severity of the structured log.
+/// <para>This API is experimental and it may change in the future.</para>
+/// </summary>
+/// <seealso href="https://develop.sentry.dev/sdk/telemetry/logs/"/>


Maybe worth adding something to the comments here about the significance of the int values assigned to each of the enum members. I was initially a bit surprised we were assigning values here... only worked out why by reading these docs:

https://develop.sentry.dev/sdk/telemetry/logs/#log-severity-level

jamescrosswell · 2025-05-15T23:54:08Z

test/Sentry.Tests/SentryLogTests.cs

+
+        reader.EndOfStream.Should().BeTrue();
+
+        header.ToIndentedJsonString().Should().Be($$"""


This is the sort of thing Verify is good at testing... might be a good excuse to get familiar with that.

bruno-garcia

just went half the PR this time but leaving some notes for now

bruno-garcia · 2025-05-16T01:25:40Z

samples/Sentry.Samples.Console.Basic/Program.cs

@@ -33,6 +33,19 @@

    // This option tells Sentry to capture 100% of traces. You still need to start transactions and spans.
    options.TracesSampleRate = 1.0;
+
+    // This option enables the (experimental) Sentry Logs.
+    options.EnableLogs = true;


I guess because they have [Experimental(Infrastructure.DiagnosticId.ExperimentalFeature)] it could be OK without it?

bruno-garcia · 2025-05-16T01:26:12Z

samples/Sentry.Samples.Console.Basic/Program.cs

+    options.EnableLogs = true;
+    options.SetBeforeSendLog(static log =>
+    {
+        if (log.TryGetAttribute("suppress", out bool attribute) && attribute)


Suggested change

if (log.TryGetAttribute("suppress", out bool attribute) && attribute)

// A demonstration of how you can drop logs based on some attribute they have

if (log.TryGetAttribute("suppress", out bool attribute) && attribute)

bruno-garcia · 2025-05-16T01:27:14Z

samples/Sentry.Samples.Console.Basic/Program.cs

@@ -88,6 +104,8 @@ async Task ThirdFunction()
        // Simulate doing some work
        await Task.Delay(100);

+        SentrySdk.Logger.LogFatal("Crash imminent!", [], static log => log.SetAttribute("suppress", true));


Odd that we must pass an empty array as a second argument.
There's no way we can set up the overloads in a way that this is not needed?

bruno-garcia · 2025-05-16T01:29:46Z

src/Sentry/Protocol/SentryAttribute.cs

+{
+    internal static void WriteAttribute(Utf8JsonWriter writer, string propertyName, SentryAttribute attribute)
+    {
+        Debug.Assert(attribute.Type is not null);


do we need this assert on all calls?
it's odd to have it here but no on the others
Or only on the method these all fall into: WriteAttributeValue?

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

src/Sentry/Infrastructure/DiagnosticId.cs Outdated Show resolved Hide resolved

jamescrosswell reviewed Apr 30, 2025

View reviewed changes

src/Sentry/Internal/Extensions/JsonExtensions.cs Outdated Show resolved Hide resolved

jamescrosswell reviewed Apr 30, 2025

View reviewed changes

src/Sentry/SentryLogger.cs Outdated Show resolved Hide resolved

jamescrosswell reviewed Apr 30, 2025

View reviewed changes

test/Sentry.DiagnosticSource.IntegrationTests/SqlListenerTests.verify.cs Outdated Show resolved Hide resolved

style(logs): consolidate

d96b092

Flash0ver mentioned this pull request May 5, 2025

build(deps): bump Polyfill from 1.23.0 to 1.32.0 #4167

Merged

Flash0ver and others added 17 commits May 5, 2025 15:33

ref(logs): remove generic WriteSerializable overload

2958a47

ref(logs): consolidate experimental Diagnostic-ID

a63371b

feat(logs): add experimental options

d8d2567

Merge branch 'main' into feat/logs-initial-api

fefd24c

ref(logs): remove custom polyfills now provided through Polyfill

165996a

Format code

32e7e25

Merge branch 'feat/logs-initial-api' of https://github.com/getsentry/…

fc88722

…sentry-dotnet into feat/logs-initial-api

ref(logs): move types out of Experimental namespace

2ba87e4

feat(logs): change 'integer' from Int32 to Int64

0f1d4a4

ref(logs): refine API surface area

8dec5d5

ref(logs): match SeverityLevel to OTel spec

a664f7e

ref(logs): rename SentrySeverity to LogSeverityLevel

96693d0

Format code

83964cf

ref(logs): hide underlying Dictionary`2 for Attributes

dadc69b

ref(logs): restructure attributes

c91cdde

ref(logs): extract TraceId and ParentSpanId methods

0740c3b

ref(logs): remove SentryOptions.LogsSampleRate

eee06bf

bruno-garcia reviewed May 14, 2025

View reviewed changes

Flash0ver added 13 commits May 14, 2025 21:01

fix(logs): incorrectly serializing attributes

dd39fae

fix(logs): do not capture log when template/parameters are invalid

6eb5b9b

fix(logs): do not capture log on user callback exceptions

69c05b8

ref(logs): move new public types to root namespace

430cf82

ref(logs): rework sample

31a8f1f

ref(logs): ensure that DisabledHub dues not capture logs

2ae4476

ref(logs): allow out-of-range Log-Level

69678ce

docs(logs): add XML comments indicating that logs will be ignored on …

97995a8

…delegate exceptions

docs(logs): add to changelog

fbe747d

fix(logs): add to Bindable-Options

64adf33

fix(logs): add to ApiApprovalTests

cdfa901

test(logs): add missing net48 ApiApproval

d2ac53b

test(logs): fix line endings on Windows

4011ba6

Flash0ver mentioned this pull request May 15, 2025

Sentry Structured Logging for .NET #4132

Open

Flash0ver and others added 8 commits May 15, 2025 14:05

Update src/Sentry/Protocol/Envelopes/Envelope.cs

4ae82d0

Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>

Update SentryLog.cs

bc1c465

Co-Authored-By: Bruno Garcia <bruno-garcia@users.noreply.github.com>

test(logs): fix floating-point ToString expectation for .NET Framework

79fb190

ref(logs): remove some using declarations

b4e80f4

test(ci): trying to work around floating-point formatter on .NET Fram…

b21adef

…ework

test(logs): skip failing tests on Mono (non-Windows)

0032858

test(log): fix Skip.If missing SkippableFact

a9769f8

try: fix floating-point formatting on Windows

9a51033

Flash0ver changed the title ~~WIP: Sentry Logs~~ feat(logs): initial API for Sentry Logs May 15, 2025

Flash0ver marked this pull request as ready for review May 15, 2025 18:55

Flash0ver requested a review from aritchie as a code owner May 15, 2025 18:55

jamescrosswell self-requested a review May 15, 2025 22:48

jamescrosswell reviewed May 15, 2025

View reviewed changes

bruno-garcia reviewed May 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(logs): initial API for Sentry Logs #4158

feat(logs): initial API for Sentry Logs #4158

Flash0ver commented Apr 30, 2025 •

edited

Loading

bruno-garcia Apr 30, 2025

Flash0ver May 2, 2025

Flash0ver May 15, 2025

bruno-garcia commented Apr 30, 2025

jamescrosswell commented Apr 30, 2025

jamescrosswell commented Apr 30, 2025

bruno-garcia May 14, 2025

Flash0ver May 14, 2025 •

edited

Loading

github-actions bot commented May 15, 2025

jamescrosswell left a comment

jamescrosswell May 15, 2025

bruno-garcia May 16, 2025

jamescrosswell May 15, 2025

jamescrosswell May 15, 2025

jamescrosswell May 15, 2025

jamescrosswell May 15, 2025

jamescrosswell May 15, 2025

jamescrosswell May 15, 2025

jamescrosswell May 15, 2025

jamescrosswell May 15, 2025

bruno-garcia left a comment

bruno-garcia May 16, 2025

bruno-garcia May 16, 2025

bruno-garcia May 16, 2025

bruno-garcia May 16, 2025


		reader.EndOfStream.Should().BeTrue();

		header.ToIndentedJsonString().Should().Be($$"""

	if (log.TryGetAttribute("suppress", out bool attribute) && attribute)
	// A demonstration of how you can drop logs based on some attribute they have
	if (log.TryGetAttribute("suppress", out bool attribute) && attribute)

feat(logs): initial API for Sentry Logs #4158

Are you sure you want to change the base?

feat(logs): initial API for Sentry Logs #4158

Conversation

Flash0ver commented Apr 30, 2025 • edited Loading

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

jamescrosswell commented Apr 30, 2025

Choose a reason for hiding this comment

Flash0ver May 14, 2025 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented May 15, 2025

jamescrosswell left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruno-garcia left a 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

Flash0ver commented Apr 30, 2025 •

edited

Loading

Flash0ver May 14, 2025 •

edited

Loading