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

feat: Pass the enableProfiling configuration through to the native SDK #851

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

indragiek
Copy link
Member

📜 Description

The most recent versions of the Sentry SDK's for iOS and Android support profiling. This PR adds an argument to the Flutter configuration options that gets passed through to the native SDK.

💡 Motivation and Context

To be able to profile native code in an app that uses the Sentry Flutter SDK. Note that this does not add support for actually profiling within the Dart VM, only native code.

💚 How did you test it?

Updated the existing 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

@github-actions
Copy link
Contributor

github-actions bot commented Apr 28, 2022

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 9f0b06c

Copy link
Collaborator

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -132,6 +132,7 @@ class SentryFlutterPlugin : FlutterPlugin, MethodCallHandler, ActivityAware {
args.getIfNotNull<Boolean>("anrEnabled") { options.isAnrEnabled = it }
args.getIfNotNull<Boolean>("sendDefaultPii") { options.isSendDefaultPii = it }
args.getIfNotNull<Boolean>("enableNdkScopeSync") { options.isEnableScopeSync = it }
args.getIfNotNull<Boolean>("enableProfiling") { options.isProfilingEnabled = it }
Copy link
Contributor

Choose a reason for hiding this comment

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

That alone isn't enough, since we don't enable performance by default on the Native layer.
My suggestion is: if enableProfiling is given, set the tracesSampleRate to the options as well.

Ps: This will only work if people startTransaction on the Native layer or using auto instrumentation for Native screens, transactions started on the Dart layer won't contain any profiling data, this is something we can improve later.

Comment on lines +265 to +267
if let enableProfiling = arguments["enableProfiling"] as? Bool {
options.enableProfiling = enableProfiling
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +174 to +176
/// Enable capturing CPU profiles alongside transactions.
/// Only available on iOS and Android. This is currently a beta feature that is
/// not available to all Sentry customers.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also write that transactions started on the Dart layer won't have profile data, but only transactions started on the Native layer (Android/iOS).

@@ -376,12 +376,24 @@ class AndroidExample extends StatelessWidget {
},
child: const Text('Platform exception'),
),
ElevatedButton(
Copy link
Contributor

Choose a reason for hiding this comment

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

Move under the Main widget instead of AndroidExample only

@codecov-commenter
Copy link

codecov-commenter commented Apr 28, 2022

Codecov Report

Merging #851 (9f0b06c) into main (dfe8ff4) will decrease coverage by 0.39%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #851      +/-   ##
==========================================
- Coverage   90.33%   89.94%   -0.40%     
==========================================
  Files         119        9     -110     
  Lines        3776      169    -3607     
==========================================
- Hits         3411      152    -3259     
+ Misses        365       17     -348     
Impacted Files Coverage Δ
flutter/lib/src/sentry_flutter_options.dart
dart/lib/src/protocol/sentry_transaction.dart
...art/lib/src/http_client/failed_request_client.dart
flutter/lib/src/sentry_native_channel.dart
dart/lib/src/protocol/sentry_gpu.dart
dart/lib/src/version.dart
dart/lib/src/client_reports/client_report.dart
dart/lib/src/noop_client.dart
dart/lib/src/sentry_sampling_context.dart
...ib/src/environment/_web_environment_variables.dart
... and 100 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfe8ff4...9f0b06c. Read the comment docs.

@@ -39,6 +43,12 @@ class MainActivity : FlutterActivity() {
"platform_exception" -> {
result.success(Thread.currentThread().getStackTrace())
}
"startProfiling" -> {
Copy link
Contributor

@marandaneto marandaneto Apr 28, 2022

Choose a reason for hiding this comment

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

Should also exist under AppDelegate#handleMessage or let the buttons live only in the Android widget as it is now.

@@ -39,6 +43,12 @@ class MainActivity : FlutterActivity() {
"platform_exception" -> {
result.success(Thread.currentThread().getStackTrace())
}
"startProfiling" -> {
transaction = Sentry.startTransaction("myTransactionWithProfiling", "task")
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't test this because of getsentry/sentry-java#2009

@@ -42,5 +42,7 @@
<meta-data
android:name="flutterEmbedding"
android:value="2" />

<meta-data android:name="io.sentry.traces.profiling.enable" android:value="true" />
Copy link
Contributor

Choose a reason for hiding this comment

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

@marandaneto marandaneto marked this pull request as draft May 4, 2022 17:53
@getsantry
Copy link

getsantry bot commented Oct 18, 2023

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added the Stale label Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants