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 <[email protected]>
newrelic · Feb 8, 2024 · 4746cca · 4746cca
1 parent 2cd020e
commit 4746cca
Show file tree

Hide file tree

Showing 11 changed files with 104 additions and 3 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -9,6 +9,7 @@
 #include "nr_txn.h"
 #include "nr_distributed_trace_private.h"
 #include "util_memory.h"
+#include <locale.h>
 
 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
@@ -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
@@ -0,0 +1,37 @@
+<?php
+/*
+ * Copyright 2020 New Relic Corporation. All rights reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+/*DESCRIPTION
+Tests that the locale is handled correctly in w3c dt tracestate header
+priority for locales that use `,` instead of `.` for decimal values
+*/
+
+/*INI
+newrelic.distributed_tracing_enabled = true
+newrelic.distributed_tracing_exclude_newrelic_header = false
+newrelic.cross_application_tracer.enabled = false
+*/
+
+/*EXPECT
+ok - insert function succeeded
+ok - preexisting header is present
+ok - newrelic header is present
+ok - tracestate header is present
+ok - traceparent header is present
+ok - locale handled correctly for w3c dt priority
+*/
+
+require_once(realpath (dirname ( __FILE__ )) . '/../../../include/tap.php');
+
+$headers = array('Accept-Language' => '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');
+