feat(tracing): Always create spans even if they're unsampled

src/sentry_core.c

@@ -782,9 +782,9 @@ sentry_transaction_finish(sentry_transaction_t *opaque_tx)
             sentry_value_t scope_tx = scope->transaction_object->inner;
 
             const char *tx_id = sentry_value_as_string(
-                sentry_value_get_by_key(tx, "trace_id"));
+                sentry_value_get_by_key(tx, "span_id"));
             const char *scope_tx_id = sentry_value_as_string(
-                sentry_value_get_by_key(scope_tx, "trace_id"));
+                sentry_value_get_by_key(scope_tx, "span_id"));
             if (sentry__string_eq(tx_id, scope_tx_id)) {
                 sentry__transaction_decref(scope->transaction_object);
                 scope->transaction_object = NULL;
@@ -933,6 +933,12 @@ sentry_span_finish(sentry_span_t *opaque_span)
 
     sentry_value_t root_transaction = opaque_root_transaction->inner;
 
+    if (!sentry_value_is_true(
+            sentry_value_get_by_key(root_transaction, "sampled"))) {
+        SENTRY_DEBUG("root transaction is unsampled, dropping span");
+        goto fail;
+    }
+
     if (!sentry_value_is_null(
             sentry_value_get_by_key(root_transaction, "timestamp"))) {
         SENTRY_DEBUG("span's root transaction is already finished, aborting "
@@ -947,16 +953,25 @@ sentry_span_finish(sentry_span_t *opaque_span)
             sentry_value_t scope_span = scope->span->inner;
 
             const char *span_id = sentry_value_as_string(
-                sentry_value_get_by_key(span, "trace_id"));
+                sentry_value_get_by_key(span, "span_id"));
             const char *scope_span_id = sentry_value_as_string(
-                sentry_value_get_by_key(scope_span, "trace_id"));
+                sentry_value_get_by_key(scope_span, "span_id"));
             if (sentry__string_eq(span_id, scope_span_id)) {
                 sentry__span_decref(scope->span);
                 scope->span = NULL;
             }
         }
     }
 
+    // Note that the current API makes it impossible to set a sampled value
+    // that's different from the span's root transaction, but let's just be safe
+    // here.
+    if (!sentry_value_is_true(sentry_value_get_by_key(span, "sampled"))) {
+        SENTRY_DEBUG("span is unsampled, dropping span");
+        sentry_value_decref(span);
+        goto fail;
+    }
+
     if (!sentry_value_is_null(sentry_value_get_by_key(span, "timestamp"))) {
         SENTRY_DEBUG("span is already finished, aborting span finish");
         sentry_value_decref(span);

src/sentry_tracing.c

@@ -21,19 +21,13 @@ sentry__value_new_span(sentry_value_t parent, const char *operation)
 
     sentry_value_set_by_key(span, "status", sentry_value_new_string("ok"));
 
-    // Span creation is currently aggressively pruned prior to this function so
-    // once we're in here we definitely know that the span and its parent
-    // transaction are sampled.
-    // Sampling decisions inherited from traces created in other SDKs should be
-    // taken care of `continue_from_headers`, spans don't need to worry about
-    // (inheriting) forced sampling decisions, and transactions cannot be
-    // children of other transactions, so no inheriting of the sampling field is
-    // needed.
     if (!sentry_value_is_null(parent)) {
         sentry_value_set_by_key(span, "trace_id",
             sentry_value_get_by_key_owned(parent, "trace_id"));
         sentry_value_set_by_key(span, "parent_span_id",
             sentry_value_get_by_key_owned(parent, "span_id"));
+        sentry_value_set_by_key(
+            span, "sampled", sentry_value_get_by_key_owned(parent, "sampled"));
     }
 
     return span;
@@ -256,13 +250,6 @@ sentry__value_span_new(
         goto fail;
     }
 
-    // Aggressively discard spans if a transaction is unsampled to avoid
-    // wasting memory
-    sentry_value_t sampled = sentry_value_get_by_key(parent, "sampled");
-    if (!sentry_value_is_true(sampled)) {
-        SENTRY_DEBUG("span's parent is unsampled, not creating span");
-        goto fail;
-    }
     sentry_value_t spans = sentry_value_get_by_key(parent, "spans");
     // This only checks that the number of _completed_ spans matches the
     // number of max spans. This means that the number of in-flight spans
@@ -279,7 +266,6 @@ sentry__value_span_new(
     sentry_value_set_by_key(child, "start_timestamp",
         sentry__value_new_string_owned(
             sentry__msec_time_to_iso8601(sentry__msec_time())));
-    sentry_value_set_by_key(child, "sampled", sentry_value_new_bool(1));
 
     return child;
 fail:

tests/unit/test_tracing.c

@@ -557,6 +557,74 @@ SENTRY_TEST(overflow_spans)
     sentry_close();
 }
 
+SENTRY_TEST(unsampled_spans)
+{
+    sentry_options_t *options = sentry_options_new();
+    sentry_options_set_traces_sample_rate(options, 1.0);
+    sentry_init(options);
+
+    sentry_transaction_context_t *opaque_tx_cxt
+        = sentry_transaction_context_new("noisemakers", NULL);
+    sentry_transaction_context_set_sampled(opaque_tx_cxt, 0);
+    sentry_transaction_t *opaque_tx
+        = sentry_transaction_start(opaque_tx_cxt, sentry_value_new_null());
+    sentry_value_t tx = opaque_tx->inner;
+    TEST_CHECK(!sentry_value_is_true(sentry_value_get_by_key(tx, "sampled")));
+
+    // check that children and grandchildren inherit the sampling decision,
+    // i.e. it cascades 1+ levels down
+    sentry_span_t *opaque_child
+        = sentry_transaction_start_child(opaque_tx, "honk", "goose");
+    sentry_value_t child = opaque_child->inner;
+    TEST_CHECK(!sentry_value_is_null(child));
+    TEST_CHECK(
+        !sentry_value_is_true(sentry_value_get_by_key(child, "sampled")));
+
+    sentry_span_t *opaque_grandchild
+        = sentry_span_start_child(opaque_child, "beep", "car");
+    sentry_value_t grandchild = opaque_grandchild->inner;
+    TEST_CHECK(!sentry_value_is_null(grandchild));
+    TEST_CHECK(
+        !sentry_value_is_true(sentry_value_get_by_key(grandchild, "sampled")));
+
+    // finishing does not add (grand)children to the spans list
+    sentry_span_finish(opaque_grandchild);
+    TEST_CHECK(
+        0 == sentry_value_get_length(sentry_value_get_by_key(tx, "spans")));
+
+    sentry_span_finish(opaque_child);
+    TEST_CHECK(
+        0 == sentry_value_get_length(sentry_value_get_by_key(tx, "spans")));
+
+    // perform the same checks, but with the transaction on the scope
+    sentry_set_transaction_object(opaque_tx);
+
+    opaque_child = sentry_transaction_start_child(opaque_tx, "toot", "boat");
+    child = opaque_child->inner;
+    TEST_CHECK(!sentry_value_is_null(child));
+    TEST_CHECK(
+        !sentry_value_is_true(sentry_value_get_by_key(child, "sampled")));
+
+    opaque_grandchild
+        = sentry_span_start_child(opaque_child, "vroom", "sportscar");
+    grandchild = opaque_grandchild->inner;
+    TEST_CHECK(!sentry_value_is_null(grandchild));
+    TEST_CHECK(
+        !sentry_value_is_true(sentry_value_get_by_key(grandchild, "sampled")));
+
+    sentry_span_finish(opaque_grandchild);
+    TEST_CHECK(
+        0 == sentry_value_get_length(sentry_value_get_by_key(tx, "spans")));
+
+    sentry_span_finish(opaque_child);
+    TEST_CHECK(
+        0 == sentry_value_get_length(sentry_value_get_by_key(tx, "spans")));
+
+    sentry_transaction_finish(opaque_tx);
+
+    sentry_close();
+}
+
 static void
 check_spans(sentry_envelope_t *envelope, void *data)
 {
@@ -720,12 +788,20 @@ SENTRY_TEST(distributed_headers)
     TEST_CHECK(!sentry_value_is_true(
         sentry_value_get_by_key(dist_tx->inner, "sampled")));
 
+    child = sentry_transaction_start_child(tx, "honk", "goose");
+    TEST_CHECK(!sentry_value_is_true(
+        sentry_value_get_by_key(child->inner, "sampled")));
+
+    tx_ctx = sentry_transaction_context_new("distributed from a child!", NULL);
+    sentry_span_iter_headers(child, forward_headers_to, (void *)tx_ctx);
     sentry__transaction_decref(dist_tx);
+    dist_tx = sentry_transaction_start(tx_ctx, sentry_value_new_null());
 
-    // TODO: Check the sampled flag on a child span as well, but I think we
-    // don't create one if the transaction is not sampled? Well, here is the
-    // reason why we should!
+    TEST_CHECK(!sentry_value_is_true(
+        sentry_value_get_by_key(dist_tx->inner, "sampled")));
 
+    sentry__transaction_decref(dist_tx);
+    sentry__span_free(child);
     sentry__transaction_decref(tx);
 
     sentry_close();

tests/unit/tests.inc

@@ -60,6 +60,7 @@ XX(transaction_name_backfill_on_finish)
 XX(transactions_skip_before_send)
 XX(transport_sampling_transactions)
 XX(uninitialized)
+XX(unsampled_spans)
 XX(unwinder)
 XX(url_parsing_complete)
 XX(url_parsing_invalid)