feat: Implement distributed trace propagation (NATIVE-304) #657
Conversation
This implements the `iter_headers` and something analogoes to the `continue_from_headers` APIs. C does not make is easy to consume an iterator, so I went with a slightly different naming.
|
|
||
| // check transaction | ||
| tx_ctx = sentry_transaction_context_new("distributed!", NULL); | ||
| sentry_transaction_iter_headers(tx, forward_headers_to, (void *)tx_ctx); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // 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! |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
https://develop.sentry.dev/sdk/performance/#propagation
A transaction's sampling decision should be passed to all of its children, including across service boundaries.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
is there an actual use case for calling
iter_headerson asentry_span_t?
absolutely! You attach the headers (continue the trace) from whereever you currently are.
| 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"); |
There was a problem hiding this comment.
is this guaranteed not to be more that 64 chars? I mean really shouldn't the return code of snprintf be checked?
There was a problem hiding this comment.
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.
|
|
||
| // check transaction | ||
| tx_ctx = sentry_transaction_context_new("distributed!", NULL); | ||
| sentry_transaction_iter_headers(tx, forward_headers_to, (void *)tx_ctx); |
There was a problem hiding this comment.
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.
This implements the
iter_headersand something analogous to thecontinue_from_headersAPIs.C does not make is easy to consume an iterator, so I went with a slightly different naming.