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.

Sign up for GitHub

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: Implement distributed trace propagation (NATIVE-304) #657

Merged
merged 2 commits into from
Jan 18, 2022
Merged

feat: Implement distributed trace propagation (NATIVE-304) #657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e7455c2
feat: Implement trace propagation (NATIVE-304)
Swatinem Jan 14, 2022
fb91cda
return void
Swatinem Jan 17, 2022
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
35 changes: 35 additions & 0 deletions include/sentry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1317,6 +1317,17 @@ SENTRY_EXPERIMENTAL_API void sentry_transaction_context_set_sampled(
SENTRY_EXPERIMENTAL_API void sentry_transaction_context_remove_sampled(
sentry_transaction_context_t *tx_cxt);

/**
* Update the Transaction Context with the given HTTP header key/value pair.
*
* This is used to propagate distributed tracing metadata from upstream
* services. Therefore, the headers of incoming requests should be fed into this
* function so that sentry is able to continue a trace that was started by an
* upstream service.
*/
SENTRY_EXPERIMENTAL_API void sentry_transaction_context_update_from_header(
sentry_transaction_context_t *tx_cxt, const char *key, const char *value);

/**
* Starts a new Transaction based on the provided context, restored from an
* external integration (i.e. a span from a different SDK) or manually
Expand Down Expand Up @@ -1553,6 +1564,30 @@ SENTRY_EXPERIMENTAL_API void sentry_span_set_status(
SENTRY_EXPERIMENTAL_API void sentry_transaction_set_status(
sentry_transaction_t *tx, sentry_span_status_t status);

/**
* Type of the `iter_headers` callback.
*
* The callback is being called with HTTP header key/value pairs.
* These headers can be attached to outgoing HTTP requests to propagate
* distributed tracing metadata to downstream services.
*
*/
typedef void (*sentry_iter_headers_function_t)(
const char *key, const char *value, void *userdata);

/**
* Iterates the distributed tracing HTTP headers for the given span.
*/
SENTRY_EXPERIMENTAL_API void sentry_span_iter_headers(sentry_span_t *span,
sentry_iter_headers_function_t callback, void *userdata);

/**
* Iterates the distributed tracing HTTP headers for the given transaction.
*/
SENTRY_EXPERIMENTAL_API void sentry_transaction_iter_headers(
sentry_transaction_t *tx, sentry_iter_headers_function_t callback,
void *userdata);
relaxolotl marked this conversation as resolved.
Show resolved Hide resolved

#endif

#ifdef __cplusplus
Expand Down
75 changes: 75 additions & 0 deletions src/sentry_tracing.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,47 @@ sentry_transaction_context_remove_sampled(sentry_transaction_context_t *tx_cxt)
sentry_value_remove_by_key(tx_cxt->inner, "sampled");
}

void
sentry_transaction_context_update_from_header(
sentry_transaction_context_t *tx_cxt, const char *key, const char *value)
{
if (!sentry__string_eq(key, "sentry-trace")) {
return;
}

// https://develop.sentry.dev/sdk/performance/#header-sentry-trace
// sentry-trace = traceid-spanid(-sampled)?
const char *trace_id_start = value;
const char *trace_id_end = strchr(trace_id_start, '-');
if (!trace_id_end) {
return;
}

sentry_value_t inner = tx_cxt->inner;

char *s
= sentry__string_clonen(trace_id_start, trace_id_end - trace_id_start);
sentry_value_t trace_id = sentry__value_new_string_owned(s);
sentry_value_set_by_key(inner, "trace_id", trace_id);

const char *span_id_start = trace_id_end + 1;
const char *span_id_end = strchr(span_id_start, '-');
if (!span_id_end) {
// no sampled flag
sentry_value_t parent_span_id = sentry_value_new_string(span_id_start);
sentry_value_set_by_key(inner, "parent_span_id", parent_span_id);
return;
}
// else: we have a sampled flag

s = sentry__string_clonen(span_id_start, span_id_end - span_id_start);
sentry_value_t parent_span_id = sentry__value_new_string_owned(s);
sentry_value_set_by_key(inner, "parent_span_id", parent_span_id);

bool sampled = *(span_id_end + 1) == '1';
sentry_value_set_by_key(inner, "sampled", sentry_value_new_bool(sampled));
}

sentry_transaction_t *
sentry__transaction_new(sentry_value_t inner)
{
Expand Down Expand Up @@ -431,3 +472,37 @@ sentry_transaction_set_status(
{
set_status(tx->inner, status);
}

static void
sentry__span_iter_headers(sentry_value_t span,
sentry_iter_headers_function_t callback, void *userdata)
{
sentry_value_t trace_id = sentry_value_get_by_key(span, "trace_id");
sentry_value_t span_id = sentry_value_get_by_key(span, "span_id");
sentry_value_t sampled = sentry_value_get_by_key(span, "sampled");

if (sentry_value_is_null(trace_id) || sentry_value_is_null(span_id)) {
return;
}

char buf[64];
snprintf(buf, sizeof(buf), "%s-%s-%s", sentry_value_as_string(trace_id),
sentry_value_as_string(span_id),
sentry_value_is_true(sampled) ? "1" : "0");
Comment on lines +488 to +491
Copy link
Contributor

Choose a reason for hiding this comment

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

is this guaranteed not to be more that 64 chars? I mean really shouldn't the return code of snprintf be checked?

Copy link
Member Author

Choose a reason for hiding this comment

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

pretty much, yes. trace_id is 32 chars, span_id is 16. Even if for whatever reason they exceed that limit, it wouldn’t be "unsafe", just generate an invalid value. IMO not worth it, we do not check the return value of this in quite a bunch of places.


callback("sentry-trace", buf, userdata);
}

void
sentry_span_iter_headers(sentry_span_t *span,
sentry_iter_headers_function_t callback, void *userdata)
{
sentry__span_iter_headers(span->inner, callback, userdata);
}

void
sentry_transaction_iter_headers(sentry_transaction_t *tx,
sentry_iter_headers_function_t callback, void *userdata)
{
sentry__span_iter_headers(tx->inner, callback, userdata);
}
98 changes: 97 additions & 1 deletion tests/unit/test_tracing.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#include "sentry_scope.h"
#include "sentry_testsupport.h"

#include "sentry_scope.h"
#include "sentry_string.h"
#include "sentry_tracing.h"
#include "sentry_uuid.h"

Expand Down Expand Up @@ -663,5 +665,99 @@ SENTRY_TEST(drop_unfinished_spans)
TEST_CHECK_INT_EQUAL(called_transport, 1);
}

static void
forward_headers_to(const char *key, const char *value, void *userdata)
{
sentry_transaction_context_t *tx_ctx
= (sentry_transaction_context_t *)userdata;

sentry_transaction_context_update_from_header(tx_ctx, key, value);
}

SENTRY_TEST(distributed_headers)
{
sentry_options_t *options = sentry_options_new();
sentry_options_set_dsn(options, "https://[email protected]/42");

sentry_options_set_traces_sample_rate(options, 1.0);
sentry_options_set_max_spans(options, 2);
sentry_init(options);

sentry_transaction_context_t *tx_ctx
= sentry_transaction_context_new("wow!", NULL);
sentry_transaction_t *tx = sentry_transaction_start(tx_ctx);

const char *trace_id = sentry_value_as_string(
sentry_value_get_by_key(tx->inner, "trace_id"));
TEST_CHECK(!sentry__string_eq(trace_id, ""));

const char *span_id
= sentry_value_as_string(sentry_value_get_by_key(tx->inner, "span_id"));
TEST_CHECK(!sentry__string_eq(span_id, ""));

// check transaction
tx_ctx = sentry_transaction_context_new("distributed!", NULL);
sentry_transaction_iter_headers(tx, forward_headers_to, (void *)tx_ctx);
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like a fairly unconventional signature given what i understand of the intent of the api's design: the way i read it was that you would use the headers getter/setter like so, using this test as an example:

headers = sentry_transaction_iter_headers(tx);
continuing_transaction = sentry_transaction_continue_from_headers(sentrytrace_header);

this appears to have pushed aside the idea of returning some sort of map in favour of a more "iterable"-like api. is there a particular reason why that decision was made? does it make using this more natural over returning some sort of map?

stepping outside of the scope of the test itself, the return value is presumably going to be attached to the headers of requests (i.e. events and transactions) sent to sentry, so would this api still feel natural once we add that functionality?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, maps don’t exist in C, neither do "standard" iterators.
We have a map in sentry_value_t, but as @flub noticed, we can’t even iterate over its keys. Also having API users go through constructing a sentry_value_t might be a bit inconvenient.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the generic alternative would be to define a struct with key, value, next and force people to populate that. but really it's no better than this way i guess, passing the userdata around saves some conversions in favour of more function calls.

sentry_transaction_t *dist_tx = sentry_transaction_start(tx_ctx);

const char *dist_trace_id = sentry_value_as_string(
sentry_value_get_by_key(dist_tx->inner, "trace_id"));
TEST_CHECK_STRING_EQUAL(dist_trace_id, trace_id);

const char *parent_span_id = sentry_value_as_string(
sentry_value_get_by_key(dist_tx->inner, "parent_span_id"));
TEST_CHECK_STRING_EQUAL(parent_span_id, span_id);

sentry__transaction_decref(dist_tx);

// check span
sentry_span_t *child = sentry_transaction_start_child(tx, "honk", "goose");

span_id = sentry_value_as_string(
sentry_value_get_by_key(child->inner, "span_id"));
TEST_CHECK(!sentry__string_eq(span_id, ""));

tx_ctx = sentry_transaction_context_new("distributed!", NULL);
sentry_span_iter_headers(child, forward_headers_to, (void *)tx_ctx);
dist_tx = sentry_transaction_start(tx_ctx);

dist_trace_id = sentry_value_as_string(
sentry_value_get_by_key(dist_tx->inner, "trace_id"));
TEST_CHECK_STRING_EQUAL(dist_trace_id, trace_id);

parent_span_id = sentry_value_as_string(
sentry_value_get_by_key(dist_tx->inner, "parent_span_id"));
TEST_CHECK_STRING_EQUAL(parent_span_id, span_id);

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);

// check sampled flag
tx_ctx = sentry_transaction_context_new("wow!", NULL);
sentry_transaction_context_set_sampled(tx_ctx, 0);
tx = sentry_transaction_start(tx_ctx);

tx_ctx = sentry_transaction_context_new("distributed!", NULL);
sentry_transaction_iter_headers(tx, forward_headers_to, (void *)tx_ctx);
dist_tx = sentry_transaction_start(tx_ctx);

TEST_CHECK(!sentry_value_is_true(
sentry_value_get_by_key(dist_tx->inner, "sampled")));

sentry__transaction_decref(dist_tx);

// 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!
Comment on lines +753 to +755
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm a little confused as to how this would make for a good use case to carry forward the sampled field into a child span. if it's purely for testing purposes, i can sort of see the argument. it seems like we could just expose unit test-exclusive functionality to check this, though.

if i were to consider the cases outside of the unit test, i'll admit that it's still not too clear to me how and where unsampled spans will be used. i do recognize the fact that other SDKs do still construct spans even if they're unsampled, and perhaps that's enough of an argument to do so.

from the perspective of this feature (distributed trace propagation), a need to continue a trace from an unsampled span and not an unsampled transaction is what's required to make constructing unsampled spans useful. however, it's not clear to me what sort of situations would form the basis of that requirement. are users directly grabbing headers from a span? do we have instrumentation in this SDK that requires grabbing headers from spans?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://develop.sentry.dev/sdk/performance/#propagation

A transaction's sampling decision should be passed to all of its children, including across service boundaries.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure; the sampling decision does get implicitly propagated by not constructing spans if they're not sampled in the native SDK right now. the only scenario where the native SDK doesn't work is if we're continuing an unsampled span across service boundaries.

we can continue unsampled transactions across service boundaries in the native SDK, and the one place where you would normally look for a transaction to iter_headers off of would be in the scope, which currently exclusively stores a transaction.

do we need to continue unsampled spans? do you ever need to invoke iter_headers on a span and not a transaction, ie is there an actual use case for calling iter_headers on a sentry_span_t?

Copy link
Contributor

Choose a reason for hiding this comment

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

either way we could probably retroactively add this in if it's needed, ie it really doesn't affect the primary purpose of this PR (unless not having this straight up breaks distributed tracing but we'd fix that in a follow-up PR anyways)

Copy link
Member Author

Choose a reason for hiding this comment

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

is there an actual use case for calling iter_headers on a sentry_span_t?

absolutely! You attach the headers (continue the trace) from whereever you currently are.


sentry__transaction_decref(tx);

sentry_close();
}

#undef IS_NULL
#undef CHECK_STRING_PROPERTY
1 change: 1 addition & 0 deletions tests/unit/tests.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ XX(concurrent_init)
XX(concurrent_uninit)
XX(count_sampled_events)
XX(custom_logger)
XX(distributed_headers)
XX(drop_unfinished_spans)
XX(dsn_parsing_complete)
XX(dsn_parsing_invalid)
Expand Down