Review feedback things

src/constants.h

@@ -92,7 +92,6 @@ constexpr const char* SENTRY_DER =
 constexpr const char* SENTRY_ENVELOPE_INGESTION =
     "https://o1396220.ingest.sentry.io/api/6719480/envelope/";
 
-
 constexpr const char* API_PRODUCTION_URL = "https://vpn.mozilla.org";
 constexpr const char* API_STAGING_URL =
     "https://stage-vpn.guardian.nonprod.cloudops.mozgcp.net";

src/featureslist.h

@@ -240,3 +240,11 @@ FEATURE_SIMPLE(gleanRust,             // Feature ID
                FeatureCallback_true,  // Can be flipped off
                QStringList(),         // feature dependencies
                FeatureCallback_false)
+
+FEATURE_SIMPLE(sentry,                     // Feature ID
+               "Sentry Crash Report SDK",  // Feature name
+               "2.13.0",                   // released
+               FeatureCallback_false,      // Can be flipped on
+               FeatureCallback_false,      // Can be flipped off
+               QStringList(),              // feature dependencies
+               FeatureCallback_inStaging)

src/qmake/sources.pri

@@ -132,7 +132,6 @@ SOURCES += \
         rfc/rfc4193.cpp \
         rfc/rfc4291.cpp \
         rfc/rfc5735.cpp \
-        sentry/dummysentryadapter.cpp \
         serveri18n.cpp \
         serverlatency.cpp \
         settingsholder.cpp \
@@ -306,7 +305,6 @@ HEADERS += \
         rfc/rfc4193.h \
         rfc/rfc4291.h \
         rfc/rfc5735.h \
-        sentry/sentryadapter.h \
         serveri18n.h \
         serverlatency.h \
         settingsholder.h \

src/sentry/sentryadapter.cpp

@@ -9,6 +9,7 @@
 #include <QDir>
 
 #include "constants.h"
+#include "models/feature.h"
 #include "leakdetector.h"
 #include "loghandler.h"
 #include "logger.h"
@@ -33,11 +34,9 @@ SentryAdapter::SentryAdapter() { MVPN_COUNT_CTOR(SentryAdapter); }
 SentryAdapter::~SentryAdapter() { MVPN_COUNT_DTOR(SentryAdapter); }
 
 void SentryAdapter::init() {
-  if (Constants::inProduction()) {
-    // If we're not in Production let's just not enable this :)
+  if (!Feature::get(Feature::Feature_sentry)->isSupported()) {
     return;
   }
-  // Okay so Lets INIT
   auto vpn = MozillaVPN::instance();
   auto log = LogHandler::instance();
 
@@ -67,15 +66,16 @@ void SentryAdapter::init() {
   sentry_options_set_transport(options, transport);
 #endif
 
-  // Leaving this for convinence, be warned, it's spammy to stdout.
+  // Uncomment the following line for debugging purposes.
+  // Be warned, it's spammy to stdout.
   // sentry_options_set_debug(options, 1);
 
   if (sentry_init(options) == -1) {
     logger.error() << "Sentry failed to init!";
     return;
   };
   m_initialized = true;
-  logger.info() << "Sentry initialised";
+  logger.info() << "Sentry initialized";
 }
 
 void SentryAdapter::report(const QString& errorType, const QString& message,
@@ -94,35 +94,31 @@ void SentryAdapter::report(const QString& errorType, const QString& message,
   sentry_capture_event(event);
 }
 
-void SentryAdapter::onBeforeShutdown() {
-  // Flush everything,
-  sentry_close();
-}
+void SentryAdapter::onBeforeShutdown() { sentry_close(); }
 
 void SentryAdapter::onLoglineAdded(const QByteArray& line) {
   if (!m_initialized) {
     return;
   }
   // Todo: we could certainly catch this more early and format the data ?
+  // (VPN-3276)
   sentry_value_t crumb =
       sentry_value_new_breadcrumb("Logger", line.constData());
   sentry_add_breadcrumb(crumb);
 }
 
-sentry_value_t SentryAdapter::onCrash(
-    const sentry_ucontext_t*
-        uctx,              // provides the user-space context of the crash
-    sentry_value_t event,  // used the same way as in `before_send`
-    void* closure  // user-data that you can provide at configuration time
-) {
+sentry_value_t SentryAdapter::onCrash(const sentry_ucontext_t* uctx,
+                                      sentry_value_t event, void* closure) {
   logger.info() << "Sentry ON CRASH";
   // Do contextual clean-up before the crash is sent to sentry's backend
   // infrastructure
   bool shouldSend = true;
   // Todo: We can use this callback to make sure
   // we only send data with user consent.
+  // Tracked in: VPN-3158
   // We could:
   //  -> Maybe start a new Process for the Crash-Report UI ask for consent
+  //  (VPN-2823)
   //  -> Check if a setting "upload crashes" is present.
   // If we should not send it, we can discard the crash data here :)
   if (shouldSend) {
@@ -135,24 +131,16 @@ sentry_value_t SentryAdapter::onCrash(
 // static
 void SentryAdapter::transportEnvelope(sentry_envelope_t* envelope,
                                       void* state) {
-  /*
-   * Send the event here. If the transport requires state, such as an HTTP
-   * client object or request queue, it can be specified in the `state`
-   * parameter when configuring the transport. It will be passed as second
-   * argument to this function.
-   * The transport takes ownership of the `envelope`, and must free it once it
-   * is done.
-   */
   Q_UNUSED(state);
   size_t sentry_buf_size = 0;
   char* sentry_buf = sentry_envelope_serialize(envelope, &sentry_buf_size);
 
   // Qt Will copy this.
   auto qt_owned_buffer = QByteArray(sentry_buf, sentry_buf_size);
-  // We can now free the stuff.
+  // We can now free the envelope.
   sentry_envelope_free(envelope);
   sentry_free(sentry_buf);
 
   auto t = new TaskSentry(qt_owned_buffer);
   TaskScheduler::scheduleTask(t);
-}
+}

src/sentry/sentryadapter.h

@@ -18,30 +18,78 @@ class SentryAdapter final : public QObject {
   ~SentryAdapter();
   static SentryAdapter* instance();
 
-  // Inits Sentry.
-  // In case Telemetry is disabled this is a no-op.
+  /**
+   * @brief Inits Sentry.
+   *
+   * This is a no-op if the client is in production mode.
+   */
   void init();
 
-  // Reports an Error - can attach extra data and a stack trace if needed.
+  /**
+   * @brief Sends an "Issue" report to Sentry
+   *
+   * @param category - "Category" of the error, any String is valid.
+   * @param message - Additional message content.
+   * @param attachStackTrace - If true a stacktrace for later debugging will be
+   * attached.
+   */
   void report(const QString& category, const QString& message,
               bool attachStackTrace = false);
 
-  // Called when a Line is added, will add this as a breadcrumb
+  /**
+   * @brief Event Slot for when a log-line is added.
+   *
+   * The logline will be added as a Sentry-Breadrumb, so that
+   * the next "report" or crash will have the last few
+   * loglines available
+   *
+   * @param line - UTF-8 encoded bytes of the logline.
+   */
   Q_SLOT void onLoglineAdded(const QByteArray& line);
-  // Called before shutdown, will flush pending events.
+
+  /**
+   * @brief Event Slot for when the client is about to Shut down
+   *
+   * This will call sentry to wrap up - this might cause new network requests
+   * or drisk writes.
+   * After calling this, any call to sentry is UB.
+   */
   Q_SLOT void onBeforeShutdown();
 
-  // Only define Sentry related callbacks if we have the typedefs :)
-  // Called before Sentry will send a crash report
+  /**
+   * @brief Callback for when sentry's backend recieved a crash.
+   *
+   * In this function we can decide to either send, discard the crash
+   * and additonally "scrub" the minidump of data, if wanted.
+   *
+   * @param uctx provides the user-space context of the crash
+   * @param event used the same way as in `before_send`
+   * @param closure user-data that you can provide at configuration time
+   * (we'dont.)
+   * @return either the @param event or null_sentry_value , if the crash event
+   * should not be recorded.
+   */
   static sentry_value_t onCrash(const sentry_ucontext_t* uctx,
                                 sentry_value_t event, void* closure);
 
-  // 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.
+  /**
+   * @brief Send's a Sentry Event "envelope" to the Sentry endpoint.
+   *
+   * Will be used if NONE_TRANSPORT is enabled in cmake.
+   * Will create a Task in the TaskSheudler to send that.
+   *
+   * @param envelope Envelope to be sent to sentry.
+   * The transport takes ownership of the `envelope`, and must free it once it
+   * is done.
+   * @param state
+   * If the transport requires state, such as an HTTP
+   * client object or request queue, it can be specified in the `state`
+   * parameter when configuring the transport. It will be passed as second
+   * argument to this function. We are not using that.
+   *
+   */
   static void transportEnvelope(sentry_envelope_t* envelope, void* state);
 
-
  private:
   bool m_initialized = false;
   SentryAdapter();