ref(session): align processing sequence in sentry__capture_event() with docs

.github/workflows/ci.yml

@@ -68,7 +68,7 @@ jobs:
             ERROR_ON_WARNINGS: 1
             SYSTEM_VERSION_COMPAT: 0
             CMAKE_DEFINES: -DCMAKE_OSX_ARCHITECTURES=arm64;x86_64
-          - name: macOS (clang 11 + asan + llvm-cov)
+          - name: macOS (clang 13 + asan + llvm-cov)
             os: macOs-latest
             CC: clang
             CXX: clang++
@@ -136,7 +136,7 @@ jobs:
 
       - name: Expose llvm PATH for Mac
         if: ${{ runner.os == 'macOS' }}
-        run: echo "/usr/local/opt/llvm/bin/" >> $GITHUB_PATH
+        run: echo $(brew --prefix llvm@13)/bin >> $GITHUB_PATH
 
       - name: Installing Android SDK Dependencies
         if: ${{ env['ANDROID_API'] }}

CHANGELOG.md

@@ -1,5 +1,13 @@
 # Changelog
 
+## Unreleased
+
+**Fixes**
+
+- Aligned pre-send event processing in `sentry_capture_event()` with the 
+  [cross-SDK session filter order](https://develop.sentry.dev/sdk/sessions/#filter-order) 
+  ([#729](https://github.com/getsentry/sentry-native/pull/729))
+
 ## 0.4.18
 
 **Features**:

include/sentry.h

@@ -775,6 +775,13 @@ SENTRY_API void sentry_options_set_transport(
  * On Windows, it may be called from inside of a `UnhandledExceptionFilter`, see
  * the documentation on SEH (structured exception handling) for more information
  * https://docs.microsoft.com/en-us/windows/win32/debug/structured-exception-handling
+ *
+ * Up to version 0.4.18 the `before_send` callback wasn't invoked in case the
+ * event sampling discarded an event. In the current implementation the
+ * `before_send` callback is invoked even if the event sampling discards the
+ * event, following the cross-SDK session filter order:
+ *
+ * https://develop.sentry.dev/sdk/sessions/#filter-order
  */
 typedef sentry_value_t (*sentry_event_function_t)(
     sentry_value_t event, void *hint, void *closure);
@@ -801,6 +808,19 @@ SENTRY_API const char *sentry_options_get_dsn(const sentry_options_t *opts);
  * Sets the sample rate, which should be a double between `0.0` and `1.0`.
  * Sentry will randomly discard any event that is captured using
  * `sentry_capture_event` when a sample rate < 1 is set.
+ *
+ * The sampling happens at the end of the event processing according to the
+ * following order:
+ *
+ * https://develop.sentry.dev/sdk/sessions/#filter-order
+ *
+ * Only items 3. to 6. are currently applicable to sentry-native. This means
+ * each processing step is executed even if the sampling discards the event
+ * before sending it to the backend. This is particularly relevant to users of
+ * the `before_send` callback.
+ *
+ * The above is in contrast to versions up to 0.4.18 where the sampling happened
+ * at the beginning of the processing/filter sequence.
  */
 SENTRY_API void sentry_options_set_sample_rate(
     sentry_options_t *opts, double sample_rate);

src/sentry_core.c

@@ -389,6 +389,14 @@ sentry_capture_event(sentry_value_t event)
     }
 }
 
+bool
+sentry__roll_dice(double probability)
+{
+    uint64_t rnd;
+    return probability >= 1.0 || sentry__getrandom(&rnd, sizeof(rnd))
+        || ((double)rnd / (double)UINT64_MAX) <= probability;
+}
+
 sentry_uuid_t
 sentry__capture_event(sentry_value_t event)
 {
@@ -416,8 +424,15 @@ sentry__capture_event(sentry_value_t event)
                 mut_options->session->init = false;
                 sentry__options_unlock();
             }
-            sentry__capture_envelope(options->transport, envelope);
-            was_sent = true;
+
+            bool should_skip = !sentry__roll_dice(options->sample_rate);
+            if (should_skip) {
+                SENTRY_DEBUG("throwing away event due to sample rate");
+                sentry_envelope_free(envelope);
+            } else {
+                sentry__capture_envelope(options->transport, envelope);
+                was_sent = true;
+            }
         }
     }
     if (!was_captured) {
@@ -426,14 +441,6 @@ sentry__capture_event(sentry_value_t event)
     return was_sent ? event_id : sentry_uuid_nil();
 }
 
-bool
-sentry__roll_dice(double probability)
-{
-    uint64_t rnd;
-    return probability >= 1.0 || sentry__getrandom(&rnd, sizeof(rnd))
-        || ((double)rnd / (double)UINT64_MAX) <= probability;
-}
-
 bool
 sentry__should_send_transaction(sentry_value_t tx_cxt)
 {
@@ -461,12 +468,6 @@ sentry__prepare_event(const sentry_options_t *options, sentry_value_t event,
         sentry__record_errors_on_current_session(1);
     }
 
-    bool should_skip = !sentry__roll_dice(options->sample_rate);
-    if (should_skip) {
-        SENTRY_DEBUG("throwing away event due to sample rate");
-        goto fail;
-    }
-
     SENTRY_WITH_SCOPE (scope) {
         SENTRY_TRACE("merging scope into event");
         sentry_scope_mode_t mode = SENTRY_SCOPE_ALL;

tests/unit/test_basic.c

@@ -1,7 +1,6 @@
 #include "sentry_core.h"
 #include "sentry_database.h"
 #include "sentry_testsupport.h"
-#include "sentry_utils.h"
 
 static void
 send_envelope_test_basic(const sentry_envelope_t *envelope, void *data)
@@ -63,14 +62,20 @@ SENTRY_TEST(basic_function_transport)
     TEST_CHECK_INT_EQUAL(called, 2);
 }
 
+static void
+counting_transport_func(const sentry_envelope_t *UNUSED(envelope), void *data)
+{
+    uint64_t *called = data;
+    *called += 1;
+}
+
 static sentry_value_t
 before_send(sentry_value_t event, void *UNUSED(hint), void *data)
 {
     uint64_t *called = data;
     *called += 1;
 
-    sentry_value_decref(event);
-    return sentry_value_new_null();
+    return event;
 }
 
 SENTRY_TEST(sampling_before_send)
@@ -82,7 +87,7 @@ SENTRY_TEST(sampling_before_send)
     sentry_options_set_dsn(options, "https://foo@sentry.invalid/42");
     sentry_options_set_transport(options,
         sentry_new_function_transport(
-            send_envelope_test_basic, &called_transport));
+            counting_transport_func, &called_transport));
     sentry_options_set_before_send(options, before_send, &called_beforesend);
     sentry_options_set_sample_rate(options, 0.75);
     sentry_init(options);
@@ -94,9 +99,46 @@ SENTRY_TEST(sampling_before_send)
 
     sentry_close();
 
+    // The behavior here has changed with version 0.4.19:
+    // the documentation (https://develop.sentry.dev/sdk/sessions/#filter-order)
+    // requires that the sampling-rate filter for all SDKs is executed last.
+    // This means the `before_send` callback will be invoked every time and only
+    // the actual transport will be randomly sampled.
+    TEST_CHECK(called_transport > 50 && called_transport < 100);
+    TEST_CHECK_INT_EQUAL(called_beforesend, 100);
+}
+
+static sentry_value_t
+discarding_before_send(sentry_value_t event, void *UNUSED(hint), void *data)
+{
+    uint64_t *called = data;
+    *called += 1;
+
+    sentry_value_decref(event);
+    return sentry_value_new_null();
+}
+
+SENTRY_TEST(discarding_before_send)
+{
+    uint64_t called_beforesend = 0;
+    uint64_t called_transport = 0;
+
+    sentry_options_t *options = sentry_options_new();
+    sentry_options_set_dsn(options, "https://foo@sentry.invalid/42");
+    sentry_options_set_transport(options,
+        sentry_new_function_transport(
+            counting_transport_func, &called_transport));
+    sentry_options_set_before_send(
+        options, discarding_before_send, &called_beforesend);
+    sentry_init(options);
+
+    sentry_capture_event(
+        sentry_value_new_message_event(SENTRY_LEVEL_INFO, NULL, "foo"));
+
+    sentry_close();
+
     TEST_CHECK_INT_EQUAL(called_transport, 0);
-    // well, its random after all
-    TEST_CHECK(called_beforesend > 50 && called_beforesend < 100);
+    TEST_CHECK_INT_EQUAL(called_beforesend, 1);
 }
 
 SENTRY_TEST(crash_marker)

tests/unit/tests.inc

@@ -18,15 +18,16 @@ XX(child_spans)
 XX(concurrent_init)
 XX(concurrent_uninit)
 XX(count_sampled_events)
-XX(crashed_last_run)
 XX(crash_marker)
+XX(crashed_last_run)
 XX(custom_logger)
+XX(discarding_before_send)
 XX(distributed_headers)
 XX(drop_unfinished_spans)
 XX(dsn_parsing_complete)
 XX(dsn_parsing_invalid)
-XX(dsn_store_url_without_path)
 XX(dsn_store_url_with_path)
+XX(dsn_store_url_without_path)
 XX(empty_transport)
 XX(fuzz_json)
 XX(init_failure)