Improve ergonomics for reporting nested exceptions

[ueman]

I'd like to request a way to make it easier to handle nested exceptions.
Since there is no built-in way in Dart to nest exceptions, a lot of libraries have their own way of nesting exceptions.

This can already be seen in the current Sentry code base as there are already a couple of places in the current code base where nested exceptions are handled. Those include the PlatformException event processor for Android, the Dio EventProcessor and possibly more.

Now the problem is, that they all handle just their own nested exceptions, but there's no way of recursively resolve interdependencies, like if a nested dio exception is included in a hypothetical other nested exception.

Solution 1:

Therefore, I would like to have an API where I can input an exception and get a sentry exception (or even a sentry event) out of it, without it being send. That api should also apply all the event processors. That way, it's way easier to create better reports for nested exceptions.

Solution 2:

Introduce an interface for nested exceptions which libraries/applications can use and which Sentry can resolve on its own. This probably doesn't really solve the problem for other libraries but maybe for applications.

[marandaneto]

I am not entirely sure if I get it, what you need is something like this?

List<SentryException> sentryExceptions(List<dynamic> exceptions) {
    return exceptions
        .map((exception) => SentryExceptionFactory.getSentryException(exception))
        .toList();
  }

  SentryEvent sentryEvent(List<dynamic> exceptions) {
    final event = SentryEvent(exceptions: sentryExceptions(exceptions));
    return event;
  }

  // final event = sentryEvent([...]);

[ueman]

Yeah, but those should also be passed though the event processors, so that for example the dio eventprocessor can add the request information to the event.

I'll try to add an example, which should demonstrate the problem much better.

[ueman]

Image the following scenario:

• I'm using package dio with package graphql
• The http request fails for whatever reason

The graphql package now catches the DioError from the dio packge.
Which means, I'm now getting a NetworkException which wraps a DioError.

Simplified, it looks like this:

// from graphql
class NetworkException extends LinkException {
  Object? originalException;
  StackTrace? originalStackTrace;
  String? message;
  Uri? uri;
}

// from dio
class DioError implements Exception {
  RequestOptions requestOptions;

  Response? response;
  dynamic error;

  StackTrace? stackTrace;
}

Now I still want to get the http request information added to the Sentry event. In order to do that, I have to write a custom event processor for the NetworkException which also handles the DioError because I can't easily reuse the existing event processor from the Dio integration.

This is just one example, but it also see it happening with our application code and other libraries :(

Does that help making the problem more understandable?

[marandaneto]

I understand the problem but both exceptions come from different libs, so I don't see how this would be solved automatically by the Sentry SDK.
Even #1045 (comment) would not help since we can't do is LinkException either inside of the SDK.
I guess we'd need an integration/processor for graphql that converts NetworkException into a SentryException that also gets added to the event.exception list.
Since they are custom exceptions with fields that are not part of the Dart API for exceptions, it's tricky.
I'll think about it, but feel free to suggest an alternative, open to ideas.

[ueman]

I would like to have an API which is like Sentry.captureException but without the sending it to Sentry. That way, it's easy to recursively create events and merge them in event processors. You also don't have to know the type of inner exceptions because that's done via the recursive call

[ueman]

I've played around a bit and came up with this proof-of-concept:

import '../sentry.dart';

import 'utils/isolate_utils.dart';

class NestedExceptionUtil {
  Future<SentryEvent?> eventFromException({
    required SentryOptions options,
    required Object exception,
    StackTrace? stackTrace,
  }) async {
    SentryEvent event = _attachStackTrace(
      options: options,
      event: SentryEvent(throwable: exception),
      stackTrace: stackTrace,
    );

    for (final processor in options.eventProcessors) {
      SentryEvent? processedEvent = event;
      try {
        processedEvent = await processor.apply(event);
      } catch (exception, stackTrace) {
        options.logger(
          SentryLevel.error,
          'An exception occurred while processing event by a processor',
          exception: exception,
          stackTrace: stackTrace,
        );
      }
      if (processedEvent == null) {
        return null;
      }
      event = processedEvent;
    }
    return event;
  }

  SentryEvent _attachStackTrace({
    required SentryOptions options,
    required SentryEvent event,
    StackTrace? stackTrace,
  }) {
    final isolateName = getIsolateName();
    // Isolates have no id, so the hashCode of the name will be used as id
    final isolateId = isolateName?.hashCode;

    if (event.throwableMechanism != null) {
      var sentryException = options.exceptionFactory.getSentryException(
        event.throwableMechanism,
        stackTrace: stackTrace,
      );

      if (options.platformChecker.isWeb) {
        return event.copyWith(
          exceptions: [
            ...?event.exceptions,
            sentryException,
          ],
        );
      }

      SentryThread? thread;

      if (isolateName != null && options.attachThreads) {
        sentryException = sentryException.copyWith(threadId: isolateId);
        thread = SentryThread(
          id: isolateId,
          name: isolateName,
          crashed: true,
          current: true,
        );
      }

      return event.copyWith(
        exceptions: [...?event.exceptions, sentryException],
        threads: [
          ...?event.threads,
          if (thread != null) thread,
        ],
      );
    }

    // The stacktrace is not part of an exception,
    // therefore add it to the threads.
    // https://develop.sentry.dev/sdk/event-payloads/stacktrace/

    // Don't automatically add stacktraces to inner exceptions
    if (stackTrace != null) {
      final frames = options.stackTraceFactory.getStackFrames(stackTrace);

      if (frames.isNotEmpty) {
        event = event.copyWith(threads: [
          ...?event.threads,
          SentryThread(
            name: isolateName,
            id: isolateId,
            crashed: false,
            current: true,
            stacktrace: SentryStackTrace(frames: frames),
          ),
        ]);
      }
    }
    return event;
  }

  SentryEvent merge(SentryEvent main, SentryEvent second) {
    return main.copyWith(
      user: main.user ?? second.user,
      exceptions: [
        ...?main.exceptions,
        ...?second.exceptions,
      ],
      threads: [
        ...?main.threads,
        ...?second.threads,
      ],
      extra: {
        ...?main.extra,
        ...?second.extra,
      },
      request: main.request ?? second.request,
      serverName: main.serverName ?? second.serverName,
    );
  }
}

Then you could do something like this in an event processor:

// Given this exception from graphql
class NetworkException extends LinkException {
  Object? originalException;
  StackTrace? originalStackTrace;
  String? message;
  Uri? uri;
}

class ExampleEventProcessor implements EventProcessor {

  @override
  FutureOr<SentryEvent?> apply(SentryEvent event, {dynamic hint}) {
    final exception = event.throwable;
    if(exception is NetworkException) {
      final secondaryEvent = NestedExceptionUtil.eventFromException(options, exception.originalException!, exception.originalStackTrace!);
      return NestedExceptionUtil.merge(event,secondaryEvent);
    }
    return event;
  }
}

That way nested exception are recursively added to an event when possible.

[marandaneto]

Give me some time to think about this approach.

[marandaneto]

@ueman what if we offer a method like:

Sentry.captureNestedExceptions(list) and/or SentryEvent is able to take a list of throwable? does that help? so the SDK is able to do its job internally without the need for a custom event processor.

[ueman]

How would that help for automatic reported nested exceptions? At that point, you don't know whether it is a nested exception. For manually reported exceptions that would help though.

[marandaneto]

For manually reported exceptions that would help though.

That would be the first step, yes.

How would that help for automatic reported nested exceptions

You have the instance of the event, the event now gets a list of throwable, you just append the nested exceptions to that list, and the SDK will later transform that list of throwables into a list of SentryException.

Example:

class ExampleEventProcessor implements EventProcessor {

  @override
  FutureOr<SentryEvent?> apply(SentryEvent event, {dynamic hint}) {
    final exception = event.throwable;
    if(exception is NetworkException) {
      event.throwables.add(exception.originalException).
    }
    return event;
  }
}

[marandaneto]

Quick note, maybe this list should also get a stackTrace, so a different data structure would make more sense.

[ueman]

Ah, I see, that makes sense. But currently the event processors run after the SentryException and SentryStackTrace are created right?

[marandaneto]

Ah, I see, that makes sense. But currently the event processors run after the SentryException and SentryStackTrace are created right?

yes, that's a good point, and it should run after because the users should be able to mutate the final version of the event, this is something that should be considered when working on this, not sure how this would look like, maybe having a NestedExceptionUtil would be easier to implement but it has to be documented otherwise most users would never know.

[marandaneto]

Its a good candidate for v7, I will discuss this, thanks for the input.

[ueman]

Maybe there could be hook between Sentry.captureException() and the EventProcessors.

Basically something like (probably not the best naming, but I hope you get the idea)

class ExceptionExctractor<T> {
  // list to handle cases where there might be list of exceptions
  List<Tuple<Object, StackTrace?>> extract(T exception);
}

class NetworkExceptionExtractor implements ExceptionExctractor<NetworkException> {
  List<Tuple<Object, StackTrace?>> extract(NetworkException exception) {
    return [Tuple(exception.originalException, exception.originalStackTrace)];
  }
}

options.registerExtractor(NetworkException, NetworkExceptionExtractor());

And the results of ExceptionExctractor.extract are also passed through each ExceptionExctractor to recursively traverse nested exceptions. After that's done, each individual exception can be passed through the exception/stacktrace factories.

Event Processors then can just handle the case, where there's additional data in an exception to put it on the event.

For dio the event processor would then basically just be

class DioEventProcessor implements EventProcessor {
  @override
  FutureOr<SentryEvent?> apply(SentryEvent event, {dynamic hint}) {
    final dioError = event.throwables.firstWhereOrNull((e) => e is DioError);
    if (dioError == null) {
      return event;
    }

    Contexts contexts = event.contexts;
    if (event.contexts.response == null) {
      contexts = contexts.copyWith(response: _responseFrom(dioError));
    }
    // Don't override just parts of the original request.
    // Keep the original one or if there's none create one.
    event = event.copyWith(
      request: event.request ?? _requestFrom(dioError),
      contexts: contexts,
    );
    return event;
  }
}

That would solve the problem of the order of processing, seems like it's mostly backwards compatible and easy to use.

[marandaneto]

Then event.throwable has to be deprecated in favor of event.throwables, event.throwable for now would return the first item of event.throwables if any.

[marandaneto]

This likely requires a breaking change so adding the v7 milestone in it.
I'm currently discussing this internally and gathering info on how other SDKs handle such use cases.

[marandaneto]

Some languages have a cause or similar method/property within their Exception class, for example, Java, see implementation, JS does something similar with error.cause.
Some languages don't, and people/libs/SDKs usually create/extend their own Exception type, with a cause property, for example, Dart/Dio HTTP client.
The problem is that either when those exceptions are automatically or manually captured, it never reports the real cause because it's wrapped up in a custom exception, so the users don't really know what really happened besides the approximation of the call site (SDKs don't know all the custom exceptions, so we cannot transform them in nested SentryExceptions).
For dio, we added an EventProcessor that does it automatically, but this is not scalable, we cannot have a processor per custom exception/lib/SDK.
Dart does not have reflection nor monkey-patching so we cannot do that at runtime automatically either.
On Cocoa, we have a completely different concept: NSError. Devs use enums for error types. As this turns into an int when sending to Sentry, there is Apple recommends implementing a special interface. So it's something very language-specific which we point out in the docs.

[marandaneto]

@denrase we can solve for what makes more sense to the Dart/Flutter SDK, other SDKs either don't have this problem or the language itself has a builtin solution.
@ueman is there a solution that you played with and liked more?

[ueman]

I was starting to prototype this #1045 (comment) proposal, but I didn't yet make it work. So far, that's the solution I like the most from the solutions from this thread. Maybe there are even better ways?