Grouping for Errors

[philipphofmann]

@nburyak recently opened an issue #901, that Sentry groups errors with the same domain but different error code together. So MyDomain 1 and MyDomain 2 end up in the same issue no matter the stacktraces, because grouping groups by message instead. This brought up a discussion if they shouldn't be two individual issues instead.

The grouping algorithm doesn't consider errors with the same domain but different code as a reason to split up issues into different groups. The error code can stand for a different type of error on Apple even if the domain is the same. Therefore it makes sense to split up errors with the same domain but different code.

A way to achieve this manually is to use SDK Fingerprinting. Our docs even mention such a use case, but we didn't add the code sample for Apple yet. For sure, we should the code sample.

We could add an option, which is disabled by default as such a change would have a big impact on grouping, to set the fingerprint in the SDK and call something like this in SentryClient.buildErrorEvent:

event.fingerprint = @[ @"{{ default }}", error.domain, 
	[NSString stringWithFormat:@"%ld", (long)error.code] ];

The advantage would be that our users don't have to add SDK Fingerprinting manually in beforeSend. After gathering some feedback, we can discuss if we want to turn this feature on by default in 7.x.x.

[philipphofmann]

I think I just found another problem. We use a simple message when capturing errors in the SentryClient. Instead, I think we should use a SentryException, as the exception interface can also be used for errors, see https://develop.sentry.dev/sdk/event-payloads/exception/.
The patch in the client could look something like this

- (SentryEvent *)buildErrorEvent:(NSError *)error
{
    SentryEvent *event = [[SentryEvent alloc] initWithLevel:kSentryLevelError];
    SentryException *sentryException = 
        [[SentryException alloc] initWithValue:[NSString stringWithFormat:@"%ld", (long)error.code] type:error.domain];
    event.exceptions = @[sentryException];
    [self setUserInfo:error.userInfo withEvent:event];
    
    return event;
}

[philipphofmann]

The SDK should mark the stacktrace not as snapshot because the stacktrace tells where the error was captured.

[philipphofmann]

Both @untitaker and @mitsuhiko agree that an NSError should be an exception. The stacktrace generated in the SentryClient of an NSError gives you no indication of where the error was propagated, but the stacktrace tells where the error occurred. Using some stacktrace frames for grouping could make sense.

[philipphofmann]

Even if we don't use the stacktrace for default grouping, it adds value as the issue tells you where the error occurred. Furthermore, users can use it for custom grouping.

[philipphofmann]

We don't add the stacktrace to the exception interface as it's generated by the SDK and not coming from the NSError, as NSErrors don't have stacktraces. Instead, we stay with the current approach of adding the stacktrace via the event.threads.

[philipphofmann]

We already agreed that MyDomain 1 and MyDomain 2 should be two different issues in Sentry. Our current proposal suggests that error domain and code should always be grouped together. In other words, MyDomain 1 is always going to end up in the same issue no matter the location in the code. Feedback on this change is highly appreciated.

The patch in the client could look something like this

- (SentryEvent *)buildErrorEvent:(NSError *)error
{
    SentryEvent *event = [[SentryEvent alloc] initWithLevel:kSentryLevelError];
    event.fingerprint = @[ error.domain,
        [NSString stringWithFormat:@"%ld", (long)error.code]];
    
    return event;
}

[untitaker]

I think this is something hierarchical grouping can work with, we will make sdk fingerprints the "root of the tree" and will allow you to split up by the "default grouping", first frame, then second frame, etc... should you so desire

I'm not sure about adding the sdk fingerprint right now as right now you cannot unmerge issues into multiple (unless they have been merged in the UI before), so you could never really triage by top-frame once the SDK starts sending this fingerprint

[philipphofmann]

@untitaker thanks for the input. Can we achieve the behavior described above without SDK side fingerprinting? Using the exception interface with value and type doesn't work right now as Sentry groups these issues by stacktrace, see

https://sentry.io/share/issue/c7a5423c198440c8941f609aff9718fb/

[philipphofmann]

When the SDK doesn't attach the stacktrace Sentry groups by exception and still puts MyDomain 1 and MyDomain 2 into the same issue.

https://sentry.io/share/issue/f0e407a2d4e34cbca5b8681821f15998/

[philipphofmann]

As discussed with @untitaker, SDK fingerprinting is not going to cause problems unless we start using any fingerprint variables as {{default}}.

The ideal solution could be to use a string template for the value of the Exception interface as we have it already for Message. The server then could react accordingly, and we wouldn't need to use SDK Fingerprinting. As this needs alignment across multiple teams we for now we move on with the SDK Fingerprinting and might remove it in the future.

[philipphofmann]

After reaching out to some customers and internal discussions, the decision is to use the exception interface with SDK fingerprinting for error domain and code. We don't add the stacktrace to the exception interface as it's generated by the SDK and not coming from the NSError, as NSErrors don't have stacktraces. Instead, we stay with the current approach of adding the stacktrace via the event.threads.

Merged PR is here #941

[mitsuhiko]

Based on some conversations I would propose we try to avoid setting a fingerprint for now but achieve the same on the server. For that to work we are currently lacking information in the protocol. While we already set a lot of information in the mechanism for ns exceptions we don't yet have space for nserror

Proposed protocol changes for NSError in the mechanism field:

{
   "mechanism": {
     "type": "NSError",
     "ns_error": {
       "domain": error.domain,
       "code": error.code
     }
   }
}

Since nothing sends this currently we can safely update the grouping algorithm on the server to take mechanism into account for NSError. That way we only need to add this to relay and the grouping algorithm and all users who update to the latest SDK will benefit from the updated grouping.

cc @jan-auer @untitaker

[philipphofmann]

PR on Sentry getsentry/sentry#23794

[philipphofmann]

Draft PR in Cocoa #946

[philipphofmann]

On the 28th of July, @untitaker raised concerns that the sentry-cocoa should send the stacktrace of an NSError on the exceptions interface instead of the current approach via event.threads because this causes problems with hierarchical grouping. In February, we decided against this approach, see #929 (reply in thread)

We don't add the stacktrace to the exception interface as it's generated by the SDK and not coming from the NSError, as NSErrors don't have stacktraces. Instead, we stay with the current approach of adding the stacktrace via the event.threads.

An internal discussion with @untitaker and @mitsuhiko came to the conclusion that we stick with the current approach and no further changes are necessary.

[armcknight]

It looks like this is resolved, and in any case has been stale for a while, so will close this discussion.