sentry-dotnet icon indicating copy to clipboard operation
sentry-dotnet copied to clipboard

Logs & Breadcrumbs leaks

Open Vandersteen opened this issue 2 years ago • 5 comments

Please mark the type framework used:

  • [x] ASP.NET Core

Please mark the type of the runtime used:

  • [x] .NET Core
  • Version:

Please mark the NuGet packages used:

  • [x] Sentry.Extensions.Logging
  • [x] Sentry.AspNetCore
  • Version: 3.17.1

Is there a way to start a 'fresh' scope with the Sentry Sdk ?

We use sentry in WorkerServices etc. but logs / breadcrumb do leak over from Startup / other IHostedServices.

Example IHostedService:

public class Worker : BackgroundService
{
    private readonly IHub _hub;

    public Worker(IHub hub)
    {
        _hub = hub;
    }

    public async Task ExecuteAsync(CancellationToken token)
    {
        while (!token.IsCancellationRequested)
        {
            using (var scope = _hub.PushScope())
            {
                await DoSomeWork();
            }
        }
    }
}

Every issue that is reported from this Worker, also contains logs from Startup and other IHostedService (that we do not always have control over)

Is there a way to not 'Push' a scope but start from a 'fresh' scope instead ?

Vandersteen avatar May 19 '22 08:05 Vandersteen

Is this just about the breadcrumbs list having entries from before that scope? Or any other data from the scope isn't relevant? Because this could be a matter of PushScope having some argument to return an empty scope. Or Scope could have a Clear method. Or just the Breadcrumbs on the scope could have a clear method

bruno-garcia avatar Jun 14 '22 20:06 bruno-garcia

Is this just about the breadcrumbs list having entries from before that scope?

Yes

Another example could be the following:

Some Framework code we have no control over

while(true)
{
  var message = FetchNextMessage();

  _logger.LogInformation("Fetched message with id {id}", message.id);
  ProcessMessage(message);
  _logger.LogInformation("Processed message with id {id}", message.id);
}

Where you can implement a 'Processor'

public class MyProcessor : IProcessor
{
  public Task Process(Message message)
  {
     using var scope = _hub.PushScope();
     _logger.LogInformation("...");
   
    //Throw an exception here
    //Or send an event to sentry 
  }
}

The breadcrumbs slowly get filled with the framework code from previous messages (which we can't control) This can quickly lead to thousand / millions of (unrelated) breadcrumbs being sent with events (a leak if you like)

So having something like a _hub.StartScope(); or _hub.NewScope(); or scope.Clear() would be very handy in this use case

Vandersteen avatar Jun 14 '22 21:06 Vandersteen

Clear scope is a fair API and IIRC we expose in some SDKs.

Java has for crumbs: https://github.com/getsentry/sentry-java/blob/a88509c1b7b51bad58053b2457a79dd22fe29f70/sentry/src/main/java/io/sentry/Scope.java#L358-L360

And one to clear everything: https://github.com/getsentry/sentry-java/blob/a88509c1b7b51bad58053b2457a79dd22fe29f70/sentry/src/main/java/io/sentry/Scope.java#L381-L392

bruno-garcia avatar Jun 15 '22 21:06 bruno-garcia

@Vandersteen would you be willing to send a PR to add Clear?

bruno-garcia avatar Jun 15 '22 21:06 bruno-garcia

@bruno-garcia I might be able to free some time, no promises thought. Could you point me in the right direction ?

Vandersteen avatar Aug 30 '22 10:08 Vandersteen

FWIW, it appears that scope.Clear and scope.ClearBreadcrumbs are defined in the Unified API spec, so we should definitely add them.

https://develop.sentry.dev/sdk/unified-api/#scope

mattjohnsonpint avatar Dec 12 '22 20:12 mattjohnsonpint

Also please add SentryEvent.ClearBreadcrumbs() so we can clear breadcrumbs in BeforeSend. (i'm not sure if this is seperate or same)

hheexx avatar Dec 28 '22 05:12 hheexx

Hi. Just for clarity, I've created two new issue to track this work:

  • #2272
  • #2273

Closing this issue in favor of those. Thanks.

mattjohnsonpint avatar Mar 30 '23 21:03 mattjohnsonpint

Scope.Clear has been released in 3.30.0.

See example: in #2274

Also, just to clarify options.IsGlobalMode should be false (which is the default).

mattjohnsonpint avatar Apr 14 '23 17:04 mattjohnsonpint