Skip to content

Logging integration behaviour when message is Object is inconsistent with the Logging package #1581

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
maBarabas opened this issue Aug 3, 2023 · 5 comments · Fixed by #1591
Assignees

Comments

@maBarabas
Copy link
Contributor

Platform

Dart

Obfuscation

Enabled

Debug Info

Enabled

Doctor

[!] Flutter (Channel unknown, 3.10.6, on macOS 13.4 22F66 darwin-x64, locale en-GB)
    ! Flutter version 3.10.6 on channel unknown at /Users/barabas/src/flutter
      Currently on an unknown channel. Run `flutter channel` to switch to an official channel.
      If that doesn't fix the issue, reinstall Flutter by following instructions at https://flutter.dev/docs/get-started/install.
    ! Unknown upstream repository.
      Reinstall Flutter by following instructions at https://flutter.dev/docs/get-started/install.
    • Framework revision f468f3366c (3 weeks ago), 2023-07-12 15:19:05 -0700
    • Engine revision cdbeda788a
    • Dart version 3.0.6
    • DevTools version 2.23.1
    • If those were intentional, you can disregard the above warnings; however it is recommended to use "git" directly to perform update checks and upgrades.

[✓] Android toolchain - develop for Android devices (Android SDK version 34.0.0)
    • Android SDK at /Users/barabas/Library/Android/sdk
    • Platform android-34, build-tools 34.0.0
    • ANDROID_HOME = /Users/barabas/Library/Android/sdk
    • Java binary at: /Applications/Android Studio.app/Contents/jbr/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 17.0.6+0-17.0.6b829.9-10027231)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 14.3.1)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Build 14E300c
    • CocoaPods version 1.11.3

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 2022.3)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 17.0.6+0-17.0.6b829.9-10027231)

[✓] Connected device (3 available)
    • Pixel 5 (mobile) • 0C221FDD4003LH • android-arm64  • Android 13 (API 33)
    • macOS (desktop)  • macos          • darwin-x64     • macOS 13.4 22F66 darwin-x64
    • Chrome (web)     • chrome         • web-javascript • Google Chrome 113.0.5672.126
    ! Error: iPhone XR has recently restarted. Xcode will continue when iPhone XR is unlocked. (code -14)

[✓] Network resources
    • All expected network resources are available.

! Doctor found issues in 1 category.

Version

7.9.0

Steps to Reproduce

  1. Set up an app with Sentry and logging integration
  2. Log an object: Logger('test').info(DemoObject());
class DemoObject {
  final int answer = 42;

  @override
  String toString() {
    return 'DemoObject: $answer';
  }
}

Expected Result

Sentry creates a breadcrumb from the log that matches the output of the app's logger.

Actual Result

Sentry drops the breadcrumb with an exception.

[sentry] [error] Native call `addBreadcrumb` failed
         Invalid argument: Instance of 'DemoObject'
         #0      StandardMessageCodec.writeValue (package:flutter/src/services/message_codecs.dart:465:7)
         #1      StandardMessageCodec.writeValue.<anonymous closure> (package:flutter/src/services/message_codecs.dart:462:9)
         #2      _LinkedHashMapMixin.forEach (dart:collection-patch/compact_hash.dart:625:13)
         #3      StandardMessageCodec.writeValue (package:flutter/src/services/message_codecs.dart:460:13)
         #4      StandardMessageCodec.writeValue.<anonymous closure> (package:flutter/src/services/message_codecs.dart:462:9)
         #5      _LinkedHashMapMixin.forEach (dart:collection-patch/compact_hash.dart:625:13)
         #6      StandardMessageCodec.writeValue (package:flutter/src/services/message_codecs.dart:460:13)
         #7      StandardMessageCodec.writeValue.<anonymous closure> (package:flutter/src/services/message_codecs.dart:462:9)
         #8      _LinkedHashMapMixin.forEach (dart:collection-patch/compact_hash.dart:625:13)
         #9      StandardMessageCodec.writeValue (package:flutter/src/services/message_codecs.dart:460:13)
         #10     StandardMethodCodec.encodeMethodCall (package:flutter/src/services/message_codecs.dart:603:18)
         #11     MethodChannel._invokeMethod (package:flutter/src/services/platform_channel.dart:299:34)
         #12     MethodChannel.invokeMethod (package:flutter/src/services/platform_channel.dart:483:12)
         #13     SentryNativeChannel.addBreadcrumb (package:sentry_flutter/src/sentry_native_channel.dart:59:12)
         #14     SentryNative.addBreadcrumb (package:sentry_flutter/src/sentry_native.dart:72:34)
         #15     NativeScopeObserver.addBreadcrumb (package:sentry_flutter/src/native_scope_observer.dart:29:25)
         #16     Scope.addBreadcrumb.<anonymous closure> (package:sentry/src/scope.dart:195:31)
         #17     Scope._callScopeObservers (package:sentry/src/scope.dart:455:21)
         #18     Scope.addBreadcrumb (package:sentry/src/scope.dart:194:13)
         #19     Hub.addBreadcrumb (package:sentry/src/hub.dart:293:24)
         #20     Sentry.addBreadcrumb (package:sentry/src/sentry.dart:233:12)
         #21     HubAdapter.addBreadcrumb (package:sentry/src/hub_adapter.dart:30:20)
         #22     LoggingIntegration._onLog (package:sentry_logging/src/logging_integration.dart:66:18)
         #23     _RootZone.runUnaryGuarded (dart:async/zone.dart:1594:10)
         #24     _BufferingStreamSubscription._sendData (dart:async/stream_impl.dart:339:11)
         #25     _BufferingStreamSubscription._add (dart:async/stream_impl.dart:271:7)
         #26     _SyncBroadcastStreamController._sendData.<anonymous closure> (dart:async/broadcast_stream_controller.dart:385:20)
         #27     _BroadcastStreamController._forEachListener (dart:async/broadcast_stream_controller.dart:322:15)
         #28     _SyncBroadcastStreamController._sendData (dart:async/broadcast_stream_controller.dart:384:5)
         #29     _BroadcastStreamController.add (dart:async/broadcast_stream_controller.dart:244:5)
         #30     Logger._publish (package:logging/src/logger.dart:287:51)
         #31     Logger.log (package:logging/src/logger.dart:215:18)
         #32     Logger.info (package:logging/src/logger.dart:255:7)
         #33     appMain (package:maestrounite/main.dart:102:8)
         #34     Sentry._init (package:sentry/src/sentry.dart:149:24)
         <asynchronous suspension>
         #35     Sentry.init (package:sentry/src/sentry.dart:70:5)
         <asynchronous suspension>
         #36     SentryFlutter.init (package:sentry_flutter/src/sentry_flutter.dart:83:5)
         <asynchronous suspension>
         #37     main (package:maestrounite/main.dart:69:3)
         <asynchronous suspension>

It seems like the caller of StandardMessageCodec.writeValue should call toString() before passing the Object?

Are you willing to submit a PR?

None

@maBarabas
Copy link
Contributor Author

I think this is the root cause:

image

Breadcrumb.toJson returns invalid Dart JSON map, containing the instance of the class passed to the logger. I think this map should only contain the well known JSON types that are used in Dart. i.e. Map<String, dynamic>, with the dynamic representing int, double, List or Map. Any other type can get turned into a String. Altough this doesn't make a lot of sense, as the string version of the object is already available in the message field.

@krystofwoldrich
Copy link
Member

Hi,
thank you for the message, you point out the cause correctly, the issue is with the object not being known JSON type.

Related Breadcrumb.toJson code.

/// Converts this breadcrumb to a map that can be serialized to JSON according
/// to the Sentry protocol.
Map<String, dynamic> toJson() {
return {
'timestamp': formatDateAsIso8601WithMillisPrecision(timestamp),
if (message != null) 'message': message,
if (category != null) 'category': category,
if (data?.isNotEmpty ?? false) 'data': data,
if (level != null) 'level': level!.name,
if (type != null) 'type': type,
};
}

@marandaneto
Copy link
Contributor

marandaneto commented Aug 7, 2023

That is a bug when calling the Native bridge and syncing data, thanks for raising this.

Instead of fixing this only for crumbs, I'd rather do something similar to

@internal
final utf8JsonEncoder = JsonUtf8Encoder(null, jsonSerializationFallback, null);
@internal
Object? jsonSerializationFallback(Object? nonEncodable) {
if (nonEncodable == null) {
return null;
}
return nonEncodable.toString();
}
but for Map, so the bridge is always called with a valid Map.

The code that fallback to toString should be applied for every data class that is converted to Map and calls the NativeScopeObserver/SentryNativeChannel.
This is only necessary when the data class has a Map with a non-validated field, eg Object or dynamic or any type that is not serializable.

@marandaneto marandaneto moved this from Backlog to Needs Discussion in [DEPRECATED] Mobile SDKs Aug 7, 2023
@marandaneto marandaneto moved this from Needs Discussion to Backlog in [DEPRECATED] Mobile SDKs Aug 7, 2023
@marandaneto
Copy link
Contributor

@marandaneto
Copy link
Contributor

@ueman shared a code snippet:

jsonDecode(jsonEncode(obj.toJson(), toEncodable: jsonSerializationFallback))

This will make it serializable but it has the cost to do a serialization roundtrip.
Let's check if it's possible to just normalize the Map first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment