search

getsentry / sentry-dotnet

Sentry SDK for .NET
https://docs.sentry.io/platforms/dotnet
MIT License
590 stars 206 forks source link

Don't pass call from ILogger.BeginScope to SentrySdk.PushScope #843

Open bruno-garcia opened 3 years ago

bruno-garcia commented 3 years ago

We've added a nice API to add data to an event through the instance of the exception.

To extend that to add attachments, we have the problem that attachments live on the Scope. And the code pulling data from the Exception only has SentryEvent, and no scope.

The API such as WithScope doesn't work properly on ASP.NET Core due to "scope lock" which was a work around to keep breadcrumbs around even though the ILogger interface BeginScope mapped to the SDK PushScope. To avoid dropping these breadcrumbs. Ideally we'd simply not map BeingScope to PushScope and simply abandon "scope lock". The negative side effect here is that adding an attachment with WithScope with a locked scope actually means the attachment never goes away, and each new call adds yet another attachment.

The unified API also includes overloads to the public API to pass a local scope, or mutate a clone. Such as:

Sentry.CaptureException(exception, s => s.SetTag() which isn't in .NET yet since lots of use cases in .NET are done through the logger integration anyway: BeginScope and structured logging.

Tyrrrz commented 3 years ago

Idea: use weakref to track the scope at the time the exception is thrown, and associate them later.

CteixeiraPW commented 2 years ago

Hello @bruno-garcia Do you guys have an idea when this enhancement will be released?

We have some standards in my organization about when we must create an entry to the logs. For us is required to create a new log entry in every write operation to the DB. After using "WithScope" to include some data/tags and create a new log entry about this operation, the following log entries include the same data (we don't want that to happen). We want to use sentry to replace our internal logger (an internal app) and our exception handler for over 100 applications (WebForms, ASP .NET 5, React). So far this issue is what is keeping us waiting to move forward with you product.

bruno-garcia commented 2 years ago

I think the simplest solution for this is to remove the call from ILogger.BeginScope to SentrySdk.PushScope. And remove the ScopeLock thing after that.

Possibly this could be done and shipped as part of the version 4.0.0 of the SDK. Combined with other changes. @SimonCropp thoughts?

CteixeiraPW commented 2 years ago

@Bruno-garcia do you have an estimated date for the release of version 4.x?

bruno-garcia commented 2 years ago

@CteixeiraPW We don't have a ETA yet. Is this something you're willing to send a PR?

bruno-garcia commented 2 years ago

Relates to #675

mattjohnsonpint commented 2 years ago

Actually, related to #1492

jamescrosswell commented 2 months ago

Do we want to do this?

It would be good to get some clarity around what we want the SDK to do.

  1. On the one hand we have this issue where we're discussing removing the code that passes Logger to Sentry scope for MEL.
  2. On the other hand, we've got https://github.com/getsentry/sentry-dotnet/issues/3544 where we're discussing extending this functionality so that it also works in Serilog

If we want to do this, what are we talking about?

We've added a nice API to add data to an event through the instance of the exception.

To extend that to add attachments, we have the problem that attachments live on the Scope. And the code pulling data from the Exception only has SentryEvent, and no scope.

It's worth noting that the only place where exception processors get applied is here, and the scope is available there, so we could easily pass it as an additional parameter to the Process event... that would be a breaking change (to the interface) but we could do it in a major release: https://github.com/getsentry/sentry-dotnet/blob/5b35d8ea901225019929ff93bf7b24720f604d95/src/Sentry/SentryClient.cs#L302-L309

What would we like a [potential] new API to look like? Is is something like exception.AddAttachment(/*...*/)?

The problem with that is presumably that if the scope is locked then we can't push and pop a scope to temporarily store the attachment on the scope while capturing the event?

No longer relevant?

The API such as WithScope doesn't work properly on ASP.NET Core due to "scope lock" which was a work around to keep breadcrumbs around even though the ILogger interface BeginScope mapped to the SDK PushScope.

The API no longer appears to include a WithScope method. We can still manually push and pop scopes though.

The unified API also includes overloads to the public API to pass a local scope, or mutate a clone. Such as:

Sentry.CaptureException(exception, s => s.SetTag() which isn't in .NET yet since lots of use cases in .NET are done through the logger integration anyway: BeginScope and structured logging.

The API does now have support for a configure scope callback https://github.com/getsentry/sentry-dotnet/blob/76e9f660296dd23ff5d1f63e8c3f55e9815c8e6d/src/Sentry/SentrySdk.cs#L458-L459

bruno-garcia commented 1 month ago

@jamescrosswell we can keep calling into the Sentry SDK to set the scope data, we just sholdn't call PushScope in Sentry.

We want the data they pass in, but we don't want scope isolation. There might be some side effect to this (possibly a leak in case we're just appending stuff? We have a ring buffer for breadcrumbs but other data are just maps for example)

bruno-garcia commented 1 month ago

Note that this ticket rquests the behavior that we have on ASP.NET Core already.

One idea that came up in a chat: BeginScope could then create a breadcrumb. The data passed as argument to BeginScope can be passed to the breadcrumb Data bag. So we can see clearly when scope was created or disposed, and the values added to it.

In this case we could pop the scope dropping all other data, but the breadcrumb would live in the parent scope