getsentry / sentry-dotnet

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

Logs & Breadcrumbs leaks #1660

Closed Vandersteen closed 1 year ago

Vandersteen commented 2 years ago

Please mark the type framework used:

Please mark the type of the runtime used:

Please mark the NuGet packages used:

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 ?

bruno-garcia commented 2 years ago

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

Vandersteen commented 2 years ago

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

bruno-garcia commented 2 years ago

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 commented 2 years ago

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

Vandersteen commented 2 years ago

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

mattjohnsonpint commented 1 year ago

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

hheexx commented 1 year ago

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

mattjohnsonpint commented 1 year ago

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

Closing this issue in favor of those. Thanks.

mattjohnsonpint commented 1 year ago

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).