Skip to content

Conversation

@ueman
Copy link
Collaborator

@ueman ueman commented Oct 12, 2021

📜 Description

This is an integration which intercepts debugPrint and adds them as breadcrumbs.
This fixes #492 by basically removing the call to print done by debugPrint.

💡 Motivation and Context

Fixes #492

💚 How did you test it?

New unit tests

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2021

Codecov Report

❌ Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.24%. Comparing base (64eec98) to head (7c051fe).
⚠️ Report is 1650 commits behind head on main.

Files with missing lines Patch % Lines
.../lib/src/integrations/debug_print_integration.dart 93.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #618      +/-   ##
==========================================
+ Coverage   90.21%   90.24%   +0.02%     
==========================================
  Files          92       93       +1     
  Lines        3016     3034      +18     
==========================================
+ Hits         2721     2738      +17     
- Misses        295      296       +1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@marandaneto
Copy link
Contributor

@ueman does that mean that if calling print(message) it'll forward to our integration?

the other question is, the integration lives in the flutter package, but the ZoneSpecification with the print call is in the dart package, so it'd not work for the dart package?

should we only add this integration by default if enablePrintBreadcrumbs is enabled?

@ueman
Copy link
Collaborator Author

ueman commented Oct 13, 2021

The problem this solves is, that FlutterErrors are collected by the existing FlutterErrorIntegration and then forwarded to the default FlutterError.onError implementation. The default FlutterError.onError implementation calls debugPrint and debugPrint calls print(). And we also intercept calls to print(). The default implementation of debugPrint does throttling and line splitting so that we end up with #492 . Errors are collected twice, once as an event and once as possibly multiple breadcrumbs.

With this we may still end up with duplication, but it's at least well formatted and it may add breadcrumbs for logs which are done directly with debugPrint.

By replacing debugPrint with our own implementation we avoid that print() is called. Thats's why this shouldn't be enabled in debug builds, because that would effectivly disable most of Flutters logging.


@ueman does that mean that if calling print(message) it'll forward to our integration?

No, but debugPrint calls are collected as breadcrumbs.

the other question is, the integration lives in the flutter package, but the ZoneSpecification with the print call is in the dart package, so it'd not work for the dart package?

debugPrint is a Flutter method and can't live in the dart package.

should we only add this integration by default if enablePrintBreadcrumbs is enabled?

That does make sense. Though as mentioned before I'm not sure we ever want to enable it in debug builds.

@marandaneto
Copy link
Contributor

@ueman

That does make sense. Though as mentioned before I'm not sure we ever want to enable it in debug builds.

maybe we do both guards then, only add if release build & enablePrintBreadcrumbs is enabled

@ueman ueman added the flutter label Oct 14, 2021
Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

thanks @ueman, looks good, LGTM

@marandaneto marandaneto merged commit 5395984 into getsentry:main Oct 18, 2021
@ueman ueman deleted the feat/flutter-debug-print-integration branch November 5, 2021 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Don't enable print() breadcrumbs by default on Flutter

3 participants