Cross-Plattform Crash Reporting #4455

strseb · 2022-09-14T18:15:52Z

Description

This PR adds experimental crash reporting support using sentry-native.
The feature is disabled by default, only enabled when we're on guardian-stage.

By Crossplattform, i mean osx and windows for now :) - Linux should be straightforward to add, i don't have a vm to test that on right now.

Android & iOS are theoretically supported. We need to either figurre out how to get this into the qmake structure, or port those plattforms finally to cmake again like (#4301)

##Ref
https://mozilla-hub.atlassian.net/browse/VPN-2822

Checklist

My code follows the style guidelines for this project
I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question) -> I DID.
I have performed a self review of my own code
I have commented my code PARTICULARLY in hard to understand areas
I have added thorough tests where needed

codecov-commenter · 2022-09-14T18:40:43Z

Codecov Report

Base: 37.23% // Head: 35.50% // Decreases project coverage by -1.72% ⚠️

Coverage data is based on head (21664db) compared to base (46f177c).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4455      +/-   ##
==========================================
- Coverage   37.23%   35.50%   -1.73%     
==========================================
  Files         325      328       +3     
  Lines       21699    22038     +339     
  Branches    11739    11832      +93     
==========================================
- Hits         8079     7825     -254     
- Misses       6080     6778     +698     
+ Partials     7540     7435     -105

Flag	Coverage Δ
functional_tests	`22.32% <0.00%> (-5.12%)`	⬇️
lottie_tests	`23.06% <ø> (ø)`
unit_tests	`34.14% <0.00%> (+0.68%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/commands/commandui.cpp	`38.61% <ø> (-1.39%)`	⬇️
src/constants.h	`20.00% <ø> (-2.23%)`	⬇️
src/featureslist.h	`0.00% <0.00%> (ø)`
src/mozillavpn.cpp	`23.58% <0.00%> (-0.65%)`	⬇️
src/networkrequest.cpp	`17.91% <0.00%> (-0.85%)`	⬇️
src/networkrequest.h	`0.00% <ø> (-50.00%)`	⬇️
src/tasks/sentry/tasksentry.cpp	`0.00% <0.00%> (ø)`
src/tasks/addonindex/taskaddonindex.h	`0.00% <0.00%> (-100.00%)`	⬇️
src/captiveportal/captiveportalrequest.h	`0.00% <0.00%> (-100.00%)`	⬇️
src/tasks/removedevice/taskremovedevice.h	`0.00% <0.00%> (-100.00%)`	⬇️
... and 113 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.

LICENSE.md

src/cmake/sentry.cmake

bakulf · 2022-09-15T05:37:15Z

src/cmake/sentry.cmake

+
+
+# Defines which OS builds can include sentry. Check src/cmake Lists for all values of MVPN_PLATFORM_NAME
+set(SENTRY_SUPPORTED_OS  "windows" "macos")


Why only these 2 platforms?

Adding it is trivial, but i assume that I cannot use the ExernalProject with the PPA builds, as that would need to be present in the source bundle thing?

src/qmake/sources.pri

bakulf · 2022-09-15T05:43:50Z

src/sentry/sentryadapter.cpp

+  auto appLocal = appDatas.first() + "\\sentry";
+  sentry_options_set_dsn(options, Constants::SENTRY_DER);
+  sentry_options_set_environment(
+      options, Constants::inProduction() ? "production" : "stage");


We are in prod. See the if-stmt at the top

src/sentry/sentryadapter.cpp

src/sentry/sentryadapter.h

bakulf · 2022-09-15T05:47:59Z

src/sentry/sentryadapter.cpp

+  //  -> Maybe start a new Process for the Crash-Report UI ask for consent
+  //  -> Check if a setting "upload crashes" is present.
+  // If we should not send it, we can discard the crash data here :)
+  if (shouldSend) {


Check with legal if we can use the telemetry policy flag.

src/sentry/sentryadapter.cpp

src/sentry/dummysentryadapter.cpp

brizental · 2022-11-17T10:50:01Z

src/cmake/sentry.cmake

+        # this is super easy to link against and we do not need another binary shipped with the client.
+        SET(SENTRY_ARGS -DSENTRY_BACKEND=breakpad -DSENTRY_BUILD_SHARED_LIBS=false -DSENTRY_TRANSPORT=none)
+    endif()
+    if(WIN32)


nit: Can we use an elseif here? Also for the Android conditional.

brizental · 2022-11-17T10:52:03Z

src/cmake/sentry.cmake

+
+
+# Defines which OS builds can include sentry. Check src/cmake Lists for all values of MVPN_PLATFORM_NAME
+set(SENTRY_SUPPORTED_OS  "Windows" "Darwin" "Android")


Why no iOS and Linux?

Not tested so far, as i don't have a build machine setuped yet 😅.
For iOS I want to wait for it to be moved to cmake before i start that.

Heh, totally get that. Let's file tickets for the iOS and Linux work though, so we can keep track of it.

brizental · 2022-11-17T10:53:16Z

src/cmake/sentry.cmake

+else()
+    # Sentry is not supported on this Plattform.
+    message("Sentry supported OS -> ${SENTRY_SUPPORTED_OS}")
+    message("Cannot build sentry for ${CMAKE_SYSTEM_NAME}")


nit: I'd remove this message. It's redudant with the previous one and it looks like an error.

brizental · 2022-11-17T10:53:37Z

src/cmake/sentry.cmake

+target_sources(mozillavpn PRIVATE
+    tasks/sentry/tasksentry.cpp
+    tasks/sentry/tasksentry.h
+)


nit: newline.

brizental · 2022-11-17T10:54:26Z

src/commands/commandui.cpp

@@ -179,9 +178,6 @@ int CommandUI::run(QStringList& tokens) {
      std::cerr.clear();
    }
 #  endif
-
-    CrashClient::instance().start(CommandLineParser::argc(),
-                                  CommandLineParser::argv());


Question: What are we removing here? What is the crash client? How is this related to the Sentry changes?

The crash client is a mini qt application we're shipping in the client. It shows the "ohno it's crashed, wanna upload?" prompt. It's currently only running under windows.
This line will make the crash client register itself for "structured exception handling", - which we don't want as sentry will do that.
On that note, I currently don't want to rip that whole thing out.... yet. We might still want to create the crash-ui process, in the sentry onCrash handler.

Alright, yeah my question was a precursor to me asking if you wanted to also just remove the whole thing. Thanks for clarifying!

brizental · 2022-11-17T11:04:58Z

src/sentry/sentryadapter.cpp

+   * argument to this function.
+   * The transport takes ownership of the `envelope`, and must free it once it
+   * is done.
+   */


Make this also a doxygen style comment?

brizental · 2022-11-17T11:07:46Z

src/sentry/sentryadapter.h

+  // Will be used if NONE_TRANSPORT is enabled
+  // in that case this will be called if the client has a
+  // report to send to Sentry.
+  static void transportEnvelope(sentry_envelope_t* envelope, void* state);


Love the comment adding on this file. In mozilla-central docstrings are added to the header files too, right? I've been adding them to the .cpp file, maybe we should reach an agreement about this, happy to swich from .cpp to .h. On another note, I believe ideally we should use doxygen format comments, just because if we ever decide to auto generate docs based on these doc-comments it will be easier that way.

mc has most in the header; Which i kind of like because I can easily can get a glimpse of what everything in the class is happening. :D
Totally with you about the standard comment, I already forgot that we agreed on a standard format :D

LOL, well me and you agreed. We should probably broadcast that. I agree with having it in the header. I'll do that from now on and also change the stuff that is not in the header!

File looks beautiful now with all the comments 💯

brizental · 2022-11-17T11:08:02Z

src/tasks/sentry/tasksentry.cpp

+            logger.debug() << "Sentry sent events";
+            emit completed();
+          });
+}


nit: newline.

brizental · 2022-11-17T11:08:18Z

src/tasks/sentry/tasksentry.h

+  QByteArray m_envelope;
+};
+
+#endif  // TASKSENTRY_H


nit: newline.

taskcluster/scripts/fetch/enter_dev_shell.ps1

brizental

LGTM. Left some minor stuff. This is a great change, excited for those sentry reports!

brizental · 2022-11-17T13:57:07Z

src/featureslist.h

+               "Sentry Crash Report SDK",  // Feature name
+               "2.13.0",                   // released
+               FeatureCallback_false,      // Can be flipped on
+               FeatureCallback_false,      // Can be flipped off


Why not allow flipping it on and off?

brizental · 2022-11-17T13:59:06Z

src/sentry/sentryadapter.h

+  /**
+   * @brief Inits Sentry.
+   *
+   * This is a no-op if the client is in production mode.


nit: "This is a no-op if the "sentry" feature is turned off".

This way if we change the feature we don't need to update this again :)

src/featureslist.h

strseb requested a review from bakulf September 14, 2022 18:15

bakulf reviewed Sep 15, 2022

View reviewed changes

bakulf reviewed Sep 18, 2022

View reviewed changes

src/sentry/sentryadapter.cpp Show resolved Hide resolved

strseb mentioned this pull request Sep 19, 2022

Cross-Plattform Crash Reporting: Add Android Support #4484

Closed

strseb requested a review from bakulf September 20, 2022 11:58

bakulf reviewed Sep 21, 2022

View reviewed changes

src/sentry/dummysentryadapter.cpp Outdated Show resolved Hide resolved

strseb added 13 commits November 14, 2022 15:16

Sentry WIP

d5afd95

Make it a proper integration

c248adb

Fix fallback for unsupported plattforms

5d118dc

Proper windows support

dd2f9d8

Cleanups

d8fbfb9

dummy version for qmake variants

39b6523

add license

7d00bf6

cleanup

2493b1f

cleanup

0e7fe2d

Address feedback

dfae022

add mit license

3ca44c3

Fix Lib importing ?

d6c0a95

Check init state before reporting stuff

31a86e6

strseb force-pushed the sentry branch from d561f23 to 31a86e6 Compare November 14, 2022 14:20

strseb added 5 commits November 14, 2022 17:37

add subdir

a1ab039

Fix OSX build woes

138ff48

Add android support to the supported list

60d5e10

Remove unused variable

d3d5126

Pr cleanups

b279b62

strseb requested review from bakulf and brizental November 16, 2022 17:34

Remove: dummysentry

5d480e2

brizental reviewed Nov 17, 2022

View reviewed changes

Review feedback things

60e95ce

strseb requested a review from brizental November 17, 2022 12:58

brizental approved these changes Nov 17, 2022

View reviewed changes

strseb force-pushed the sentry branch from 3402e9f to 996e2f0 Compare November 17, 2022 17:18

WIP: Fix submodules?

46dd859

strseb force-pushed the sentry branch from 996e2f0 to 46dd859 Compare November 17, 2022 18:18

strseb added 5 commits November 22, 2022 16:06

Load Sentry config from Cmake Arguments

2c87cb0

Remove android 32 bit support

25b3023

assert stuff exists during release builds

1bfe91a

just disable sentry if the key's are not given

891a0f9

linting

9b52e63

strseb force-pushed the sentry branch from 1bb7220 to 9b52e63 Compare November 22, 2022 18:26

strseb added 2 commits November 22, 2022 20:44

Fix addons

c4ff6a2

remove console fix for windows

21664db

strseb merged commit 65f10b6 into main Nov 23, 2022

strseb deleted the sentry branch November 23, 2022 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cross-Plattform Crash Reporting #4455

Cross-Plattform Crash Reporting #4455

strseb commented Sep 14, 2022

codecov-commenter commented Sep 14, 2022 •

edited

Loading

bakulf Sep 15, 2022

strseb Sep 15, 2022

bakulf Sep 15, 2022

bakulf Sep 15, 2022

brizental Nov 17, 2022

brizental Nov 17, 2022

strseb Nov 17, 2022

brizental Nov 17, 2022 •

edited

Loading

brizental Nov 17, 2022

brizental Nov 17, 2022

brizental Nov 17, 2022

strseb Nov 17, 2022

brizental Nov 17, 2022

brizental Nov 17, 2022

brizental Nov 17, 2022

strseb Nov 17, 2022

brizental Nov 17, 2022

brizental Nov 17, 2022

brizental Nov 17, 2022

brizental Nov 17, 2022

brizental left a comment

brizental Nov 17, 2022

brizental Nov 17, 2022



		# Defines which OS builds can include sentry. Check src/cmake Lists for all values of MVPN_PLATFORM_NAME
		set(SENTRY_SUPPORTED_OS "windows" "macos")



		# Defines which OS builds can include sentry. Check src/cmake Lists for all values of MVPN_PLATFORM_NAME
		set(SENTRY_SUPPORTED_OS "Windows" "Darwin" "Android")

Cross-Plattform Crash Reporting #4455

Cross-Plattform Crash Reporting #4455

Conversation

strseb commented Sep 14, 2022

Description

Checklist

codecov-commenter commented Sep 14, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brizental Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brizental left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 14, 2022 •

edited

Loading

brizental Nov 17, 2022 •

edited

Loading