From 27213090dcc2cb3ec272c232f21d3981d8130abd 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 | 19 ++++-- src/sentry_value.c | 34 +++++++--- src/sentry_value.h | 7 ++ tests/unit/test_tracing.c | 137 ++++++++++++++++++++++++-------------- tests/unit/tests.inc | 1 + 5 files changed, 134 insertions(+), 64 deletions(-) diff --git a/src/sentry_core.c b/src/sentry_core.c index 1d9380ce9..f29575fc1 100644 --- a/src/sentry_core.c +++ b/src/sentry_core.c @@ -713,7 +713,13 @@ sentry_set_level(sentry_level_t level) sentry_value_t 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"); // TODO(tracing): stuff transaction into the scope bool should_sample = sentry__should_send_transaction(tx_cxt); @@ -733,8 +739,10 @@ sentry_transaction_start(sentry_value_t tx_cxt) tx, "trace_id", sentry_value_get_by_key_owned(tx_cxt, "trace_id")); sentry_value_set_by_key( tx, "span_id", sentry_value_get_by_key_owned(tx_cxt, "trace_id")); + 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()))); @@ -765,10 +773,13 @@ sentry_transaction_finish(sentry_value_t tx) sentry__msec_time_to_iso8601(sentry__msec_time()))); sentry_value_set_by_key(tx, "level", sentry_value_new_string("info")); - // TODO(tracing): 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 9217267d8..90f21961a 100644 --- a/src/sentry_value.c +++ b/src/sentry_value.c @@ -1124,10 +1124,32 @@ sentry_value_new_stacktrace(void **ips, size_t len) return stacktrace; } +sentry_value_t +sentry__value_new_span(sentry_value_t parent, const char *operation) +{ + sentry_value_t span = sentry_value_new_object(); + + sentry_transaction_context_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_context(const char *name, const char *operation) { - sentry_value_t transaction_context = sentry_value_new_object(); + sentry_value_t transaction_context + = sentry__value_new_span(sentry_value_new_null(), operation); + sentry_transaction_context_set_name(transaction_context, name); sentry_uuid_t trace_id = sentry_uuid_new_v4(); sentry_value_set_by_key(transaction_context, "trace_id", @@ -1147,14 +1169,8 @@ void sentry_transaction_context_set_name( sentry_value_t transaction_context, 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_context, "name", sv_name); + sentry_value_set_by_key( + transaction_context, "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 782b43d6c..103d94675 100644 --- a/tests/unit/test_tracing.c +++ b/tests/unit/test_tracing.c @@ -2,6 +2,12 @@ #include "sentry_tracing.h" #include "sentry_uuid.h" +#define IS_NULL(Src, Field) \ + sentry_value_is_null(sentry_value_get_by_key(Src, Field)) +#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(); @@ -38,46 +44,31 @@ SENTRY_TEST(basic_transaction) { sentry_value_t tx_cxt = sentry_value_new_transaction_context(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, ""); - TEST_CHECK( - !sentry_value_is_null(sentry_value_get_by_key(tx_cxt, "trace_id"))); - TEST_CHECK( - !sentry_value_is_null(sentry_value_get_by_key(tx_cxt, "span_id"))); + CHECK_STRING_PROPERTY(tx_cxt, "transaction", ""); + CHECK_STRING_PROPERTY(tx_cxt, "op", ""); + TEST_CHECK(!IS_NULL(tx_cxt, "trace_id")); + TEST_CHECK(!IS_NULL(tx_cxt, "span_id")); sentry_value_decref(tx_cxt); tx_cxt = sentry_value_new_transaction_context("", ""); 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, ""); - TEST_CHECK( - !sentry_value_is_null(sentry_value_get_by_key(tx_cxt, "trace_id"))); - TEST_CHECK( - !sentry_value_is_null(sentry_value_get_by_key(tx_cxt, "span_id"))); + CHECK_STRING_PROPERTY(tx_cxt, "transaction", ""); + CHECK_STRING_PROPERTY(tx_cxt, "op", ""); + TEST_CHECK(!IS_NULL(tx_cxt, "trace_id")); + TEST_CHECK(!IS_NULL(tx_cxt, "span_id")); sentry_value_decref(tx_cxt); tx_cxt = sentry_value_new_transaction_context("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"); - TEST_CHECK( - !sentry_value_is_null(sentry_value_get_by_key(tx_cxt, "trace_id"))); - TEST_CHECK( - !sentry_value_is_null(sentry_value_get_by_key(tx_cxt, "span_id"))); + CHECK_STRING_PROPERTY(tx_cxt, "transaction", "honk.beep"); + CHECK_STRING_PROPERTY(tx_cxt, "op", "beepbeep"); + TEST_CHECK(!IS_NULL(tx_cxt, "trace_id")); + TEST_CHECK(!IS_NULL(tx_cxt, "span_id")); sentry_transaction_context_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_context_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_context_set_sampled(tx_cxt, 1); TEST_CHECK( @@ -86,24 +77,65 @@ 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 tx = sentry_envelope_get_transaction(envelope); + TEST_CHECK(!sentry_value_is_null(tx)); + CHECK_STRING_PROPERTY(tx, "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 tx_cxt = sentry_value_new_transaction_context(NULL, NULL); + sentry_value_t tx = sentry_transaction_start(tx_cxt); + sentry_uuid_t event_id = sentry_transaction_finish(tx); + TEST_CHECK(!sentry_uuid_is_nil(&event_id)); + + tx_cxt = sentry_value_new_transaction_context("", ""); + tx = sentry_transaction_start(tx_cxt); + event_id = sentry_transaction_finish(tx); + TEST_CHECK(!sentry_uuid_is_nil(&event_id)); + + sentry_close(); + TEST_CHECK_INT_EQUAL(called, 2); +} + static void send_transaction_envelope_test_basic(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)); - const char *event_id = sentry_value_as_string( - sentry_value_get_by_key(transaction, "event_id")); + sentry_value_t tx = sentry_envelope_get_transaction(envelope); + TEST_CHECK(!sentry_value_is_null(tx)); + const char *event_id + = sentry_value_as_string(sentry_value_get_by_key(tx, "event_id")); TEST_CHECK_STRING_EQUAL(event_id, "4c035723-8638-4c3a-923f-2ab9d08b4018"); if (*called == 1) { - const char *type = sentry_value_as_string( - sentry_value_get_by_key(transaction, "type")); + const char *type + = sentry_value_as_string(sentry_value_get_by_key(tx, "type")); TEST_CHECK_STRING_EQUAL(type, "transaction"); const char *name = sentry_value_as_string( - sentry_value_get_by_key(transaction, "transaction")); + sentry_value_get_by_key(tx, "transaction")); TEST_CHECK_STRING_EQUAL(name, "honk"); } @@ -126,25 +158,25 @@ SENTRY_TEST(basic_function_transport_transaction) sentry_options_set_require_user_consent(options, true); sentry_init(options); - sentry_value_t transaction = sentry_value_new_transaction_context( + sentry_value_t tx_cxt = sentry_value_new_transaction_context( "How could you", "Don't capture this."); - transaction = sentry_transaction_start(transaction); - sentry_uuid_t event_id = sentry_transaction_finish(transaction); + sentry_value_t tx = sentry_transaction_start(tx_cxt); + sentry_uuid_t event_id = sentry_transaction_finish(tx); // TODO: `sentry_capture_event` acts as if the event was sent if user // consent was not given TEST_CHECK(!sentry_uuid_is_nil(&event_id)); sentry_user_consent_give(); - transaction = sentry_value_new_transaction_context("honk", "beep"); - transaction = sentry_transaction_start(transaction); - event_id = sentry_transaction_finish(transaction); + tx_cxt = sentry_value_new_transaction_context("honk", "beep"); + tx = sentry_transaction_start(tx_cxt); + event_id = sentry_transaction_finish(tx); TEST_CHECK(!sentry_uuid_is_nil(&event_id)); sentry_user_consent_revoke(); - transaction = sentry_value_new_transaction_context( + tx_cxt = sentry_value_new_transaction_context( "How could you again", "Don't capture this either."); - transaction = sentry_transaction_start(transaction); - event_id = sentry_transaction_finish(transaction); + tx = sentry_transaction_start(tx_cxt); + event_id = sentry_transaction_finish(tx); // TODO: `sentry_capture_event` acts as if the event was sent if user // consent was not given TEST_CHECK(!sentry_uuid_is_nil(&event_id)); @@ -171,10 +203,10 @@ SENTRY_TEST(transport_sampling_transactions) uint64_t sent_transactions = 0; for (int i = 0; i < 100; i++) { - sentry_value_t transaction + sentry_value_t tx_cxt = sentry_value_new_transaction_context("honk", "beep"); - transaction = sentry_transaction_start(transaction); - sentry_uuid_t event_id = sentry_transaction_finish(transaction); + sentry_value_t tx = sentry_transaction_start(tx_cxt); + sentry_uuid_t event_id = sentry_transaction_finish(tx); if (!sentry_uuid_is_nil(&event_id)) { sent_transactions += 1; } @@ -214,10 +246,10 @@ SENTRY_TEST(transactions_skip_before_send) sentry_options_set_before_send(options, before_send, &called_beforesend); sentry_init(options); - sentry_value_t transaction + sentry_value_t tx_cxt = sentry_value_new_transaction_context("honk", "beep"); - transaction = sentry_transaction_start(transaction); - sentry_uuid_t event_id = sentry_transaction_finish(transaction); + sentry_value_t tx = sentry_transaction_start(tx_cxt); + sentry_uuid_t event_id = sentry_transaction_finish(tx); TEST_CHECK(!sentry_uuid_is_nil(&event_id)); sentry_close(); @@ -225,3 +257,6 @@ SENTRY_TEST(transactions_skip_before_send) TEST_CHECK_INT_EQUAL(called_transport, 1); TEST_CHECK_INT_EQUAL(called_beforesend, 0); } + +#undef IS_NULL +#undef CHECK_STRING_PROPERTY diff --git a/tests/unit/tests.inc b/tests/unit/tests.inc index 3c904fa2a..c85f2ac24 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)