Ensure Breadcrumbs and SentryEvents are always serialisable

[maBarabas]

📜 Description

Fixes unserialisable Breadcrumbs and SentryEvents created from log records containing an object, stacktrace or error of a type not supported by the StandardMessageCodec.

Fixes #1581

💡 Motivation and Context

Fixes #1581

💚 How did you test it?

Run the dart tests in the repo.

📝 Checklist

• I reviewed submitted code
• I added tests to verify changes
• No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
• I updated the docs if needed
• All tests passing
• No breaking changes

🔮 Next steps

I'm not sure if there is any utility in uploading a second copy of the object this way. It should always be identical to the existing message field of the breadcrumb, so it is just a duplicate. The only information it adds that the log call looked like log.info(MyObject()), rather than log.info('${MyObject}') which is irrelevant.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.87% ⚠️

Comparison is base (d783424) 91.19% compared to head (6312481) 90.32%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1582      +/-   ##
==========================================
- Coverage   91.19%   90.32%   -0.87%     
==========================================
  Files          61      181     +120     
  Lines        2078     5861    +3783     
==========================================
+ Hits         1895     5294    +3399     
- Misses        183      567     +384     

Files Changed	Coverage Δ
logging/lib/src/extension.dart	100.00% <100.00%> (ø)

... and 120 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ueman]

Shouldn't

sentry-dart/dart/lib/src/utils.dart

Line 31 in d783424

Object? jsonSerializationFallback(Object? nonEncodable) {

be responsible for that? That way you can ensure in a more generic way that everything correctly encodable to json.

[maBarabas]

You mean using this callback in combination with jsonEncode? That does seem like a more elegant and would automatically work for any new map entries that are not encodable.

However, if one of the custom user objects has a toJson function, then it will be encoded. This does not seem desirable, as Sentry does not know how to decode it. I guess it would be possible to copy paste from the web UI and decode it using some dart code. This is also more useful than duplicating the toString result.

I can also ignore jsonEncode, and manually call the callback on the objects, but then we lose the automatic fallback for new objects.

[maBarabas]

Just had a closer look at the logs in my Sentry instance, and actually the events that cause the original issue are there in the web UI even without this fix (using Sentry 6.22.0). It seems like the only change from this PR is that the errors are not logged any more.

Is there another path in the code that submits breadcrumbs? The native call here is definitely failing, so the breadcrumb should not be present.

[ueman]

The Flutter to native sync could be a culprit:

sentry-dart/flutter/lib/src/sentry_native_channel.dart

Line 59 in d783424

.invokeMethod('addBreadcrumb', {'breadcrumb': breadcrumb.toJson()});

That method is executed when a breadcrumb is added, which then directly sends it via method channel to the native side. As the object is not serializable, it fails.

But when the actual error is send, the serialization succeeds because the code path is a different one.

This is just a hunch though.

[marandaneto]

See #1581 (comment)
Making the whole Map serializable would benefit the other methods too, so instead of fixing only crumbs, I'd rather fix for all the cases mentioned above in the comment.

[marandaneto]

On Java there's a base interface called JsonSerializable/JsonDeserializer that in case any data class implements this interface, we can call serialize/deserialize, so users that have control under those classes, can implement the interface and write their own custom serializers/deserializers, but it's not really used a lot so I think it's an overkill.
It's enough to fall back to toString when normalizing the Map/Json.

The SDK though should not set any field (automatically) that isn't serializable by default, so I think:

        if (object != null) 'LogRecord.object': object!,
        if (error != null) 'LogRecord.error': error!,
        if (stackTrace != null) 'LogRecord.stackTrace': stackTrace!,

Those should not be part of the crumb anyway, probably an oversight, not just because of serialization.

[marandaneto]

This is going to be addressed by #1591 which is a more generic approach.