-
-
Notifications
You must be signed in to change notification settings - Fork 192
(WIP) feat: traceparent
header support
#1394
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
base: master
Are you sure you want to change the base?
Conversation
// Test case-insensitive header parsing | ||
const char *header_variants[] | ||
= { "traceparent", "TRACEPARENT", "TraceParent", "TrAcEpArEnT", NULL }; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be any as described on https://www.w3.org/TR/trace-context/#header-name
@sentry review |
|
||
sentry_value_t inner = tx_ctx->inner; | ||
|
||
char *s = sentry__string_clone_n( | ||
trace_id_start, (size_t)(trace_id_end - trace_id_start)); | ||
if (!is_valid_trace_id(s)) { | ||
sentry_free(s); | ||
return; | ||
} | ||
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); | ||
if (!is_valid_span_id(sentry_value_as_string(parent_span_id))) { | ||
sentry_value_decref(parent_span_id); | ||
return; | ||
} | ||
sentry_value_set_by_key( | ||
inner, "parent_span_id", parent_span_id); | ||
return; | ||
} | ||
// else: we have a sampled flag | ||
|
||
s = sentry__string_clone_n( | ||
span_id_start, (size_t)(span_id_end - span_id_start)); | ||
if (!is_valid_span_id(s)) { | ||
sentry_free(s); | ||
return; | ||
} | ||
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)); | ||
return; | ||
} | ||
sentry_value_set_by_key(inner, "parent_span_id", parent_span_id); | ||
return; | ||
} | ||
// else: we have a sampled flag | ||
|
||
s = sentry__string_clone_n( | ||
span_id_start, (size_t)(span_id_end - span_id_start)); | ||
if (!is_valid_span_id(s)) { | ||
sentry_free(s); | ||
return; | ||
} | ||
sentry_value_t parent_span_id = sentry__value_new_string_owned(s); | ||
sentry_value_set_by_key(inner, "parent_span_id", parent_span_id); | ||
// Check for traceparent header: 00-<traceId>-<spanId>-<sampled> | ||
const char traceparent[] = "traceparent"; | ||
const size_t traceparent_len = sizeof(traceparent) - 1; | ||
if (key_len == traceparent_len) { | ||
bool is_traceparent = true; | ||
for (size_t i = 0; i < traceparent_len; i++) { | ||
if (tolower(key[i]) != traceparent[i]) { | ||
is_traceparent = false; | ||
break; | ||
} | ||
} | ||
|
||
bool sampled = *(span_id_end + 1) == '1'; | ||
sentry_value_set_by_key(inner, "sampled", sentry_value_new_bool(sampled)); | ||
if (is_traceparent) { | ||
// Parse W3C traceparent header: 00-<traceId>-<spanId>-<flags> | ||
if (value_len < 55) { // Minimum length: 00-32char-16char-02char | ||
SENTRY_WARN("invalid traceparent format: too short"); | ||
return; | ||
} | ||
|
||
// Check version | ||
if (value[0] != '0' || value[1] != '0' || value[2] != '-') { | ||
SENTRY_WARN("invalid traceparent format: unsupported version " | ||
"or missing delimiter"); | ||
return; | ||
} | ||
|
||
// Extract trace ID (32 hex chars) | ||
const char *trace_id_start = value + 3; | ||
if (value[35] != '-') { | ||
SENTRY_WARN("invalid traceparent format: missing delimiter " | ||
"after trace ID"); | ||
return; | ||
} | ||
|
||
char *trace_id_str = sentry__string_clone_n(trace_id_start, 32); | ||
if (!is_valid_trace_id(trace_id_str)) { | ||
sentry_free(trace_id_str); | ||
return; | ||
} | ||
|
||
// Extract span ID (16 hex chars) | ||
const char *span_id_start = value + 36; | ||
if (value[52] != '-') { | ||
SENTRY_WARN("invalid traceparent format: missing delimiter " | ||
"after span ID"); | ||
sentry_free(trace_id_str); | ||
return; | ||
} | ||
|
||
char *span_id_str = sentry__string_clone_n(span_id_start, 16); | ||
if (!is_valid_span_id(span_id_str)) { | ||
sentry_free(trace_id_str); | ||
sentry_free(span_id_str); | ||
return; | ||
} | ||
|
||
// Extract flags (2 hex chars) | ||
const char *flags_start = value + 53; | ||
if (value_len < 55) { | ||
SENTRY_WARN("invalid traceparent format: missing flags"); | ||
sentry_free(trace_id_str); | ||
sentry_free(span_id_str); | ||
return; | ||
} | ||
|
||
// Parse sampled flag from the last bit of flags | ||
char flags_str[3] = { flags_start[0], flags_start[1], '\0' }; | ||
unsigned int flags_value = 0; | ||
if (sscanf(flags_str, "%02x", &flags_value) != 1) { | ||
SENTRY_WARN("invalid traceparent format: invalid flags"); | ||
sentry_free(trace_id_str); | ||
sentry_free(span_id_str); | ||
return; | ||
} | ||
|
||
bool sampled = (flags_value & 0x01) != 0; | ||
|
||
sentry_value_t inner = tx_ctx->inner; | ||
sentry_value_set_by_key(inner, "trace_id", | ||
sentry__value_new_string_owned(trace_id_str)); | ||
sentry_value_set_by_key(inner, "parent_span_id", | ||
sentry__value_new_string_owned(span_id_str)); | ||
sentry_value_set_by_key( | ||
inner, "sampled", sentry_value_new_bool(sampled)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nested if statements for header parsing can be refactored to improve code readability and maintainability. Consider extracting separate functions for parsing sentry-trace and traceparent headers to reduce the cyclomatic complexity of this function.
Did we get this right? 👍 / 👎 to inform future reviews.
if (options->propagate_traceparent) { | ||
char traceparent[64]; | ||
snprintf(traceparent, sizeof(traceparent), "00-%s-%s-%s", | ||
sentry_value_as_string(trace_id), | ||
sentry_value_as_string(span_id), | ||
sentry_value_is_true(sampled) ? "01" : "00"); | ||
callback("traceparent", traceparent, userdata); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The traceparent header generation code uses a hardcoded buffer size of 64 characters. Consider using a named constant and validating that the generated string doesn't exceed the buffer size for better safety.
Did we get this right? 👍 / 👎 to inform future reviews.
bool is_sentry_trace = true; | ||
for (size_t i = 0; i < sentry_trace_len; i++) { | ||
if (tolower(key[i]) != sentry_trace[i]) { | ||
is_sentry_trace = false; | ||
break; | ||
} | ||
} | ||
} | ||
|
||
// 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 = memchr(trace_id_start, '-', value_len); | ||
if (!trace_id_end) { | ||
SENTRY_WARN("invalid trace id format in given header"); | ||
return; | ||
} | ||
|
||
sentry_value_t inner = tx_ctx->inner; | ||
|
||
char *s = sentry__string_clone_n( | ||
trace_id_start, (size_t)(trace_id_end - trace_id_start)); | ||
if (!is_valid_trace_id(s)) { | ||
sentry_free(s); | ||
return; | ||
} | ||
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); | ||
if (!is_valid_span_id(sentry_value_as_string(parent_span_id))) { | ||
sentry_value_decref(parent_span_id); | ||
if (is_sentry_trace) { | ||
// Parse sentry-trace header: traceid-spanid(-sampled)? | ||
const char *trace_id_start = value; | ||
const char *trace_id_end = memchr(trace_id_start, '-', value_len); | ||
if (!trace_id_end) { | ||
SENTRY_WARN("invalid trace id format in given header"); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The case-insensitive header comparison logic is duplicated for both sentry-trace and traceparent headers. Consider extracting this into a helper function to reduce code duplication.
Did we get this right? 👍 / 👎 to inform future reviews.
Fixes #1350
Develop Docs:
propagatetraceparent
w3c-trace-context-header