From 613094d78f4e24dfa3959c4505c1fda3ed81c546 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Fri, 21 Jan 2022 18:48:44 -0500 Subject: [PATCH 1/7] feat(tracing): Unsampled spans are always created --- src/sentry_core.c | 15 +++++++++ src/sentry_tracing.c | 18 ++--------- tests/unit/test_tracing.c | 66 +++++++++++++++++++++++++++++++++++++++ tests/unit/tests.inc | 1 + 4 files changed, 84 insertions(+), 16 deletions(-) diff --git a/src/sentry_core.c b/src/sentry_core.c index f5ab25f6ea..36178ea095 100644 --- a/src/sentry_core.c +++ b/src/sentry_core.c @@ -919,6 +919,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 " @@ -928,6 +934,15 @@ sentry_span_finish(sentry_span_t *opaque_span) sentry_value_t span = sentry__value_clone(opaque_span->inner); + // 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 038912526e..52bce7b372 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; @@ -235,13 +229,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 @@ -258,7 +245,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 90d89930f2..ba1339155f 100644 --- a/tests/unit/test_tracing.c +++ b/tests/unit/test_tracing.c @@ -559,6 +559,72 @@ 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_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"); + TEST_CHECK(opaque_child != NULL); + 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_span(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); +} + static void check_spans(sentry_envelope_t *envelope, void *data) { 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) From 4414e8891c8d2d3fde3d01823cd1739140c9e297 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Fri, 21 Jan 2022 18:56:19 -0500 Subject: [PATCH 2/7] update tests for distributed spans --- tests/unit/test_tracing.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_tracing.c b/tests/unit/test_tracing.c index ba1339155f..36c2940c03 100644 --- a/tests/unit/test_tracing.c +++ b/tests/unit/test_tracing.c @@ -788,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); - // 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(); From ac4b42a31d3189952eb38acbc81c55087436d60b Mon Sep 17 00:00:00 2001 From: Betty Da Date: Fri, 21 Jan 2022 19:13:10 -0500 Subject: [PATCH 3/7] update to master --- tests/unit/test_tracing.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/unit/test_tracing.c b/tests/unit/test_tracing.c index 36c2940c03..3ec50d4652 100644 --- a/tests/unit/test_tracing.c +++ b/tests/unit/test_tracing.c @@ -568,7 +568,8 @@ SENTRY_TEST(unsampled_spans) 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_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"))); @@ -795,7 +796,7 @@ SENTRY_TEST(distributed_headers) 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); + dist_tx = sentry_transaction_start(tx_ctx, sentry_value_new_null()); TEST_CHECK(!sentry_value_is_true( sentry_value_get_by_key(dist_tx->inner, "sampled"))); From 7cb71df1007e8519b40923e95b7f6231382c8fe8 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Mon, 24 Jan 2022 21:06:11 -0500 Subject: [PATCH 4/7] fix botched merge --- src/sentry_core.c | 63 +++++++++++++++++++++++------------------------ 1 file changed, 31 insertions(+), 32 deletions(-) diff --git a/src/sentry_core.c b/src/sentry_core.c index 84d7eaba10..261b0e733a 100644 --- a/src/sentry_core.c +++ b/src/sentry_core.c @@ -970,44 +970,43 @@ sentry_span_finish(sentry_span_t *opaque_span) 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); - goto fail; - } - - sentry_value_set_by_key(span, "timestamp", - sentry__value_new_string_owned( - sentry__msec_time_to_iso8601(sentry__msec_time()))); - sentry_value_remove_by_key(span, "sampled"); + 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); + goto fail; + } - size_t max_spans = SENTRY_SPANS_MAX; - SENTRY_WITH_OPTIONS (options) { - max_spans = options->max_spans; - } + sentry_value_set_by_key(span, "timestamp", + sentry__value_new_string_owned( + sentry__msec_time_to_iso8601(sentry__msec_time()))); + sentry_value_remove_by_key(span, "sampled"); - sentry_value_t spans - = sentry_value_get_by_key(root_transaction, "spans"); + size_t max_spans = SENTRY_SPANS_MAX; + SENTRY_WITH_OPTIONS (options) { + max_spans = options->max_spans; + } - if (sentry_value_get_length(spans) >= max_spans) { - SENTRY_DEBUG("reached maximum number of spans for transaction, " - "discarding span"); - sentry_value_decref(span); - goto fail; - } + sentry_value_t spans = sentry_value_get_by_key(root_transaction, "spans"); - if (sentry_value_is_null(spans)) { - spans = sentry_value_new_list(); - sentry_value_set_by_key(root_transaction, "spans", spans); - } - sentry_value_append(spans, span); - sentry__span_free(opaque_span); - return; + if (sentry_value_get_length(spans) >= max_spans) { + SENTRY_DEBUG("reached maximum number of spans for transaction, " + "discarding span"); + sentry_value_decref(span); + goto fail; + } - fail: - sentry__span_free(opaque_span); - return; + if (sentry_value_is_null(spans)) { + spans = sentry_value_new_list(); + sentry_value_set_by_key(root_transaction, "spans", spans); } + sentry_value_append(spans, span); + sentry__span_free(opaque_span); + return; + +fail: + sentry__span_free(opaque_span); + return; } #endif From 4957e70e34601c5856e593d14ce977a246e6a006 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Mon, 24 Jan 2022 21:06:53 -0500 Subject: [PATCH 5/7] use more specific identifier to avoid accidentally wiping because a child matched --- src/sentry_core.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/sentry_core.c b/src/sentry_core.c index 261b0e733a..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; @@ -953,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; From 730e169a25a794e83fa6e92e643158f339aa1661 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Mon, 24 Jan 2022 21:49:01 -0500 Subject: [PATCH 6/7] remove an unneeded check --- tests/unit/test_tracing.c | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit/test_tracing.c b/tests/unit/test_tracing.c index a1e85a1ae5..7c3d86fb7d 100644 --- a/tests/unit/test_tracing.c +++ b/tests/unit/test_tracing.c @@ -575,7 +575,6 @@ SENTRY_TEST(unsampled_spans) // i.e. it cascades 1+ levels down sentry_span_t *opaque_child = sentry_transaction_start_child(opaque_tx, "honk", "goose"); - TEST_CHECK(opaque_child != NULL); sentry_value_t child = opaque_child->inner; TEST_CHECK(!sentry_value_is_null(child)); TEST_CHECK( From 05d81e89c2c4d794e5e4f29f21324f4a6c8805c6 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Tue, 25 Jan 2022 12:42:54 -0500 Subject: [PATCH 7/7] forgot a close --- tests/unit/test_tracing.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/unit/test_tracing.c b/tests/unit/test_tracing.c index 7c3d86fb7d..052cf77956 100644 --- a/tests/unit/test_tracing.c +++ b/tests/unit/test_tracing.c @@ -621,6 +621,8 @@ SENTRY_TEST(unsampled_spans) 0 == sentry_value_get_length(sentry_value_get_by_key(tx, "spans"))); sentry_transaction_finish(opaque_tx); + + sentry_close(); } static void