From ef766be5b6d0ba26dc39a306084eeca80f0ff778 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Fri, 17 Dec 2021 02:14:00 -0500 Subject: [PATCH] feat(tracing): Defer some transaction validation and allow creation of internal spans --- src/sentry_core.c | 21 ++++++++++--- src/sentry_value.c | 35 ++++++++++++++------- src/sentry_value.h | 7 +++++ tests/unit/test_tracing.c | 65 +++++++++++++++++++++++++++++---------- tests/unit/tests.inc | 1 + 5 files changed, 97 insertions(+), 32 deletions(-) diff --git a/src/sentry_core.c b/src/sentry_core.c index 4b23ca03d..1484eeefb 100644 --- a/src/sentry_core.c +++ b/src/sentry_core.c @@ -730,14 +730,24 @@ sentry_set_level(sentry_level_t level) void sentry_transaction_start(sentry_value_t tx_cxt) { + // TODO: it would be nice if we could just merge tx_cxt into tx. + // `sentry_value_new_transaction_event()` is also an option, but risks + // causing more confusion as there's already a + // `sentry_value_new_transaction`. The ending timestamp is stripped as well + // to avoid misleading ourselves later down the line. sentry_value_t tx = sentry_value_new_event(); + sentry_value_remove_by_key(tx, "timestamp"); bool should_sample = sentry__should_send_transaction(tx_cxt); sentry_value_set_by_key( tx, "sampled", sentry_value_new_bool(should_sample)); + sentry_value_set_by_key(tx, "transaction", + sentry_value_get_by_key_owned(tx_cxt, "transaction")); + sentry_value_set_by_key( - tx, "transaction", sentry_value_get_by_key_owned(tx_cxt, "name")); + tx, "status", sentry_value_get_by_key_owned(tx_cxt, "status")); + sentry_value_set_by_key(tx, "start_timestamp", sentry__value_new_string_owned( sentry__msec_time_to_iso8601(sentry__msec_time()))); @@ -786,10 +796,13 @@ sentry_transaction_finish() sentry__msec_time_to_iso8601(sentry__msec_time()))); sentry_value_set_by_key(tx, "level", sentry_value_new_string("info")); - // TODO: add tracestate - // set up trace context so it mirrors the final json value - sentry_value_set_by_key(tx, "status", sentry_value_new_string("ok")); + sentry_value_t name = sentry_value_get_by_key(tx, "transaction"); + if (sentry_value_is_null(name) || sentry_value_get_length(name) == 0) { + sentry_value_set_by_key(tx, "transaction", + sentry_value_new_string("")); + } + // TODO: add tracestate sentry_value_t trace_context = sentry__span_get_trace_context(tx); sentry_value_t contexts = sentry_value_new_object(); sentry_value_set_by_key(contexts, "trace", trace_context); diff --git a/src/sentry_value.c b/src/sentry_value.c index c71c68ecf..fe0cde718 100644 --- a/src/sentry_value.c +++ b/src/sentry_value.c @@ -1125,12 +1125,31 @@ sentry_value_new_stacktrace(void **ips, size_t len) } sentry_value_t -sentry_value_new_transaction(const char *name, const char *operation) +sentry__value_new_span(sentry_value_t parent, const char *operation) { - sentry_value_t transaction = sentry_value_new_object(); + sentry_value_t span = sentry_value_new_object(); + + sentry_transaction_set_operation(span, operation); + sentry_value_set_by_key(span, "status", sentry_value_new_string("ok")); + + 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; +} +sentry_value_t +sentry_value_new_transaction(const char *name, const char *operation) +{ + sentry_value_t transaction + = sentry__value_new_span(sentry_value_new_null(), operation); sentry_transaction_set_name(transaction, name); - sentry_transaction_set_operation(transaction, operation); return transaction; } @@ -1138,14 +1157,8 @@ sentry_value_new_transaction(const char *name, const char *operation) void sentry_transaction_set_name(sentry_value_t transaction, const char *name) { - sentry_value_t sv_name = sentry_value_new_string(name); - // TODO: Consider doing this checking right before sending or flushing - // the transaction. - if (sentry_value_is_null(sv_name) || sentry__string_eq(name, "")) { - sentry_value_decref(sv_name); - sv_name = sentry_value_new_string(""); - } - sentry_value_set_by_key(transaction, "name", sv_name); + sentry_value_set_by_key( + transaction, "transaction", sentry_value_new_string(name)); } void diff --git a/src/sentry_value.h b/src/sentry_value.h index 2eb5004f1..cba785364 100644 --- a/src/sentry_value.h +++ b/src/sentry_value.h @@ -61,6 +61,13 @@ sentry_value_t sentry__value_new_list_with_size(size_t size); */ sentry_value_t sentry__value_new_object_with_size(size_t size); +/** + * Constructs a new Span. + * + */ +sentry_value_t sentry__value_new_span( + sentry_value_t parent, const char *operation); + /** * This will parse the Value into a UUID, or return a `nil` UUID on error. * See also `sentry_uuid_from_string`. diff --git a/tests/unit/test_tracing.c b/tests/unit/test_tracing.c index 647ae2ca8..c342e3ca4 100644 --- a/tests/unit/test_tracing.c +++ b/tests/unit/test_tracing.c @@ -43,34 +43,25 @@ SENTRY_TEST(basic_transaction) { sentry_value_t tx_cxt = sentry_value_new_transaction(NULL, NULL); TEST_CHECK(!sentry_value_is_null(tx_cxt)); - const char *tx_name - = sentry_value_as_string(sentry_value_get_by_key(tx_cxt, "name")); - TEST_CHECK_STRING_EQUAL(tx_name, ""); - const char *tx_op - = sentry_value_as_string(sentry_value_get_by_key(tx_cxt, "op")); - TEST_CHECK_STRING_EQUAL(tx_op, ""); + CHECK_STRING_PROPERTY(tx_cxt, "transaction", ""); + CHECK_STRING_PROPERTY(tx_cxt, "op", ""); sentry_value_decref(tx_cxt); tx_cxt = sentry_value_new_transaction("", ""); TEST_CHECK(!sentry_value_is_null(tx_cxt)); - tx_name = sentry_value_as_string(sentry_value_get_by_key(tx_cxt, "name")); - TEST_CHECK_STRING_EQUAL(tx_name, ""); - TEST_CHECK_STRING_EQUAL(tx_op, ""); + CHECK_STRING_PROPERTY(tx_cxt, "transaction", ""); + CHECK_STRING_PROPERTY(tx_cxt, "op", ""); sentry_value_decref(tx_cxt); tx_cxt = sentry_value_new_transaction("honk.beep", "beepbeep"); - tx_name = sentry_value_as_string(sentry_value_get_by_key(tx_cxt, "name")); - TEST_CHECK_STRING_EQUAL(tx_name, "honk.beep"); - tx_op = sentry_value_as_string(sentry_value_get_by_key(tx_cxt, "op")); - TEST_CHECK_STRING_EQUAL(tx_op, "beepbeep"); + CHECK_STRING_PROPERTY(tx_cxt, "transaction", "honk.beep"); + CHECK_STRING_PROPERTY(tx_cxt, "op", "beepbeep"); sentry_transaction_set_name(tx_cxt, ""); - tx_name = sentry_value_as_string(sentry_value_get_by_key(tx_cxt, "name")); - TEST_CHECK_STRING_EQUAL(tx_name, ""); + CHECK_STRING_PROPERTY(tx_cxt, "transaction", ""); sentry_transaction_set_operation(tx_cxt, ""); - tx_op = sentry_value_as_string(sentry_value_get_by_key(tx_cxt, "op")); - TEST_CHECK_STRING_EQUAL(tx_op, ""); + CHECK_STRING_PROPERTY(tx_cxt, "op", ""); sentry_transaction_set_sampled(tx_cxt, 1); TEST_CHECK( @@ -79,6 +70,46 @@ SENTRY_TEST(basic_transaction) sentry_value_decref(tx_cxt); } +static void +check_backfilled_name(sentry_envelope_t *envelope, void *data) +{ + uint64_t *called = data; + *called += 1; + + sentry_value_t transaction = sentry_envelope_get_transaction(envelope); + TEST_CHECK(!sentry_value_is_null(transaction)); + CHECK_STRING_PROPERTY( + transaction, "transaction", ""); + + sentry_envelope_free(envelope); +} + +SENTRY_TEST(transaction_name_backfill_on_finish) +{ + uint64_t called = 0; + + sentry_options_t *options = sentry_options_new(); + sentry_options_set_dsn(options, "https://foo@sentry.invalid/42"); + + sentry_transport_t *transport = sentry_transport_new(check_backfilled_name); + sentry_transport_set_state(transport, &called); + sentry_options_set_transport(options, transport); + + sentry_options_set_traces_sample_rate(options, 1.0); + sentry_init(options); + + sentry_value_t transaction = sentry_value_new_transaction(NULL, NULL); + sentry_transaction_start(transaction); + sentry_transaction_finish(); + + transaction = sentry_value_new_transaction("", ""); + sentry_transaction_start(transaction); + sentry_transaction_finish(); + + sentry_close(); + TEST_CHECK_INT_EQUAL(called, 2); +} + static void send_transaction_envelope_test_basic(sentry_envelope_t *envelope, void *data) { diff --git a/tests/unit/tests.inc b/tests/unit/tests.inc index 2e79a18a8..0126f0394 100644 --- a/tests/unit/tests.inc +++ b/tests/unit/tests.inc @@ -49,6 +49,7 @@ XX(session_basics) XX(slice) XX(symbolizer) XX(task_queue) +XX(transaction_name_backfill_on_finish) XX(transactions_skip_before_send) XX(transport_sampling_transactions) XX(uninitialized)