Skip to content

Remove duplicated breadcrumbs when syncing with iOS/macOS#1283

Merged
marandaneto merged 16 commits into
mainfrom
fix/duplicate-breadcrumbs-logging
Feb 28, 2023
Merged

Remove duplicated breadcrumbs when syncing with iOS/macOS#1283
marandaneto merged 16 commits into
mainfrom
fix/duplicate-breadcrumbs-logging

Conversation

@denrase

@denrase denrase commented Feb 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

📜 Description

By fixing the issue that scope sync was never enabled on iOS with #1196, we introduced the issue that breadcrumbs could be duplicated because of bi-directional sync by calling addBreadcrumbs, and loading the native context with loadContexts to enrich events, which also includes the native breadcrumbs.

  • Create breadcrumbs with correct timestamp when syncing down to native layer through addBreadcrumbs
  • Remove duplicate breadcrumbs when syncing up from native layer through loadContexts

💡 Motivation and Context

Closes #1270

💚 How did you test it?

Added unit tests

📝 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

This approach in this PR fixes the issue, but we now have to compare all breadcrumbs with each other, which could get slow with a larger number of breadcrumbs.

Another way would be to just remove syncing breadcrumbs down to the native layer.

@github-actions

github-actions Bot commented Feb 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 285.38 ms 344.74 ms 59.36 ms
Size 6.06 MiB 7.09 MiB 1.03 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
8fa3934 340.64 ms 407.92 ms 67.28 ms
fbf42af 323.04 ms 365.09 ms 42.04 ms
21d4150 379.31 ms 449.23 ms 69.93 ms
08a7b4f 346.47 ms 403.29 ms 56.82 ms
9811573 309.76 ms 402.60 ms 92.84 ms
c70e01a 331.04 ms 401.46 ms 70.42 ms
48e79fd 354.22 ms 391.46 ms 37.24 ms
3637a22 322.59 ms 390.00 ms 67.41 ms
4efee31 308.92 ms 368.68 ms 59.76 ms
6d7a391 331.94 ms 367.04 ms 35.10 ms

App size

Revision Plain With Sentry Diff
8fa3934 6.06 MiB 7.09 MiB 1.03 MiB
fbf42af 5.94 MiB 6.96 MiB 1.02 MiB
21d4150 5.94 MiB 6.97 MiB 1.03 MiB
08a7b4f 5.94 MiB 6.95 MiB 1.01 MiB
9811573 5.94 MiB 6.96 MiB 1.02 MiB
c70e01a 5.94 MiB 6.97 MiB 1.03 MiB
48e79fd 5.94 MiB 6.95 MiB 1.01 MiB
3637a22 6.06 MiB 7.09 MiB 1.03 MiB
4efee31 5.94 MiB 6.92 MiB 1003.76 KiB
6d7a391 5.94 MiB 6.95 MiB 1.01 MiB

Previous results on branch: fix/duplicate-breadcrumbs-logging

Startup times

Revision Plain With Sentry Diff
40f188e 330.40 ms 361.53 ms 31.14 ms
130de29 301.22 ms 354.31 ms 53.09 ms
8d570b0 310.31 ms 368.27 ms 57.96 ms
91e64ae 316.44 ms 359.84 ms 43.40 ms
c910142 317.40 ms 355.68 ms 38.28 ms
a4d5fe6 302.13 ms 378.06 ms 75.94 ms
a638fd5 339.84 ms 379.14 ms 39.30 ms

App size

Revision Plain With Sentry Diff
40f188e 6.06 MiB 7.09 MiB 1.03 MiB
130de29 6.06 MiB 7.09 MiB 1.03 MiB
8d570b0 6.06 MiB 7.09 MiB 1.03 MiB
91e64ae 6.06 MiB 7.09 MiB 1.03 MiB
c910142 6.06 MiB 7.09 MiB 1.03 MiB
a4d5fe6 6.06 MiB 7.09 MiB 1.03 MiB
a638fd5 6.06 MiB 7.09 MiB 1.03 MiB

@github-actions

github-actions Bot commented Feb 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1252.86 ms 1267.53 ms 14.67 ms
Size 8.10 MiB 9.08 MiB 1004.34 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
21845e2 1279.37 ms 1298.81 ms 19.45 ms
b47809a 1252.61 ms 1278.57 ms 25.96 ms
49a149b 1296.47 ms 1320.20 ms 23.73 ms
5603ab2 1268.47 ms 1280.73 ms 12.26 ms
b728df4 1287.43 ms 1293.94 ms 6.51 ms
21d4150 1252.86 ms 1280.55 ms 27.69 ms
1edf30e 1254.43 ms 1272.82 ms 18.39 ms
ccc09e4 1254.74 ms 1277.08 ms 22.34 ms
a49594a 1284.83 ms 1313.29 ms 28.45 ms
c70e01a 1273.00 ms 1299.12 ms 26.12 ms

App size

Revision Plain With Sentry Diff
21845e2 8.15 MiB 9.12 MiB 991.34 KiB
b47809a 8.16 MiB 9.17 MiB 1.01 MiB
49a149b 8.15 MiB 9.12 MiB 986.26 KiB
5603ab2 8.15 MiB 9.12 MiB 990.57 KiB
b728df4 8.15 MiB 9.15 MiB 1020.72 KiB
21d4150 8.16 MiB 9.17 MiB 1.01 MiB
1edf30e 8.16 MiB 9.17 MiB 1.01 MiB
ccc09e4 8.16 MiB 9.16 MiB 1.01 MiB
a49594a 8.16 MiB 9.16 MiB 1.00 MiB
c70e01a 8.16 MiB 9.17 MiB 1.01 MiB

Previous results on branch: fix/duplicate-breadcrumbs-logging

Startup times

Revision Plain With Sentry Diff
40f188e 1270.11 ms 1275.53 ms 5.42 ms
8d570b0 1260.57 ms 1286.88 ms 26.31 ms
c910142 1232.19 ms 1265.57 ms 33.39 ms
a4d5fe6 1267.67 ms 1285.23 ms 17.57 ms
130de29 1251.83 ms 1281.11 ms 29.28 ms
91e64ae 1242.90 ms 1254.87 ms 11.98 ms
a638fd5 1247.36 ms 1263.37 ms 16.01 ms

App size

Revision Plain With Sentry Diff
40f188e 8.10 MiB 9.08 MiB 1004.40 KiB
8d570b0 8.10 MiB 9.08 MiB 1003.32 KiB
c910142 8.10 MiB 9.08 MiB 1004.28 KiB
a4d5fe6 8.10 MiB 9.08 MiB 1004.50 KiB
130de29 8.10 MiB 9.08 MiB 1003.55 KiB
91e64ae 8.10 MiB 9.08 MiB 1003.30 KiB
a638fd5 8.10 MiB 9.08 MiB 1004.35 KiB

@codecov

codecov Bot commented Feb 14, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Patch coverage has no change and project coverage change: -0.96 ⚠️

Comparison is base (2d3b03d) 89.83% compared to head (340d779) 88.87%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1283      +/-   ##
==========================================
- Coverage   89.83%   88.87%   -0.96%     
==========================================
  Files         155      121      -34     
  Lines        5064     3810    -1254     
==========================================
- Hits         4549     3386    -1163     
+ Misses        515      424      -91
Impacted Files Coverage Δ
...ib/src/integrations/load_contexts_integration.dart
.../src/user_interaction/user_interaction_widget.dart
...ter/lib/src/integrations/on_error_integration.dart
flutter/lib/src/sentry_flutter_options.dart
.../src/integrations/widgets_binding_integration.dart
flutter/lib/src/renderer/renderer.dart
...r/lib/src/integrations/screenshot_integration.dart
flutter/lib/src/sentry_native_channel.dart
flutter/lib/src/native_scope_observer.dart
...r/lib/src/integrations/native_sdk_integration.dart
... and 24 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@denrase
denrase marked this pull request as ready for review February 14, 2023 09:11
brustolin
brustolin approved these changes Feb 16, 2023
View reviewed changes

@brustolin brustolin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

marandaneto
marandaneto reviewed Feb 27, 2023
View reviewed changes
Comment thread flutter/lib/src/integrations/load_contexts_integration.dart Outdated
marandaneto
marandaneto reviewed Feb 27, 2023
View reviewed changes
Comment thread flutter/ios/Classes/SentryFlutterPluginApple.swift Outdated
@denrase
denrase requested a review from marandaneto February 28, 2023 09:35
marandaneto
marandaneto approved these changes Feb 28, 2023
View reviewed changes

@marandaneto marandaneto left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @denrase

marandaneto
marandaneto reviewed Feb 28, 2023
View reviewed changes
Comment thread flutter/test/integrations/init_native_sdk_integration_test.dart Outdated
'sendDefaultPii': false,
'enableOutOfMemoryTracking': true,
'enableNdkScopeSync': false,
'enableNdkScopeSync': true,

@marandaneto marandaneto Feb 28, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@denrase is this intentional?

@marandaneto
marandaneto merged commit d2f62d6 into main Feb 28, 2023
@marandaneto
marandaneto deleted the fix/duplicate-breadcrumbs-logging branch February 28, 2023 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@krystofwoldrich krystofwoldrich Awaiting requested review from krystofwoldrich
2 more reviewers
@marandaneto marandaneto marandaneto approved these changes
@brustolin brustolin brustolin approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Duplicated BreadCrumbs from sentry_logging

3 participants

@denrase @marandaneto @brustolin