From 4746ccafc7147c459b42232af7dfbcb9270d0436 Mon Sep 17 00:00:00 2001 From: bduranleau-nr <106178551+bduranleau-nr@users.noreply.github.com> Date: Thu, 8 Feb 2024 17:45:43 -0600 Subject: [PATCH] fix(axiom): handle locale for dt w3c priority (#828) Account for locale differences when generating the DT w3c tracestate header. Mitigates a risk against unexpected locale settings using `,` rather than `.` for the decimal priority value breaking the PHP Agent's DT w3c handling. --------- Co-authored-by: Michal Nowacki --- agent/php_globals.h | 1 + agent/php_minit.c | 1 + agent/php_nrini.c | 5 +++ agent/php_txn.c | 2 + agent/scripts/newrelic.ini.private.template | 1 + axiom/nr_distributed_trace.c | 37 +++++++++++++++++-- axiom/nr_txn.c | 6 +++ axiom/nr_txn.h | 1 + axiom/tests/test_distributed_trace.c | 14 +++++++ files/Dockerfile | 2 + .../w3c/test_insert_dt_headers_locale.php | 37 +++++++++++++++++++ 11 files changed, 104 insertions(+), 3 deletions(-) create mode 100644 tests/integration/distributed_tracing/w3c/test_insert_dt_headers_locale.php diff --git a/agent/php_globals.h b/agent/php_globals.h index 2f5ba9a41..c73b93e43 100644 --- a/agent/php_globals.h +++ b/agent/php_globals.h @@ -100,6 +100,7 @@ typedef struct _nrphpglobals_t { uint8_t debug_autorum; uint8_t show_loaded_files; uint8_t debug_cat; + uint8_t debug_dt; uint8_t disable_laravel_queue; } special_flags; /* special control options */ } nrphpglobals_t; diff --git a/agent/php_minit.c b/agent/php_minit.c index 7babac21a..0c6bc2dc6 100644 --- a/agent/php_minit.c +++ b/agent/php_minit.c @@ -702,6 +702,7 @@ PHP_MINIT_FUNCTION(newrelic) { NR_INFO_SPECIAL_FLAGS(debug_autorum); NR_INFO_SPECIAL_FLAGS(show_loaded_files); NR_INFO_SPECIAL_FLAGS(debug_cat); + NR_INFO_SPECIAL_FLAGS(debug_dt); #undef NR_INFO_SPECIAL_FLAGS nr_guzzle4_minit(TSRMLS_C); diff --git a/agent/php_nrini.c b/agent/php_nrini.c index 159a2e44b..1a5d5c831 100644 --- a/agent/php_nrini.c +++ b/agent/php_nrini.c @@ -950,6 +950,10 @@ static void foreach_special_control_flag(const char* str, NR_PHP_PROCESS_GLOBALS(special_flags).debug_cat = 1; return; } + if (0 == nr_strcmp(str, "debug_dt")) { + NR_PHP_PROCESS_GLOBALS(special_flags).debug_dt = 1; + return; + } if (0 == nr_strcmp(str, "disable_laravel_queue")) { NR_PHP_PROCESS_GLOBALS(special_flags).disable_laravel_queue = 1; return; @@ -977,6 +981,7 @@ static PHP_INI_MH(nr_special_mh) { NR_PHP_PROCESS_GLOBALS(special_flags).debug_autorum = 0; NR_PHP_PROCESS_GLOBALS(special_flags).show_loaded_files = 0; NR_PHP_PROCESS_GLOBALS(special_flags).debug_cat = 0; + NR_PHP_PROCESS_GLOBALS(special_flags).debug_dt = 0; NR_PHP_PROCESS_GLOBALS(special_flags).disable_laravel_queue = 0; if (0 != NEW_VALUE_LEN) { diff --git a/agent/php_txn.c b/agent/php_txn.c index 5cd0f75de..6cbe28bc3 100644 --- a/agent/php_txn.c +++ b/agent/php_txn.c @@ -871,6 +871,8 @@ nr_status_t nr_php_txn_begin(const char* appnames, = NR_PHP_PROCESS_GLOBALS(special_flags).show_sql_parsing; NRTXN(special_flags.debug_cat) = NR_PHP_PROCESS_GLOBALS(special_flags).debug_cat; + NRTXN(special_flags.debug_dt) + = NR_PHP_PROCESS_GLOBALS(special_flags).debug_dt; NRTXNGLOBAL(prepared_statements) = nr_hashmap_create(nr_php_txn_prepared_statement_destroy); diff --git a/agent/scripts/newrelic.ini.private.template b/agent/scripts/newrelic.ini.private.template index b366ff7db..67175b6b8 100644 --- a/agent/scripts/newrelic.ini.private.template +++ b/agent/scripts/newrelic.ini.private.template @@ -49,6 +49,7 @@ ; debug_autorum ; show_loaded_files ; debug_cat +; debug_dt ; disable_laravel_queue ; ;newrelic.special = show_executes, show_execute_params, show_execute_stack, show_execute_returns, show_executes_untrimmed diff --git a/axiom/nr_distributed_trace.c b/axiom/nr_distributed_trace.c index a7c7e356e..151ae6406 100644 --- a/axiom/nr_distributed_trace.c +++ b/axiom/nr_distributed_trace.c @@ -40,6 +40,27 @@ static inline void set_dt_field(char** field, const char* value) { } } +/* + * Purpose Format trace's priority for tracestate in W3C header. + * + * @param value double + * @return char* priority string buffer + */ +static char* nr_priority_double_to_str(nr_sampling_priority_t value) { + char* buf = NULL; + + buf = nr_formatf("%.6f", value); + + for (int i = 0; i < nr_strlen(buf); i++) { + if (',' == buf[i]) { + buf[i] = '.'; + break; + } + } + + return buf; +} + nr_distributed_trace_t* nr_distributed_trace_create(void) { nr_distributed_trace_t* dt; dt = (nr_distributed_trace_t*)nr_zalloc(sizeof(nr_distributed_trace_t)); @@ -1164,6 +1185,8 @@ char* nr_distributed_trace_create_w3c_tracestate_header( const char* app_id; char* trace_context_header = NULL; char* sampled = "0"; + nr_sampling_priority_t priority; + char* priority_buf = NULL; if (nrunlikely(NULL == dt)) { return NULL; @@ -1197,11 +1220,19 @@ char* nr_distributed_trace_create_w3c_tracestate_header( sampled = "1"; } + priority = nr_distributed_trace_get_priority(dt); + + priority_buf = nr_priority_double_to_str(priority); + if (NULL == priority_buf) { + nrl_verbosedebug(NRL_CAT, "Failed to allocate priority buffer"); + } + trace_context_header = nr_formatf( - "%s@nr=0-0-%s-%s-%s-%s-%s-%.6f-%" PRId64, trusted_account_key, account_id, + "%s@nr=0-0-%s-%s-%s-%s-%s-%s-%" PRId64, trusted_account_key, account_id, app_id, NRBLANKSTR(span_id), NRBLANKSTR(txn_id), sampled, - nr_distributed_trace_get_priority(dt), - (nr_get_time() / NR_TIME_DIVISOR_MS)); + NRBLANKSTR(priority_buf), (nr_get_time() / NR_TIME_DIVISOR_MS)); + + nr_free(priority_buf); return trace_context_header; } diff --git a/axiom/nr_txn.c b/axiom/nr_txn.c index dd084f98d..607ddfcba 100644 --- a/axiom/nr_txn.c +++ b/axiom/nr_txn.c @@ -2787,6 +2787,12 @@ char* nr_txn_create_w3c_tracestate_header(const nrtxn_t* txn, header = nr_distributed_trace_create_w3c_tracestate_header( txn->distributed_trace, span_id, txn_id); + if (txn->special_flags.debug_dt) { + nrl_verbosedebug(NRL_CAT, + "Outbound W3C TraceState Context Header generated: %s", + NRSAFESTR(header)); + } + nr_free(txn_id); return header; } diff --git a/axiom/nr_txn.h b/axiom/nr_txn.h index 357382dd0..74a762bab 100644 --- a/axiom/nr_txn.h +++ b/axiom/nr_txn.h @@ -315,6 +315,7 @@ typedef struct _nrtxn_t { uint8_t no_sql_parsing; /* don't do SQL parsing */ uint8_t show_sql_parsing; /* show various steps in SQL feature parsing */ uint8_t debug_cat; /* extra logging for CAT */ + uint8_t debug_dt; /* extra logging for DT */ } special_flags; /* diff --git a/axiom/tests/test_distributed_trace.c b/axiom/tests/test_distributed_trace.c index 9cb626535..c74d549f5 100644 --- a/axiom/tests/test_distributed_trace.c +++ b/axiom/tests/test_distributed_trace.c @@ -9,6 +9,7 @@ #include "nr_txn.h" #include "nr_distributed_trace_private.h" #include "util_memory.h" +#include static void test_distributed_trace_create_destroy(void) { // create a few instances to make sure state stays separate @@ -1351,6 +1352,19 @@ static void test_create_trace_state_header(void) { nr_strstr(result, expected)); nr_free(result); + /* + * Test: locale is set to use `,` instead of `.` for decimal values + */ + setlocale(LC_NUMERIC, "pl_PL"); + dt->priority = 0.123456; + expected = "777@nr=0-0-1234-9876-123456789-meatball!-0-0.123456-"; + result + = nr_distributed_trace_create_w3c_tracestate_header(dt, span_id, txn_id); + tlib_pass_if_not_null("locale should not affect priority formatting", + nr_strstr(result, expected)); + nr_free(result); + setlocale(LC_NUMERIC, "en_US"); + nr_distributed_trace_destroy(&dt); } diff --git a/files/Dockerfile b/files/Dockerfile index f13cd367b..c6da208c0 100644 --- a/files/Dockerfile +++ b/files/Dockerfile @@ -54,6 +54,8 @@ RUN apt-get update && apt-get install -y \ libc6-dev \ libgtest-dev \ libtool \ + locales \ + locales-all \ make \ perl \ strace \ diff --git a/tests/integration/distributed_tracing/w3c/test_insert_dt_headers_locale.php b/tests/integration/distributed_tracing/w3c/test_insert_dt_headers_locale.php new file mode 100644 index 000000000..769e611f0 --- /dev/null +++ b/tests/integration/distributed_tracing/w3c/test_insert_dt_headers_locale.php @@ -0,0 +1,37 @@ + 'en-US,en;q=0.5'); +setlocale(LC_NUMERIC, 'fr_FR'); +tap_assert(newrelic_insert_distributed_trace_headers($headers), 'insert function succeeded'); +tap_assert(array_key_exists('Accept-Language', $headers), 'preexisting header is present'); +tap_assert(array_key_exists('newrelic', $headers), 'newrelic header is present'); +tap_assert(array_key_exists('tracestate', $headers), 'tracestate header is present'); +tap_assert(array_key_exists('traceparent', $headers), 'traceparent header is present'); +tap_refute(strpos($headers['tracestate'], ','), 'locale handled correctly for w3c dt priority'); +