Skip to content
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

Setting device context in Android prevents native device context from being set #766

Closed
4 of 11 tasks
kuhnroyal opened this issue Feb 23, 2022 · 9 comments
Closed
4 of 11 tasks

Comments

@kuhnroyal
Copy link
Contributor

Platform:

  • Dart
  • Flutter Android or iOS
  • Flutter Web

IDE:

  • VSCode
  • IntelliJ/AS
  • XCode
  • Other, which one?

split-debug-info and obfuscate (Flutter Android or iOS) or CanvasKit (Flutter Web):

  • Enabled
  • Disabled

Platform installed with:

  • pub.dev
  • GitHub

Output of the command flutter doctor -v below:

The output goes here...

The version of the SDK (See pubspec.lock):
6.3.0


I have the following issue:

Setting device context in Android prevents native device context from being set.

Steps to reproduce:

  • Create an EventProcessor and set the device context
    Something like this:
class ConnectivityEventProcessor implements EventProcessor {
  const ConnectivityEventProcessor();

  @override
  Future<SentryEvent> apply(SentryEvent event, {dynamic hint}) async {
    final connectivity = await Connectivity().checkConnectivity();
    return event.copyWith(
      contexts: event.contexts.copyWith(
        device: (event.contexts.device ?? const SentryDevice()).copyWith(
          // online flag ist not being set on iOS by default
          online: event.contexts.device?.online ?? connectivity != ConnectivityResult.none,
        ),
      ),
    );
  }
}

Actual result:

  • Sentry only shows the online or whatever was set in the processor for Android devices
  • Sentry shows everything for iOS devices

Expected result:

  • Sentry shows everything for both OSes
@marandaneto
Copy link
Contributor

Yep, we don't merge field per field (Should we?) but rather only set the object if not set at all.
This should be addressed by getsentry/sentry-cocoa#1235 instead which is adding the missing context for iOS.

@kuhnroyal
Copy link
Contributor Author

(Should we?)

Not sure, that is kinda what I expected.
It behaves differently for iOS/Android. For iOS I can set additional information and it shows up, but not for Android. That is confusing.

@marandaneto
Copy link
Contributor

@kuhnroyal it is, and the reason is the way how the SDKs provide the context to the Hybrid SDKs, it's a flaw and indeed needs to be fixed, it's a known issue already tracked by another issue IIRC.
In your case, the connectivity status needs to be added by the iOS SDK anyway.

@kuhnroyal
Copy link
Contributor Author

Ok thanks, I can work around this and can follow the Cocoa issue.
Feel free to close if the general problem ist tracked somewhere else (I remember something about it in my head but didn't find a ticket).

@ueman
Copy link
Collaborator

ueman commented Apr 21, 2022

This also affects the operating system context on macOS. As soon as one value is set in it, the native side is not filling in the remaining information.

Should we?

Yeah, that's also what I would expect.

@marandaneto
Copy link
Contributor

The downside of merging field per field is that: let's say that you have a specific field set by you or this SDK (let's call device.id) and you delete this data using beforeSend or an event processor, the Native SDK may still add it back if we merge field per field since that field won't be set anymore.
So this change is not trivial and it'd require a specific strategy to avoid that happening.
We acknowledge it's a gap in the SDK design for Hybrid SDKs.

@ueman
Copy link
Collaborator

ueman commented Apr 22, 2022

Isn't that why there's this

/// Load Device's Contexts from the iOS SDK.
///
/// This integration calls the iOS SDK via Message channel to load the
/// Device's contexts before sending the event back to the iOS SDK via
/// Message channel (already enriched with all the information).
///
/// The Device's contexts are:
/// App, Device and OS.
///
/// ps. This integration won't be run on Android because the Device's Contexts
/// is set on Android when the event is sent to the Android SDK via
/// the Message channel.
/// We intend to unify this behaviour in the future.
///
/// This integration is only executed on iOS & MacOS Apps.
class LoadContextsIntegration extends Integration<SentryFlutterOptions> {
final MethodChannel _channel;
LoadContextsIntegration(this._channel);
@override
FutureOr<void> call(Hub hub, SentryFlutterOptions options) async {
options.addEventProcessor(
_LoadContextsIntegrationEventProcessor(_channel, options),
);
options.sdk.addIntegration('loadContextsIntegration');
}
}
class _LoadContextsIntegrationEventProcessor extends EventProcessor {
_LoadContextsIntegrationEventProcessor(this._channel, this._options);
final MethodChannel _channel;
final SentryFlutterOptions _options;
@override
FutureOr<SentryEvent?> apply(SentryEvent event, {hint}) async {
try {
final infos = Map<String, dynamic>.from(
await (_channel.invokeMethod('loadContexts')),
);
if (infos['contexts'] != null) {
final contexts = Contexts.fromJson(
Map<String, dynamic>.from(infos['contexts'] as Map),
);
final eventContexts = event.contexts.clone();
contexts.forEach(
(key, dynamic value) {
if (value != null) {
if (key == SentryRuntime.listType) {
contexts.runtimes.forEach(eventContexts.addRuntime);
} else if (eventContexts[key] == null) {
eventContexts[key] = value;
}
}
},
);
event = event.copyWith(contexts: eventContexts);
}
final userMap = infos['user'];
if (event.user == null && userMap != null) {
final user = Map<String, dynamic>.from(userMap as Map);
event = event.copyWith(user: SentryUser.fromJson(user));
}
if (infos['integrations'] != null) {
final integrations = List<String>.from(infos['integrations'] as List);
final sdk = event.sdk ?? _options.sdk;
for (final integration in integrations) {
if (!sdk.integrations.contains(integration)) {
sdk.addIntegration(integration);
}
}
event = event.copyWith(sdk: sdk);
}
if (infos['package'] != null) {
final package = Map<String, String>.from(infos['package'] as Map);
final sdk = event.sdk ?? _options.sdk;
final name = package['sdk_name'];
final version = package['version'];
if (name != null &&
version != null &&
!sdk.packages.any((element) =>
element.name == name && element.version == version)) {
sdk.addPackage(name, version);
}
event = event.copyWith(sdk: sdk);
}
// on iOS, captureEnvelope does not call the beforeSend callback,
// hence we need to add these tags here.
if (event.sdk?.name == 'sentry.dart.flutter') {
final tags = event.tags ?? {};
tags['event.origin'] = 'flutter';
tags['event.environment'] = 'dart';
event = event.copyWith(tags: tags);
}
} catch (exception, stackTrace) {
_options.logger(
SentryLevel.error,
'loadContextsIntegration failed',
exception: exception,
stackTrace: stackTrace,
);
}
return event;
}
}
integration? That solves the problem in most cases.

One could easily change that to be an event processor and it would solve the problem in all cases.

@marandaneto
Copy link
Contributor

@ueman we don't have for Android yet, it works differently and there's another issue tracking that.
on Android, the events get enriched on the Native SDK.

@marandaneto
Copy link
Contributor

Closing this in favor of getsentry/team-mobile#11

Repository owner moved this from Needs Discussion to Done in Mobile & Cross Platform SDK May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants