feat(tracing): Only ever allow one active transaction, stuff transact…

…ion into scope

examples/example.c

@@ -223,8 +223,8 @@ main(int argc, char **argv)
             sentry_transaction_set_sampled(tx_ctx, 0);
         }
 
-        sentry_value_t tx = sentry_transaction_start(tx_ctx);
-        sentry_transaction_finish(tx);
+        sentry_transaction_start(tx_ctx);
+        sentry_transaction_finish();
     }
 
     // make sure everything flushes

include/sentry.h

@@ -1289,14 +1289,13 @@ SENTRY_EXPERIMENTAL_API void sentry_transaction_remove_sampled(
  * from an external integration (i.e. a span from a different SDK)
  * or manually constructed by a user.
  */
-SENTRY_EXPERIMENTAL_API sentry_value_t sentry_transaction_start(
+SENTRY_EXPERIMENTAL_API void sentry_transaction_start(
     sentry_value_t transaction);
 
 /**
- * Finishes and sends a transaction to sentry.
+ * Finishes and sends the current transaction to sentry.
  */
-SENTRY_EXPERIMENTAL_API void sentry_transaction_finish(
-    sentry_value_t transaction);
+SENTRY_EXPERIMENTAL_API void sentry_transaction_finish();
 
 #ifdef __cplusplus
 }

src/sentry_core.c

@@ -506,15 +506,6 @@ sentry__prepare_transaction(const sentry_options_t *options,
 {
     sentry_envelope_t *envelope = NULL;
 
-    bool should_skip = !sentry_value_is_true(
-        sentry_value_get_by_key(transaction, "sampled"));
-    if (should_skip) {
-        SENTRY_DEBUG("throwing away transaction due to sample rate");
-        goto fail;
-    }
-    // Field is superfluous, strip so it doesn't leak into the payload
-    sentry_value_remove_by_key(transaction, "sampled");
-
     SENTRY_WITH_SCOPE (scope) {
         SENTRY_TRACE("merging scope into event");
         // Don't include debugging info
@@ -736,12 +727,11 @@ sentry_set_level(sentry_level_t level)
     }
 }
 
-sentry_value_t
+void
 sentry_transaction_start(sentry_value_t tx_cxt)
 {
     sentry_value_t tx = sentry_value_new_event();
 
-    // TODO: stuff transaction into the scope
     bool should_sample = sentry__should_send_transaction(tx_cxt);
     sentry_value_set_by_key(
         tx, "sampled", sentry_value_new_bool(should_sample));
@@ -760,22 +750,35 @@ sentry_transaction_start(sentry_value_t tx_cxt)
     sentry_value_set_by_key(
         tx, "span_id", sentry__value_new_span_uuid(&span_id));
 
+    sentry__scope_set_span(tx);
     sentry_value_decref(tx_cxt);
-
-    return tx;
 }
 
 void
-sentry_transaction_finish(sentry_value_t tx)
+sentry_transaction_finish()
 {
-    sentry_value_t sampled = sentry_value_get_by_key(tx, "sampled");
-    if (!sentry_value_is_null(sampled) && !sentry_value_is_true(sampled)) {
-        sentry_value_decref(sampled);
-        sentry_value_decref(tx);
-        // TODO: remove from scope
+    sentry_value_t tx = sentry_value_new_null();
+    SENTRY_WITH_SCOPE (scope) {
+        if (sentry_value_is_null(scope->span)) {
+            SENTRY_DEBUG("could not find a transaction on the scope to finish");
+            return;
+        }
+        sentry_value_t sampled
+            = sentry_value_get_by_key(scope->span, "sampled");
+        if (!sentry_value_is_true(sampled)) {
+            SENTRY_DEBUG("throwing away transaction due to sample rate");
+            sentry__scope_remove_span();
+            return;
+        }
+        tx = sentry__value_clone(scope->span);
+    }
+    sentry__scope_remove_span();
+    if (sentry_value_is_null(tx)) {
+        SENTRY_DEBUG("could not find a transaction on the scope to finish");
         return;
     }
-    sentry_value_decref(sampled);
+
+    sentry_value_remove_by_key(tx, "sampled");
 
     sentry_value_set_by_key(tx, "type", sentry_value_new_string("transaction"));
     sentry_value_set_by_key(tx, "timestamp",

src/sentry_core.h

@@ -116,7 +116,7 @@ void sentry__options_unlock(void);
     for (sentry_options_t *Options = sentry__options_lock(); Options;          \
          sentry__options_unlock(), Options = NULL)
 
-// these for now are only needed for tests
+// these for now are only needed outside of core for tests
 #ifdef SENTRY_UNITTEST
 bool sentry__roll_dice(double probability);
 bool sentry__should_send_transaction(sentry_value_t tx_cxt);

src/sentry_scope.c

@@ -230,9 +230,30 @@ sentry__symbolize_stacktrace(sentry_value_t stacktrace)
 void
 sentry__scope_set_span(sentry_value_t span)
 {
-    // TODO: implement this function and get rid of this line.
-    (void)span;
-    return;
+    SENTRY_WITH_SCOPE_MUT (scope) {
+        sentry_value_decref(scope->span);
+        scope->span = span;
+    }
+}
+
+#ifdef SENTRY_UNITTEST
+sentry_value_t
+sentry__scope_get_span()
+{
+    SENTRY_WITH_SCOPE (scope) {
+        return scope->span;
+    }
+    return sentry_value_new_null();
+}
+#endif
+
+void
+sentry__scope_remove_span()
+{
+    SENTRY_WITH_SCOPE_MUT (scope) {
+        sentry_value_decref(scope->span);
+        scope->span = sentry_value_new_null();
+    }
 }
 
 void

src/sentry_scope.h

@@ -77,10 +77,23 @@ void sentry__scope_apply_to_event(const sentry_scope_t *scope,
 
 /**
  * Sets the span (actually transaction) on the scope. An internal way to pass
- * around contextual information needed from a transaction into other events.
+ * around contextual information needed from a transaction into other events. If
+ * the scope already contains an unfinished transaction, that transaction will
+ * be discarded and will not be sent to sentry.
+ *
+ * This takes ownership of the span.
  */
 void sentry__scope_set_span(sentry_value_t span);
 
+/**
+ * Removes the current span (actually transaction) on the scope. If the
+ * transaction has not yet finished, this does not finish the transaction
+ * nor does it send it to sentry; The transaction will be discarded.
+ *
+ * Remove at your own discretion.
+ */
+void sentry__scope_remove_span();
+
 /**
  * These are convenience macros to automatically lock/unlock a scope inside a
  * code block.
@@ -96,3 +109,8 @@ void sentry__scope_set_span(sentry_value_t span);
          sentry__scope_unlock(), Scope = NULL)
 
 #endif
+
+// this is only used in unit tests
+#ifdef SENTRY_UNITTEST
+sentry_value_t sentry__scope_get_span();
+#endif

tests/unit/test_tracing.c

@@ -1,7 +1,12 @@
+#include "sentry_scope.h"
 #include "sentry_testsupport.h"
 #include "sentry_tracing.h"
 #include "sentry_uuid.h"
 
+#define CHECK_STRING_PROPERTY(Src, Field, Expected)                            \
+    TEST_CHECK_STRING_EQUAL(                                                   \
+        sentry_value_as_string(sentry_value_get_by_key(Src, Field)), Expected)
+
 SENTRY_TEST(basic_tracing_context)
 {
     sentry_value_t span = sentry_value_new_object();
@@ -116,19 +121,19 @@ SENTRY_TEST(basic_function_transport_transaction)
 
     sentry_value_t transaction
         = sentry_value_new_transaction("How could you", "Don't capture this.");
-    transaction = sentry_transaction_start(transaction);
-    sentry_transaction_finish(transaction);
+    sentry_transaction_start(transaction);
+    sentry_transaction_finish();
     sentry_user_consent_give();
 
     transaction = sentry_value_new_transaction("honk", "beep");
-    transaction = sentry_transaction_start(transaction);
-    sentry_transaction_finish(transaction);
+    sentry_transaction_start(transaction);
+    sentry_transaction_finish();
 
     sentry_user_consent_revoke();
     transaction = sentry_value_new_transaction(
         "How could you again", "Don't capture this either.");
-    transaction = sentry_transaction_start(transaction);
-    sentry_transaction_finish(transaction);
+    sentry_transaction_start(transaction);
+    sentry_transaction_finish();
 
     sentry_close();
 
@@ -153,13 +158,13 @@ SENTRY_TEST(transport_sampling_transactions)
     for (int i = 0; i < 100; i++) {
         sentry_value_t transaction
             = sentry_value_new_transaction("honk", "beep");
-        transaction = sentry_transaction_start(transaction);
-        sentry_transaction_finish(transaction);
+        sentry_transaction_start(transaction);
+        sentry_transaction_finish();
     }
 
     sentry_close();
 
-    // well, its random after all
+    // exact value is nondeterministic because of rng
     TEST_CHECK(called_transport > 50 && called_transport < 100);
 }
 
@@ -191,11 +196,59 @@ SENTRY_TEST(transactions_skip_before_send)
     sentry_init(options);
 
     sentry_value_t transaction = sentry_value_new_transaction("honk", "beep");
-    transaction = sentry_transaction_start(transaction);
-    sentry_transaction_finish(transaction);
+    sentry_transaction_start(transaction);
+    sentry_transaction_finish();
 
     sentry_close();
 
     TEST_CHECK_INT_EQUAL(called_transport, 1);
     TEST_CHECK_INT_EQUAL(called_beforesend, 0);
 }
+
+static void
+before_transport(sentry_envelope_t *envelope, void *data)
+{
+    uint64_t *called = data;
+    *called += 1;
+
+    sentry_envelope_free(envelope);
+}
+
+SENTRY_TEST(multiple_transactions)
+{
+    uint64_t called_transport = 0;
+
+    sentry_options_t *options = sentry_options_new();
+    sentry_options_set_dsn(options, "https://[email protected]/42");
+
+    sentry_transport_t *transport = sentry_transport_new(before_transport);
+    sentry_transport_set_state(transport, &called_transport);
+    sentry_options_set_transport(options, transport);
+
+    sentry_options_set_traces_sample_rate(options, 1.0);
+    sentry_init(options);
+
+    sentry_value_t tx_cxt = sentry_value_new_transaction("wow!", NULL);
+    sentry_transaction_start(tx_cxt);
+
+    sentry_value_t scope_tx = sentry__scope_get_span();
+    CHECK_STRING_PROPERTY(scope_tx, "transaction", "wow!");
+
+    sentry_transaction_finish();
+    scope_tx = sentry__scope_get_span();
+    TEST_CHECK(sentry_value_is_null(scope_tx));
+
+    tx_cxt = sentry_value_new_transaction("whoa!", NULL);
+    sentry_transaction_start(tx_cxt);
+    tx_cxt = sentry_value_new_transaction("wowee!", NULL);
+    sentry_transaction_start(tx_cxt);
+    scope_tx = sentry__scope_get_span();
+    CHECK_STRING_PROPERTY(scope_tx, "transaction", "wowee!");
+    sentry_transaction_finish();
+
+    sentry_close();
+
+    TEST_CHECK_INT_EQUAL(called_transport, 2);
+}
+
+#undef CHECK_STRING_PROPERTY

tests/unit/tests.inc

@@ -29,6 +29,7 @@ XX(module_finder)
 XX(mpack_newlines)
 XX(mpack_removed_tags)
 XX(multiple_inits)
+XX(multiple_transactions)
 XX(os)
 XX(page_allocator)
 XX(path_basics)