diff --git a/src/sentry_core.c b/src/sentry_core.c index 712e5c7c6c..3d869844c2 100644 --- a/src/sentry_core.c +++ b/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,9 +953,9 @@ 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; @@ -957,6 +963,15 @@ sentry_span_finish(sentry_span_t *opaque_span) } } + // 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); diff --git a/src/sentry_tracing.c b/src/sentry_tracing.c index 00b1111b3c..d30785f380 100644 --- a/src/sentry_tracing.c +++ b/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: diff --git a/tests/unit/test_tracing.c b/tests/unit/test_tracing.c index 07dde530db..052cf77956 100644 --- a/tests/unit/test_tracing.c +++ b/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(); diff --git a/tests/unit/tests.inc b/tests/unit/tests.inc index 7114022d58..551df7c1ed 100644 --- a/tests/unit/tests.inc +++ b/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)