Adds Sentry Performance Monitoring Capabilities by oguzkocer · Pull Request #18496 · wordpress-mobile/WordPress-Android

oguzkocer · 2023-05-23T12:49:50Z

This PR adds Sentry Performance Monitoring capabilities that's managed through a Firebase remote config. It's disabled for debug builds as well as for users who has tracking disabled. It's also disabled by default and will only work if we set the wp_android_performance_monitoring_sample_rate remote config value in the range of 0 < sampleRate <=1.0. The implementation closely follows it's counterpart from WooCommerce-Android project.

The remote config value is set to 0.0 for now. I intend to keep it this way and let folks who are interested in this capability to change the sampling rate as they see fit. This is to prevent us from accidentally sending too many events and getting a large bill to pay.

Note that the TransactionLaunchers and transactions for HTTP requests are out of scope for this PR. We'll need to have an internal discussion about that before we add them.

To test:

Admittedly I don't know how we should test this implementation. The implementation is unit tested and following the implementation from WCAndroid, so I am hopeful that it will work. However, if there is a good way to test this, I'd like to do that before we merge the PR.

@wzieba I'd love to hear your thoughts on testing since you worked on this for WCAndroid. Thanks in advance!

Regression Notes

Potential unintended areas of impact
N/A
What I did to test those areas of impact (or what existing automated tests I relied on)
N/A
What automated tests I added (or what prevented me from doing so)
N/A

PR submission checklist:

I have completed the Regression Notes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

Portrait and landscape orientations.
Light and dark modes.
Fonts: Larger, smaller and bold text.
High contrast.
Talkback.
Languages with large words or with letters/accents not frequently used in English.
Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
Large and small screen sizes. (Tablet and smaller phones)
Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

…rovider

wpmobilebot · 2023-05-23T13:06:59Z

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr18496-704e77f
	Commit	`704e77f`
	Direct Download	`wordpress-prototype-build-pr18496-704e77f.apk`

Note: Google Login is not supported on these builds.

wpmobilebot · 2023-05-23T13:10:59Z

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr18496-704e77f
	Commit	`704e77f`
	Direct Download	`jetpack-prototype-build-pr18496-704e77f.apk`

Note: Google Login is not supported on these builds.

ParaskP7

👋 @oguzkocer !

I have reviewed this PR as per the instructions, everything LGTM and in par with WCAndroid. Good job bringing Sentry's performance monitoring to WPAndroid and making it configurable via Firebase remote config! 🌟 💯 🥇

Admittedly I don't know how we should test this implementation. The implementation is unit tested and following the implementation from WCAndroid, so I am hopeful that it will work. However, if there is a good way to test this, I'd like to do that before we merge the PR.

Same! 😊

PS: All I did is verifying that the Firebase related wp_android_performance_monitoring_sample_rate remote config has been indeed created and has the expected parameter values.

@wzieba I'd love to hear your thoughts on testing since you worked on this for WCAndroid. Thanks in advance!

This! ⬆️

PS: I am going to approve this PR, just to unblock it, but getting @wzieba input before merging is very welcome.

ParaskP7 · 2023-05-24T09:55:35Z

...s/src/test/java/org/wordpress/android/util/crashlogging/WPPerformanceMonitoringConfigTest.kt

+private const val VALID_SAMPLE_RATE = 0.01
+private const val INVALID_SAMPLE_RATE = 0.0
+
+class WPPerformanceMonitoringConfigTest {


Praise (❤️): Awesome work on the tests, many thanks for covering as many scenarios as possible! 🥇

I borrowed 3/4 of these tests from @wzieba's implementation ❤️

ParaskP7 · 2023-05-24T09:56:53Z

WordPress/src/main/java/org/wordpress/android/util/FirebaseRemoteConfigWrapper.kt

    companion object {
+        private const val PERFORMANCE_MONITORING_SAMPLE_RATE_KEY = "wp_android_performance_monitoring_sample_rate"
+
        const val OPEN_WEB_LINKS_WITH_JETPACK_FLOW_FREQUENCY_KEY = "open_web_links_with_jetpack_flow_frequency"


Minor (🔍): Not related to your change, but maybe we should consider making this constant private as well. Feel free to totally ignore this comment. 😊

ParaskP7 · 2023-05-24T10:02:53Z

😞 Ah, this just got merged due to auto-merged being enabled. I usually don't check for auto-merge being enabled on such PRs, especially when extra feedback is waited upon, apologies @oguzkocer , I should have checked anyway! 😭

wzieba · 2023-05-24T10:50:39Z

👋 I don't see anything blocking in this PR so I think the merge was safe!

What I could suggest is:

Integrate CrashLoggingOkHttpInterceptor to have HTTP spans added in performance transaction. I believe that for our mobile apps, it's the most crucial part.
You can enable adding database operations spans by updating Sentry Gradle Plugin settings:

sentry {
    tracingInstrumentation {
        enabled = true
        features = [io.sentry.android.gradle.extensions.InstrumentationFeature.DATABASE]
    }
}

About testing: Sentry adds default transactions for all activities and start of the app. I'm installing the app now to check if the integration works.

wzieba · 2023-05-24T11:01:19Z

It's alive!

wzieba · 2023-05-24T11:06:24Z

I also see that the network interceptor was added automatically by Sentry.

It's good but not perfect - adding our own network interceptor allows us to specify our own com.automattic.android.tracks.crashlogging.RequestFormatter. We should use RequestFormatter to remove data like blog_id or post_id from requests to:

increase privacy
make it possible for Sentry to merge the same types of network spans when it analyzes performance transactions.

A sample RequestFormatter can be found here - see unit tests to see how it works. I think it can be safely reused in WordPress / Jetpack apps.

I see that Sentry also enables, by default, database operations spans, but I think they're safe to leave as it is.

oguzkocer · 2023-05-24T15:27:47Z

@ParaskP7 @wzieba Thank you both for the review! I shouldn't have enabled auto-merge for this PR, sorry about that!

I'm working on a follow up PR to apply the suggestions.

oguzkocer added 4 commits May 23, 2023 08:33

Adds performance monitoring sample rate to FirebaseRemoteConfig

a4c0232

Adds hasUserOptedOut to AnalyticsTracker

92ffb63

Adds WPPerformanceMonitoringConfig and uses it in WPCrashLoggingDataP…

4d70ab7

…rovider

Adds unit tests for WPPerformanceMonitoringConfig

704e77f

oguzkocer added the Tooling label May 23, 2023

oguzkocer added this to the 22.5 milestone May 23, 2023

oguzkocer marked this pull request as ready for review May 23, 2023 13:16

oguzkocer enabled auto-merge May 23, 2023 13:19

oguzkocer requested review from ParaskP7 and wzieba May 23, 2023 13:19

ParaskP7 self-assigned this May 24, 2023

ParaskP7 approved these changes May 24, 2023

View reviewed changes

oguzkocer merged commit babc8a9 into trunk May 24, 2023

oguzkocer deleted the enable/sentry-performance-monitoring branch May 24, 2023 10:00

oguzkocer mentioned this pull request May 24, 2023

Follow up to Adds Sentry Performance Monitoring Capabilities #18507

Merged

12 tasks

ParaskP7 mentioned this pull request May 25, 2023

[Do Not Merge] [Dependency Updates] Update firebaseConfigVersion to 21.4.0 #18513

Closed

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Sentry Performance Monitoring Capabilities#18496

Adds Sentry Performance Monitoring Capabilities#18496
oguzkocer merged 4 commits intotrunkfrom
enable/sentry-performance-monitoring

oguzkocer commented May 23, 2023 •

edited

Loading

Uh oh!

wpmobilebot commented May 23, 2023

Uh oh!

wpmobilebot commented May 23, 2023

Uh oh!

ParaskP7 left a comment

Uh oh!

ParaskP7 May 24, 2023

Uh oh!

oguzkocer May 24, 2023

Uh oh!

ParaskP7 May 24, 2023

Uh oh!

ParaskP7 commented May 24, 2023

Uh oh!

wzieba commented May 24, 2023

Uh oh!

wzieba commented May 24, 2023 •

edited

Loading

Uh oh!

wzieba commented May 24, 2023 •

edited

Loading

Uh oh!

oguzkocer commented May 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

oguzkocer commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Regression Notes

Uh oh!

wpmobilebot commented May 23, 2023

Uh oh!

wpmobilebot commented May 23, 2023

Uh oh!

ParaskP7 left a comment

Choose a reason for hiding this comment

Uh oh!

ParaskP7 May 24, 2023

Choose a reason for hiding this comment

Uh oh!

oguzkocer May 24, 2023

Choose a reason for hiding this comment

Uh oh!

ParaskP7 May 24, 2023

Choose a reason for hiding this comment

Uh oh!

ParaskP7 commented May 24, 2023

Uh oh!

wzieba commented May 24, 2023

Uh oh!

wzieba commented May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wzieba commented May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oguzkocer commented May 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

oguzkocer commented May 23, 2023 •

edited

Loading

wzieba commented May 24, 2023 •

edited

Loading

wzieba commented May 24, 2023 •

edited

Loading