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.

Sign up for GitHub

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

Jump to bottom

Feat/only send debug images referenced in the stacktrace for events #2329

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
+68 −17
Draft

Feat/only send debug images referenced in the stacktrace for events #2329

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
49e2768
load only specific debug images
martinhaintz Oct 7, 2024
365e16a
adapt tests
martinhaintz Oct 7, 2024
bad0c67
Merge branch 'main' into feat/only-send-debug-images-referenced-in-th…
martinhaintz Oct 7, 2024
211be42
fix unittest
martinhaintz Oct 8, 2024
05952f9
add fallback to previous handling if no addresses are sent
martinhaintz Oct 8, 2024
8786ea7
switch from frame to instruction address
martinhaintz Oct 8, 2024
fa97c64
Merge branch 'feat/only-send-debug-images-referenced-in-the-stacktrac…
martinhaintz Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions flutter/ios/Classes/SentryFlutterPlugin.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,11 @@
#import <FlutterMacOS/FlutterMacOS.h>
#endif

#import <Sentry/SentryDebugImageProvider.h>

@interface SentryFlutterPlugin : NSObject<FlutterPlugin>
@end

@interface SentryDebugImageProvider ()
- (NSArray<SentryDebugMeta *> * _Nonnull)getDebugImagesForAddresses:(NSSet<NSString *> * _Nonnull)addresses isCrash:(BOOL)isCrash;
@end
12 changes: 9 additions & 3 deletions flutter/ios/Classes/SentryFlutterPluginApple.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@
loadContexts(result: result)

case "loadImageList":
loadImageList(result: result)
loadImageList(call, result: result)

case "initNativeSdk":
initNativeSdk(call, result: result)
Expand Down Expand Up @@ -277,8 +277,14 @@
}
}

private func loadImageList(result: @escaping FlutterResult) {
let debugImages = PrivateSentrySDKOnly.getDebugImages() as [DebugMeta]
private func loadImageList(_ call: FlutterMethodCall, result: @escaping FlutterResult) {
guard let imageAddresses = call.arguments as? Set<String> else {
print("Arguments is not a list of string")
result(FlutterError(code: "X", message: "Cannot get image for address", details: nil))
martinhaintz marked this conversation as resolved.
Show resolved Hide resolved
return
}

let debugImages = SentryDependencyContainer.sharedInstance().debugImageProvider.getDebugImages(forAddresses: imageAddresses, isCrash: false) as [DebugMeta]

Check failure on line 287 in flutter/ios/Classes/SentryFlutterPluginApple.swift

View workflow job for this annotation

GitHub Actions / swift-lint 

Line Length Violation: Line should be 120 characters or less; currently it has 161 characters (line_length)
result(debugImages.map { $0.serialize() })
}

Expand Down
35 changes: 34 additions & 1 deletion flutter/lib/src/integrations/load_image_list_integration.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import 'dart:async';
import 'dart:io';

Check warning on line 2 in flutter/lib/src/integrations/load_image_list_integration.dart

View workflow job for this annotation

GitHub Actions / analyze / analyze

Unused import: 'dart:io'. 

Try removing the import directive.
See https://dart.dev/diagnostics/unused_import to learn more about this problem.

import 'package:sentry/sentry.dart';
import '../native/sentry_native_binding.dart';
Expand Down Expand Up @@ -33,12 +34,44 @@
@override
Future<SentryEvent?> apply(SentryEvent event, Hint hint) async {
if (event.needsSymbolication()) {
final images = await _native.loadDebugImages();
Set<String> imageAddresses = {};
var exceptions = event.exceptions;
if (exceptions != null && exceptions.isNotEmpty) {
for (var e in exceptions) {
if (e.stackTrace != null) {
imageAddresses.addAll(
_collectImageAddressesFromStackTrace(e.stackTrace!),
);
}
}
}

if (event.threads != null && event.threads!.isNotEmpty) {
for (var thread in event.threads!) {
if (thread.stacktrace != null) {
imageAddresses.addAll(
_collectImageAddressesFromStackTrace(thread.stacktrace!),
);
}
}
}

final images = await _native.loadDebugImages(imageAddresses);
if (images != null) {
return event.copyWith(debugMeta: DebugMeta(images: images));
}
}

return event;
}

Set<String> _collectImageAddressesFromStackTrace(SentryStackTrace trace) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe pass the set as an argument to avoid creating intermediate sets on each invocation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vaind can you please explain it a little bit more, what you mean? Thanks.

Copy link
Contributor

@buenaflor buenaflor Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

void _collectInstructionAddresses(
  SentryEvent event,
  Set<String> instructionAddresses,
)

he probably means like this

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exactly, thanks @buenaflor

Set<String> imageAddresses = {};
for (var frame in trace.frames) {
if (frame.imageAddr != null) {
imageAddresses.add(frame.imageAddr!);
martinhaintz marked this conversation as resolved.
Show resolved Hide resolved
}
}
return imageAddresses;
}
}
2 changes: 1 addition & 1 deletion flutter/lib/src/native/sentry_native_binding.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ abstract class SentryNativeBinding {
Future<Map<String, dynamic>?> collectProfile(
SentryId traceId, int startTimeNs, int endTimeNs);

Future<List<DebugImage>?> loadDebugImages();
Future<List<DebugImage>?> loadDebugImages(Set<String> imageAddresses);

Future<void> pauseAppHangTracking();

Expand Down
7 changes: 4 additions & 3 deletions flutter/lib/src/native/sentry_native_channel.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,10 +178,11 @@ class SentryNativeChannel
});

@override
Future<List<DebugImage>?> loadDebugImages() =>
Future<List<DebugImage>?> loadDebugImages(Set<String> imageAddresses) =>
tryCatchAsync('loadDebugImages', () async {
final images = await channel
.invokeListMethod<Map<dynamic, dynamic>>('loadImageList');
final images =
await channel.invokeListMethod<Map<dynamic, Set<String>>>(
'loadImageList', imageAddresses);
return images
?.map((e) => e.cast<String, dynamic>())
.map(DebugImage.fromJson)
Expand Down
17 changes: 11 additions & 6 deletions flutter/test/integrations/load_image_list_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,10 @@ void main() {

setUp(() async {
fixture = IntegrationTestFixture(LoadImageListIntegration.new);
when(fixture.binding.loadDebugImages())
when(fixture.binding.loadDebugImages(<String>{"0x6f80b000"}))
.thenAnswer((_) async => imageList);
// when(fixture.binding.loadDebugImages(any))
// .thenAnswer((_) async => imageList);
martinhaintz marked this conversation as resolved.
Show resolved Hide resolved
await fixture.registerIntegration();
});

Expand All @@ -44,14 +46,14 @@ void main() {
await fixture.hub.captureException(StateError('error'),
stackTrace: StackTrace.current);

verifyNever(fixture.binding.loadDebugImages());
verifyNever(fixture.binding.loadDebugImages(<String>{}));
});

test('Native layer is not called if the event has no stack traces',
() async {
await fixture.hub.captureException(StateError('error'));

verifyNever(fixture.binding.loadDebugImages());
verifyNever(fixture.binding.loadDebugImages(<String>{}));
});

test('Native layer is called because stack traces are not symbolicated',
Expand All @@ -67,7 +69,10 @@ void main() {
#01 abs 000000723d637527 virt 00000000001f0527 _kDartIsolateSnapshotInstructions+0x1e5527
''');

verify(fixture.binding.loadDebugImages()).called(1);
verify(
fixture.binding
.loadDebugImages(<String>{"000000723d6346d7", "000000723d637527"}),
).called(1);
});

test('Event processor adds image list to the event', () async {
Expand Down Expand Up @@ -100,13 +105,13 @@ void main() {
expect(fixture.options.eventProcessors.length, 1);

await fixture.hub.captureMessage('error');
verifyNever(fixture.binding.loadDebugImages());
verifyNever(fixture.binding.loadDebugImages(<String>{"0x6f80b000"}));
});
});
}

SentryEvent _getEvent() {
final frame = SentryStackFrame(platform: 'native');
final frame = SentryStackFrame(platform: 'native', imageAddr: "0x6f80b000");
final st = SentryStackTrace(frames: [frame]);
return SentryEvent(threads: [SentryThread(stacktrace: st)]);
}
6 changes: 4 additions & 2 deletions flutter/test/mocks.mocks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1536,10 +1536,12 @@ class MockSentryNativeBinding extends _i1.Mock
) as _i7.Future<Map<String, dynamic>?>);

@override
_i7.Future<List<_i2.DebugImage>?> loadDebugImages() => (super.noSuchMethod(
_i7.Future<List<_i2.DebugImage>?> loadDebugImages(
Set<String> imageAddresses) =>
(super.noSuchMethod(
Invocation.method(
#loadDebugImages,
[],
[imageAddresses],
),
returnValue: _i7.Future<List<_i2.DebugImage>?>.value(),
) as _i7.Future<List<_i2.DebugImage>?>);
Expand Down
2 changes: 1 addition & 1 deletion flutter/test/sentry_native_channel_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ void main() {
when(channel.invokeMethod('loadImageList'))
.thenAnswer((invocation) async => json);

final data = await sut.loadDebugImages();
final data = await sut.loadDebugImages({"0x6f80b000"});

expect(data?.map((v) => v.toJson()), json);
});
Expand Down
Loading