From fd0e0386779e9da12ff3160682c2fdf42cbac923 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Thu, 9 Dec 2021 01:58:03 -0500 Subject: [PATCH 1/6] feat(tracing): Groundwork to add tracing context to all events --- src/CMakeLists.txt | 2 ++ src/sentry_scope.c | 14 ++++++++++++-- src/sentry_scope.h | 12 ++++++++++++ src/sentry_tracing.c | 31 +++++++++++++++++++++++++++++++ src/sentry_tracing.h | 14 ++++++++++++++ tests/unit/CMakeLists.txt | 1 + tests/unit/test_tracing.c | 32 ++++++++++++++++++++++++++++++++ tests/unit/tests.inc | 1 + 8 files changed, 105 insertions(+), 2 deletions(-) create mode 100644 src/sentry_tracing.c create mode 100644 src/sentry_tracing.h create mode 100644 tests/unit/test_tracing.c diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 6dc330f8f..0510a9fe5 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -34,6 +34,8 @@ sentry_target_sources_cwd(sentry sentry_symbolizer.h sentry_sync.c sentry_sync.h + sentry_tracing.c + sentry_tracing.h sentry_transport.c sentry_transport.h sentry_utils.c diff --git a/src/sentry_scope.c b/src/sentry_scope.c index 5f8992e7e..98b550023 100644 --- a/src/sentry_scope.c +++ b/src/sentry_scope.c @@ -7,6 +7,7 @@ #include "sentry_string.h" #include "sentry_symbolizer.h" #include "sentry_sync.h" +#include "sentry_tracing.h" #include #ifdef SENTRY_BACKEND_CRASHPAD @@ -73,6 +74,7 @@ get_scope(void) g_scope.breadcrumbs = sentry_value_new_list(); g_scope.level = SENTRY_LEVEL_ERROR; g_scope.client_sdk = get_client_sdk(); + g_scope.span = sentry_value_new_null(); g_scope_initialized = true; @@ -93,6 +95,7 @@ sentry__scope_cleanup(void) sentry_value_decref(g_scope.contexts); sentry_value_decref(g_scope.breadcrumbs); sentry_value_decref(g_scope.client_sdk); + sentry_value_decref(g_scope.span); } sentry__mutex_unlock(&g_lock); } @@ -115,7 +118,7 @@ sentry__scope_flush_unlock() { sentry__scope_unlock(); SENTRY_WITH_OPTIONS (options) { - // we try to unlock the scope/session lock as soon as possible. The + // we try to unlock the scope as soon as possible. The // backend will do its own `WITH_SCOPE` internally. if (options->backend && options->backend->flush_scope_func) { options->backend->flush_scope_func(options->backend, options); @@ -269,7 +272,14 @@ sentry__scope_apply_to_event(const sentry_scope_t *scope, // TODO: these should merge PLACE_CLONED_VALUE("tags", scope->tags); PLACE_CLONED_VALUE("extra", scope->extra); - PLACE_CLONED_VALUE("contexts", scope->contexts); + + // TODO: better, more thorough deep merging + sentry_value_t contexts = sentry__value_clone(scope->contexts); + sentry_value_t trace = sentry__span_get_trace_context(scope->span); + if (!sentry_value_is_null(trace)) { + sentry_value_set_by_key(contexts, "trace", trace); + } + PLACE_VALUE("contexts", contexts); if (mode & SENTRY_SCOPE_BREADCRUMBS) { PLACE_CLONED_VALUE("breadcrumbs", scope->breadcrumbs); diff --git a/src/sentry_scope.h b/src/sentry_scope.h index 250ff8200..393cf349d 100644 --- a/src/sentry_scope.h +++ b/src/sentry_scope.h @@ -19,6 +19,12 @@ typedef struct sentry_scope_s { sentry_value_t breadcrumbs; sentry_level_t level; sentry_value_t client_sdk; + // Not to be confused with transaction, which is a legacy value. This is + // also known as a transaction, but to maintain consistency with other SDKs + // and to avoid a conflict with the existing transaction field this is named + // span. Whenever possible, `transaction` should pull its value from the + // `name` property nested in this field. + sentry_value_t span; } sentry_scope_t; /** @@ -69,6 +75,12 @@ void sentry__scope_apply_to_event(const sentry_scope_t *scope, const sentry_options_t *options, sentry_value_t event, sentry_scope_mode_t mode); +/** + * Sets the span (actually transaction) on the scope. An internal way to pass + * around contextual information needed from a transaction into other events. + */ +void sentry__scope_set_span(sentry_value_t span); + /** * These are convenience macros to automatically lock/unlock a scope inside a * code block. diff --git a/src/sentry_tracing.c b/src/sentry_tracing.c new file mode 100644 index 000000000..f8650c85b --- /dev/null +++ b/src/sentry_tracing.c @@ -0,0 +1,31 @@ +#include "sentry_sync.h" + +sentry_value_t +sentry__span_get_trace_context(sentry_value_t span) +{ + if (sentry_value_is_null(span) + || sentry_value_is_null(sentry_value_get_by_key(span, "trace_id")) + || sentry_value_is_null(sentry_value_get_by_key(span, "span_id"))) { + return sentry_value_new_null(); + } + + sentry_value_t trace_context = sentry_value_new_object(); + +#define PLACE_VALUE(Key, Source) \ + do { \ + sentry_value_t src = sentry_value_get_by_key(Source, Key); \ + if (!sentry_value_is_null(src)) { \ + sentry_value_incref(src); \ + sentry_value_set_by_key(trace_context, Key, src); \ + } \ + } while (0) + + PLACE_VALUE("trace_id", span); + PLACE_VALUE("span_id", span); + PLACE_VALUE("parent_span_id", span); + PLACE_VALUE("op", span); + PLACE_VALUE("description", span); + PLACE_VALUE("status", span); + + return trace_context; +} diff --git a/src/sentry_tracing.h b/src/sentry_tracing.h new file mode 100644 index 000000000..d99afe828 --- /dev/null +++ b/src/sentry_tracing.h @@ -0,0 +1,14 @@ +#ifndef SENTRY_TRACING_H_INCLUDED +#define SENTRY_TRACING_H_INCLUDED + +#include "sentry_boot.h" +#include "sentry_value.h" + +/** + * Returns an object containing tracing information extracted from a + * transaction (/span) which should be included in an event. + * See https://develop.sentry.dev/sdk/event-payloads/transaction/#examples + */ +sentry_value_t sentry__span_get_trace_context(sentry_value_t span); + +#endif diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index 86ffecaff..91b908ba9 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -33,6 +33,7 @@ add_executable(sentry_test_unit test_slice.c test_symbolizer.c test_sync.c + test_tracing.c test_uninit.c test_unwinder.c test_utils.c diff --git a/tests/unit/test_tracing.c b/tests/unit/test_tracing.c new file mode 100644 index 000000000..7ddda29b5 --- /dev/null +++ b/tests/unit/test_tracing.c @@ -0,0 +1,32 @@ +#include "sentry_testsupport.h" +#include "sentry_tracing.h" +#include "sentry_uuid.h" + +SENTRY_TEST(basic_tracing_context) +{ + sentry_value_t span = sentry_value_new_object(); + TEST_CHECK(sentry_value_is_null(sentry__span_get_trace_context(span))); + + sentry_value_set_by_key(span, "op", sentry_value_new_string("honk.beep")); + TEST_CHECK(sentry_value_is_null(sentry__span_get_trace_context(span))); + + sentry_uuid_t trace_id = sentry_uuid_new_v4(); + sentry_value_set_by_key( + span, "trace_id", sentry__value_new_internal_uuid(&trace_id)); + TEST_CHECK(sentry_value_is_null(sentry__span_get_trace_context(span))); + + sentry_uuid_t span_id = sentry_uuid_new_v4(); + sentry_value_set_by_key( + span, "span_id", sentry__value_new_span_uuid(&span_id)); + + sentry_value_t trace_context = sentry__span_get_trace_context(span); + TEST_CHECK(!sentry_value_is_null(trace_context)); + TEST_CHECK(!sentry_value_is_null( + sentry_value_get_by_key(trace_context, "trace_id"))); + TEST_CHECK(!sentry_value_is_null( + sentry_value_get_by_key(trace_context, "span_id"))); + + char *span_op + = sentry_value_as_string(sentry_value_get_by_key(trace_context, "op")); + TEST_CHECK_STRING_EQUAL(span_op, "honk.beep"); +} diff --git a/tests/unit/tests.inc b/tests/unit/tests.inc index 51615be37..5ba245f18 100644 --- a/tests/unit/tests.inc +++ b/tests/unit/tests.inc @@ -4,6 +4,7 @@ XX(basic_function_transport) XX(basic_http_request_preparation_for_event) XX(basic_http_request_preparation_for_event_with_attachment) XX(basic_http_request_preparation_for_minidump) +XX(basic_tracing_context) XX(buildid_fallback) XX(concurrent_init) XX(count_sampled_events) From d5c202be2abf8e282fc0760f8fbbfb21d72ee7cb Mon Sep 17 00:00:00 2001 From: Betty Da Date: Thu, 9 Dec 2021 02:17:36 -0500 Subject: [PATCH 2/6] fix leaks --- src/sentry_scope.c | 3 +++ tests/unit/test_tracing.c | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/sentry_scope.c b/src/sentry_scope.c index 98b550023..8b7033baf 100644 --- a/src/sentry_scope.c +++ b/src/sentry_scope.c @@ -278,8 +278,11 @@ sentry__scope_apply_to_event(const sentry_scope_t *scope, sentry_value_t trace = sentry__span_get_trace_context(scope->span); if (!sentry_value_is_null(trace)) { sentry_value_set_by_key(contexts, "trace", trace); + } else { + sentry_value_decref(trace); } PLACE_VALUE("contexts", contexts); + sentry_value_decref(contexts); if (mode & SENTRY_SCOPE_BREADCRUMBS) { PLACE_CLONED_VALUE("breadcrumbs", scope->breadcrumbs); diff --git a/tests/unit/test_tracing.c b/tests/unit/test_tracing.c index 7ddda29b5..34cb331c9 100644 --- a/tests/unit/test_tracing.c +++ b/tests/unit/test_tracing.c @@ -26,7 +26,10 @@ SENTRY_TEST(basic_tracing_context) TEST_CHECK(!sentry_value_is_null( sentry_value_get_by_key(trace_context, "span_id"))); - char *span_op + const char *span_op = sentry_value_as_string(sentry_value_get_by_key(trace_context, "op")); TEST_CHECK_STRING_EQUAL(span_op, "honk.beep"); + + sentry_value_decref(trace_context); + sentry_value_decref(span); } From f61f237c695633ca333f80be44e9d90241e28433 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Thu, 9 Dec 2021 02:27:04 -0500 Subject: [PATCH 3/6] forgot the implementation --- src/sentry_scope.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/sentry_scope.c b/src/sentry_scope.c index 8b7033baf..f2bc14aa7 100644 --- a/src/sentry_scope.c +++ b/src/sentry_scope.c @@ -227,6 +227,12 @@ sentry__symbolize_stacktrace(sentry_value_t stacktrace) } } +void +sentry__scope_set_span(sentry_value_t span) +{ + return; +} + void sentry__scope_apply_to_event(const sentry_scope_t *scope, const sentry_options_t *options, sentry_value_t event, From 2b4b705efd713cae200a9d4c7ec5ce9a0ea30990 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Thu, 9 Dec 2021 21:21:46 -0500 Subject: [PATCH 4/6] deal with unused param for now --- src/sentry_scope.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/sentry_scope.c b/src/sentry_scope.c index f2bc14aa7..63de87e3e 100644 --- a/src/sentry_scope.c +++ b/src/sentry_scope.c @@ -230,6 +230,8 @@ sentry__symbolize_stacktrace(sentry_value_t stacktrace) void sentry__scope_set_span(sentry_value_t span) { + // TODO: implement this function and get rid of this line. + (void)span; return; } From def13aeef966cffdb934276490775121f5886f94 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Thu, 9 Dec 2021 21:32:03 -0500 Subject: [PATCH 5/6] no need to free a null --- src/sentry_scope.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/sentry_scope.c b/src/sentry_scope.c index 63de87e3e..fe42e3e8c 100644 --- a/src/sentry_scope.c +++ b/src/sentry_scope.c @@ -286,8 +286,6 @@ sentry__scope_apply_to_event(const sentry_scope_t *scope, sentry_value_t trace = sentry__span_get_trace_context(scope->span); if (!sentry_value_is_null(trace)) { sentry_value_set_by_key(contexts, "trace", trace); - } else { - sentry_value_decref(trace); } PLACE_VALUE("contexts", contexts); sentry_value_decref(contexts); From 1751cdb78ee7e78112ab063c594c7eafb0eb9c43 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Thu, 9 Dec 2021 21:33:57 -0500 Subject: [PATCH 6/6] properly bookend #defines --- src/sentry_scope.c | 4 +++- src/sentry_tracing.c | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/sentry_scope.c b/src/sentry_scope.c index fe42e3e8c..5052f023f 100644 --- a/src/sentry_scope.c +++ b/src/sentry_scope.c @@ -307,7 +307,9 @@ sentry__scope_apply_to_event(const sentry_scope_t *scope, sentry__foreach_stacktrace(event, sentry__symbolize_stacktrace); } +#undef PLACE_CLONED_VALUE +#undef PLACE_VALUE #undef PLACE_STRING -#undef IS_NULL #undef SET +#undef IS_NULL } diff --git a/src/sentry_tracing.c b/src/sentry_tracing.c index f8650c85b..73e23c0d8 100644 --- a/src/sentry_tracing.c +++ b/src/sentry_tracing.c @@ -28,4 +28,6 @@ sentry__span_get_trace_context(sentry_value_t span) PLACE_VALUE("status", span); return trace_context; + +#undef PLACE_VALUE }