Add Sentry User Data #12155

jkmassel · 2020-06-10T21:18:16Z

Adds WordPress.com account userID, username, and email address to crash logs.

To test:

Smoke test – launch the app + log in to ensure that Sentry is initialized correctly and the DI stuff is working.
Add require(false) to a Kotlin file where you can trigger it (I'd suggest PagesActivity.kt)
Add return true to the top of CrashLogging.shouldSendEvents to ensure events are sent to Sentry regardless of environment.
Run (not debug) this branch on-device, then trigger the crash and relaunch the app.
Verify that the crash shows up in Sentry with your user details associated.

Next Steps:

I'd like to get the app using the Tracks version of Sentry in order to make things consistent with the other apps, but that should be a separate PR.
If a user logs in and experiences a crash in the same session, their user data will not be sent with the crash. The Tracks version of the Sentry integration has a mechanism for handling this transition, so I'd like to do that work once.

PR submission checklist:

I have considered adding unit tests where possible.
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.

peril-wordpress-mobile · 2020-06-10T21:18:21Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

peril-wordpress-mobile · 2020-06-10T21:23:50Z

You can test the changes on this Pull Request by downloading the APK here.

WordPress/src/main/java/org/wordpress/android/WordPress.java

jkmassel · 2020-06-11T04:21:00Z

WordPress/src/main/java/org/wordpress/android/ui/posts/editor/EditorActionsProvider.kt

-                            "User shouldn't be able to open a trashed post in an editor " +
-                                    "without publishing rights."
-                    )
+                    AppLog.e(T.EDITOR, "User shouldn't be able to open a trashed post in an editor " +


IMHO if we're concerned about this, we should either crash if we're in an invalid state, OR we could just do the right thing, but for now we have the log.

jkmassel · 2020-06-11T04:23:42Z

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

+
+        Sentry.setTag("version", BuildConfig.VERSION_NAME)
+
+        this.accountStore.account.apply {


This doesn't deal with user account transitions. I thought about moving it to beforeSend, but the issue there is that it's called when Sentry starts up, and I'm not sure if the accountStore has the user data at that point, so it sort of becomes a hidden dependency.

As noted in the PR description, the Tracks version of this code is a bit more resilient against this sort of issue, so the next step will be tackling that migration.

Account transitions can be handled in a different way, but I am not sure if I understand why using beforeSend would be an issue. Since beforeSend is a callback, it should be called just before sending the error. Also, since AccountStore is a argument passed in the constructor it should be ready to use at any point in this class. Am I missing something?

Chatted about this separately – because FluxC still reads from the DB on initialization, this should always be correct internally.

Changed in 7941de4

oguzkocer

@jkmassel Looks good. I left some minor suggestions/questions. Let me know what you think about them.

I've also tested the changes and verified the user info showed up in https://sentry.io/share/issue/27e9f284baff4569a93b0161f6459a5f/.

oguzkocer · 2020-06-11T20:23:37Z

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

+     * @param[tag] An optional [AppLog] tag
+     */
+    @JvmOverloads
+    fun logException(tr: Throwable, tag: AppLog.T? = null) = this.log(tr.toString(), tag)


Can we use throwable instead of tr for the parameter name? Named arguments are pretty common in Kotlin and I don't think tr immediately means throwable. This applies to reportException method as well.

For sure – fixed in c54bc21

oguzkocer · 2020-06-11T20:25:17Z

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

+        Sentry.removeExtra("tag")
+        Sentry.removeTag("tag")


I assume we remove these so it doesn't affect future events. I think this might not be super clear to everyone else, so should we add a comment on top of them to explain this? If we do that, can we do it for reportException as well?

Also fixed in c54bc21

Fix extra import Lint fix Fix user ID

Fix style Fix tests Fix test lint issues

This ensures we’re always sending the most relevant data

# Conflicts: # WordPress/src/main/java/org/wordpress/android/modules/AppComponent.java

jkmassel force-pushed the add/sentry-user-data branch 5 times, most recently from f5873f5 to 2b57574 Compare June 11, 2020 00:38

jkmassel requested a review from oguzkocer June 11, 2020 04:13

jkmassel self-assigned this Jun 11, 2020

jkmassel added the [Type] Enhancement label Jun 11, 2020

jkmassel added this to the 15.1 milestone Jun 11, 2020

jkmassel force-pushed the add/sentry-user-data branch from 2b57574 to e08a479 Compare June 11, 2020 04:15

jkmassel commented Jun 11, 2020

View reviewed changes

WordPress/src/main/java/org/wordpress/android/WordPress.java Show resolved Hide resolved

jkmassel commented Jun 11, 2020

View reviewed changes

WordPress/src/main/java/org/wordpress/android/WordPress.java Show resolved Hide resolved

jkmassel commented Jun 11, 2020

View reviewed changes

jkmassel marked this pull request as ready for review June 11, 2020 04:28

oguzkocer assigned oguzkocer and unassigned jkmassel Jun 11, 2020

oguzkocer reviewed Jun 11, 2020

View reviewed changes

jkmassel force-pushed the add/sentry-user-data branch from a34c5b7 to c54bc21 Compare June 11, 2020 22:22

jkmassel requested a review from oguzkocer June 11, 2020 22:43

jkmassel force-pushed the add/sentry-user-data branch from 5ef3875 to 946bb9f Compare June 12, 2020 21:52

jkmassel added 6 commits June 12, 2020 15:53

Rework Crash Logging to use DI + Add User Info

773e7cc

Fix extra import Lint fix Fix user ID

Remove inline crash logging calls

3984a4f

Fix style Fix tests Fix test lint issues

Add inline documentation for CrashLogging class

a1c4f20

Fix documentation and parameter names

0292764

Add user data when sending the event

fa6afbb

This ensures we’re always sending the most relevant data

Fix linter issues

5fc737d

jkmassel force-pushed the add/sentry-user-data branch from 946bb9f to 5fc737d Compare June 12, 2020 21:53

jkmassel and others added 4 commits June 12, 2020 16:16

Re-sync with develop

5457afd

Fix reporting method

89ec438

Fix outdated test references

1958d96

Fixes ZendeskPlanFieldHelperTest's crash logging usage

0600dad

oguzkocer approved these changes Jun 15, 2020

View reviewed changes

Merge remote-tracking branch 'origin/develop' into add/sentry-user-data

387980c

# Conflicts: # WordPress/src/main/java/org/wordpress/android/modules/AppComponent.java

oguzkocer merged commit d8b4ef7 into develop Jun 15, 2020

oguzkocer deleted the add/sentry-user-data branch June 15, 2020 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Sentry User Data #12155

Add Sentry User Data #12155

jkmassel commented Jun 10, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Jun 10, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Jun 10, 2020 •

edited

Loading

jkmassel Jun 11, 2020

jkmassel Jun 11, 2020

oguzkocer Jun 11, 2020

jkmassel Jun 11, 2020

oguzkocer left a comment

oguzkocer Jun 11, 2020

jkmassel Jun 11, 2020 •

edited

Loading

oguzkocer Jun 11, 2020

jkmassel Jun 11, 2020 •

edited

Loading


		Sentry.setTag("version", BuildConfig.VERSION_NAME)

		this.accountStore.account.apply {

Add Sentry User Data #12155

Add Sentry User Data #12155

Conversation

jkmassel commented Jun 10, 2020 • edited Loading

peril-wordpress-mobile bot commented Jun 10, 2020 • edited Loading

peril-wordpress-mobile bot commented Jun 10, 2020 • edited Loading

jkmassel Jun 11, 2020

Choose a reason for hiding this comment

jkmassel Jun 11, 2020

Choose a reason for hiding this comment

oguzkocer Jun 11, 2020

Choose a reason for hiding this comment

jkmassel Jun 11, 2020

Choose a reason for hiding this comment

oguzkocer left a comment

Choose a reason for hiding this comment

oguzkocer Jun 11, 2020

Choose a reason for hiding this comment

jkmassel Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

oguzkocer Jun 11, 2020

Choose a reason for hiding this comment

jkmassel Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

jkmassel commented Jun 10, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Jun 10, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Jun 10, 2020 •

edited

Loading

jkmassel Jun 11, 2020 •

edited

Loading

jkmassel Jun 11, 2020 •

edited

Loading