Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(tracing): Groundwork to add tracing context to all events #617

Merged
merged 6 commits into from
Dec 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
27 changes: 24 additions & 3 deletions src/sentry_scope.c
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "sentry_string.h"
#include "sentry_symbolizer.h"
#include "sentry_sync.h"
#include "sentry_tracing.h"
#include <stdlib.h>

#ifdef SENTRY_BACKEND_CRASHPAD
Expand Down Expand Up @@ -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;

Expand All @@ -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);
}
Expand All @@ -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);
Expand Down Expand Up @@ -224,6 +227,14 @@ 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;
}

void
sentry__scope_apply_to_event(const sentry_scope_t *scope,
const sentry_options_t *options, sentry_value_t event,
Expand Down Expand Up @@ -269,7 +280,15 @@ 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);
sentry_value_decref(contexts);

if (mode & SENTRY_SCOPE_BREADCRUMBS) {
PLACE_CLONED_VALUE("breadcrumbs", scope->breadcrumbs);
Expand All @@ -288,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
}
12 changes: 12 additions & 0 deletions src/sentry_scope.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description is very nice! Maybe name this root_span or something? Not sure what would make this less confusing.
Also not sure if you want to save the innermost span somewhere as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

truth be told i'm actually not the biggest fan of calling these spans as there are subtle differences between a transaction and a span. the nice thing about using span here is mostly that it's consistent with the naming scheme used by other SDKs (see https://github.com/getsentry/sentry-python/blob/dd0efc08414ee2ef1a5f22d2cc4e243b54a1b455/sentry_sdk/scope.py#L81-L83).

given that, i'm reluctant to use root_span because in theory, this "span" should be a transaction which means it actually should contain some sort of pointer to a span tree(s). there have been suggestions to call this something like transaction_object, which makes it less of a misnomer at the cost of giving up consistency. thoughts?

i think the question you have about the innermost span might be indirectly answered by the second paragraph: the stored "span", aka the transaction should have enough info to find any given span that branches off of it if needed. i'll admit however that i don't have a very good vision of how span tree construction and management will look like yet, and i'm leaving that as a challenge to be solved a little later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on transaction_object, why not. The comment can mention that other SDKs call this simply span it is remains greppable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's also a setter on scope that the spec explicitly requires to be named set_span: https://develop.sentry.dev/sdk/performance/#scope-changes

given that this setter is meant to be internal and this suggested change's intent is to make things easier for us internally, i'd like to do this: i'm going to merge this as-is while it still uses the terminology as required by the spec and as used by all of the other SDKs. a follow-up PR can be opened up which introduces this rename of span to transaction_object in the scope in addition to the introduction of a set_transaction_object() helper to mirror the rename. we can then assess whether the rename works or makes sense or not in there.

} sentry_scope_t;

/**
Expand Down Expand Up @@ -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.
Expand Down
33 changes: 33 additions & 0 deletions src/sentry_tracing.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
#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) \
relaxolotl marked this conversation as resolved.
Show resolved Hide resolved
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;

#undef PLACE_VALUE
}
14 changes: 14 additions & 0 deletions src/sentry_tracing.h
Original file line number Diff line number Diff line change
@@ -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
1 change: 1 addition & 0 deletions tests/unit/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
35 changes: 35 additions & 0 deletions tests/unit/test_tracing.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#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")));

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);
}
1 change: 1 addition & 0 deletions tests/unit/tests.inc
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down