Allow synchronous sidecar flushes and reduce flush limit (#2689)

* Allow synchronous flushes with the sidecar enabled Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * Set the sidecar trace force flush limit to 1 MB by default Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * Increase size limit two 2 MB and rename config Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * Fix unrelated small crash in dd_untrace trace logs Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> --------- Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
DataDog · Jun 4, 2024 · c582c9f · c582c9f
1 parent 7557b8a
commit c582c9f
Show file tree

Hide file tree

Showing 11 changed files with 96 additions and 21 deletions.
diff --git a/components-rs/sidecar.h b/components-rs/sidecar.h
@@ -70,6 +70,8 @@ ddog_MaybeError ddog_sidecar_connect(struct ddog_SidecarTransport **connection);
 
 ddog_MaybeError ddog_sidecar_ping(struct ddog_SidecarTransport **transport);
 
+ddog_MaybeError ddog_sidecar_flush_traces(struct ddog_SidecarTransport **transport);
+
 struct ddog_InstanceId *ddog_sidecar_instanceId_build(ddog_CharSlice session_id,
                                                       ddog_CharSlice runtime_id);
 

diff --git a/ext/auto_flush.c b/ext/auto_flush.c
@@ -66,7 +66,12 @@ ZEND_RESULT_CODE ddtrace_flush_tracer(bool force_on_startup, bool collect_cycles
                                 .client_computed_top_level = false,
                                 .client_computed_stats = false,
                         };
-                        ddog_MaybeError send_error = ddog_sidecar_send_trace_v04_shm(&ddtrace_sidecar, ddtrace_sidecar_instance_id, shm, written, &tags);
+                        size_t size_hint = written;
+                        zend_long n_requests = get_global_DD_TRACE_AGENT_FLUSH_AFTER_N_REQUESTS();
+                        if (n_requests) {
+                            size_hint = MAX(get_global_DD_TRACE_BUFFER_SIZE() / n_requests, size_hint);
+                        }
+                        ddog_MaybeError send_error = ddog_sidecar_send_trace_v04_shm(&ddtrace_sidecar, ddtrace_sidecar_instance_id, shm, size_hint, &tags);
                         do {
                             if (send_error.tag == DDOG_OPTION_ERROR_SOME_ERROR) {
                                 // retry sending it directly through the socket as last resort. May block though with large traces.

diff --git a/ext/configuration.c b/ext/configuration.c
@@ -4,6 +4,7 @@
 
 #include "ip_extraction.h"
 #include "logging.h"
+#include "json/json.h"
 #include <components/log/log.h>
 #include <zai_string/string.h>
 
@@ -205,3 +206,34 @@ bool ddtrace_config_integration_enabled(ddtrace_integration_name integration_nam
 
     return integration->is_enabled();
 }
+
+void ddtrace_change_default_ini(ddtrace_config_id config_id, zai_str str) {
+    zai_config_memoized_entry *memoized = &zai_config_memoized_entries[config_id];
+    zend_ini_entry *entry = memoized->ini_entries[0];
+    zend_string_release(entry->value);
+    entry->value = zend_string_init(str.ptr, str.len, 1);
+    if (entry->modified) {
+        entry->modified = false;
+        zend_string_release(entry->orig_value);
+    }
+#if ZTS
+    zend_ini_entry *runtime_entry = zend_hash_find_ptr(EG(ini_directives), entry->name);
+    if (runtime_entry != entry) {
+        zend_string_release(runtime_entry->value);
+        runtime_entry->value = zend_string_copy(entry->value);
+        if (runtime_entry->modified) {
+            runtime_entry->modified = false;
+            zend_string_release(runtime_entry->orig_value);
+        }
+    }
+#endif
+    memoized->default_encoded_value = str;
+    memoized->name_index = -1;
+
+    zval decoded;
+    ZVAL_UNDEF(&decoded);
+    if (zai_config_decode_value(str, memoized->type, memoized->parser, &decoded, 1)) {
+        zai_json_dtor_pzval(&memoized->decoded_value);
+        ZVAL_COPY_VALUE(&memoized->decoded_value, &decoded);
+    }
+}
diff --git a/ext/configuration.h b/ext/configuration.h
@@ -47,6 +47,8 @@ enum ddtrace_sampling_rules_format {
 /* This should be at least an order of magnitude higher than the userland HTTP Transport default. */
 #define DD_TRACE_BGS_TIMEOUT_VAL 5000
 
+#define DD_TRACE_AGENT_FLUSH_INTERVAL_VAL 1001
+
 #define DD_INTEGRATION_ANALYTICS_ENABLED_DEFAULT false
 #define DD_INTEGRATION_ANALYTICS_SAMPLE_RATE_DEFAULT 1
 
@@ -153,8 +155,9 @@ enum ddtrace_sampling_rules_format {
            .ini_change = zai_config_system_ini_change)                                                         \
     CONFIG(INT, DD_TRACE_BGS_TIMEOUT, DD_CFG_EXPSTR(DD_TRACE_BGS_TIMEOUT_VAL),                                 \
            .ini_change = zai_config_system_ini_change)                                                         \
-    CONFIG(INT, DD_TRACE_AGENT_FLUSH_INTERVAL, "5000", .ini_change = zai_config_system_ini_change)             \
-    CONFIG(INT, DD_TRACE_AGENT_FLUSH_AFTER_N_REQUESTS, "10")                                                   \
+    CONFIG(INT, DD_TRACE_AGENT_FLUSH_INTERVAL, DD_CFG_EXPSTR(DD_TRACE_AGENT_FLUSH_INTERVAL_VAL),               \
+           .ini_change = zai_config_system_ini_change)                                                         \
+    CONFIG(INT, DD_TRACE_AGENT_FLUSH_AFTER_N_REQUESTS, "0")                                                    \
     CONFIG(INT, DD_TRACE_SHUTDOWN_TIMEOUT, "5000", .ini_change = zai_config_system_ini_change)                 \
     CONFIG(BOOL, DD_TRACE_STARTUP_LOGS, "true")                                                                \
     CONFIG(BOOL, DD_TRACE_ONCE_LOGS, "true")                                                                   \
@@ -171,6 +174,7 @@ enum ddtrace_sampling_rules_format {
     CONFIG(CUSTOM(STRING), DD_TRACE_CLIENT_IP_HEADER, "", .parser = ddtrace_parse_client_ip_header_config)     \
     CONFIG(BOOL, DD_TRACE_FORKED_PROCESS, "true")                                                              \
     CONFIG(INT, DD_TRACE_HOOK_LIMIT, "100")                                                                    \
+    CONFIG(INT, DD_TRACE_BUFFER_SIZE, "2097152", .ini_change = zai_config_system_ini_change)              \
     CONFIG(INT, DD_TRACE_AGENT_MAX_PAYLOAD_SIZE, "52428800", .ini_change = zai_config_system_ini_change)       \
     CONFIG(INT, DD_TRACE_AGENT_STACK_INITIAL_SIZE, "131072", .ini_change = zai_config_system_ini_change)       \
     CONFIG(INT, DD_TRACE_AGENT_STACK_BACKLOG, "12", .ini_change = zai_config_system_ini_change)                \
@@ -252,4 +256,6 @@ static inline int ddtrace_quiet_zpp(void) {
     return PHP_DEBUG ? 0 : ZEND_PARSE_PARAMS_QUIET;
 }
 
+void ddtrace_change_default_ini(ddtrace_config_id config_id, zai_str str);
+
 #endif  // DD_CONFIGURATION_H
diff --git a/ext/ddtrace.c b/ext/ddtrace.c
@@ -1186,6 +1186,8 @@ static PHP_MSHUTDOWN_FUNCTION(ddtrace) {
     return SUCCESS;
 }
 
+static bool dd_rinit_once_done = false;
+
 static void dd_rinit_once(void) {
     /* The env vars are memoized on MINIT before the SAPI env vars are available.
      * We use the first RINIT to bust the env var cache and use the SAPI env vars.
@@ -1197,9 +1199,19 @@ static void dd_rinit_once(void) {
 #ifndef _WIN32
     ddtrace_signals_first_rinit();
     if (!get_global_DD_TRACE_SIDECAR_TRACE_SENDER()) {
+        if (get_global_DD_TRACE_AGENT_FLUSH_AFTER_N_REQUESTS() == 0) {
+            // Set the default to 10 so that BGS flushes faster. With sidecar it's not needed generally.
+            ddtrace_change_default_ini(DDTRACE_CONFIG_DD_TRACE_AGENT_FLUSH_AFTER_N_REQUESTS, (zai_str) ZAI_STR_FROM_CSTR("10"));
+        }
+        if (get_DD_TRACE_AGENT_FLUSH_INTERVAL() == DD_TRACE_AGENT_FLUSH_INTERVAL_VAL) {
+            // Set the default to 5000 so that BGS does not flush too often. The sidecar can flush more often, but the BGS is per process. Keep it higher to avoid too much load on the agent.
+            ddtrace_change_default_ini(DDTRACE_CONFIG_DD_TRACE_AGENT_FLUSH_INTERVAL, (zai_str) ZAI_STR_FROM_CSTR("5000"));
+        }
         ddtrace_coms_init_and_start_writer();
     }
 #endif
+
+    dd_rinit_once_done = true;
 }
 
 static pthread_once_t dd_rinit_once_control = PTHREAD_ONCE_INIT;
@@ -2143,6 +2155,22 @@ PHP_FUNCTION(dd_trace_internal_fn) {
             ddog_CharSlice slice = ddog_sidecar_stats(&ddtrace_sidecar);
             RETVAL_STRINGL(slice.ptr, slice.len);
             free((void *) slice.ptr);
+        } else if (FUNCTION_NAME_MATCHES("synchronous_flush")) {
+            uint32_t timeout = 100;
+            if (params_count == 1) {
+                timeout = Z_LVAL_P(ZVAL_VARARG_PARAM(params, 0));
+            }
+#ifndef _WIN32
+            if (!get_global_DD_TRACE_SIDECAR_TRACE_SENDER()) {
+                if (dd_rinit_once_done) {
+                    ddtrace_coms_synchronous_flush(timeout);
+                }
+            } else
+#endif
+            if (ddtrace_sidecar) {
+                ddog_sidecar_flush_traces(&ddtrace_sidecar);
+            }
+            RETVAL_TRUE;
 #ifndef _WIN32
         } else if (FUNCTION_NAME_MATCHES("init_and_start_writer")) {
             RETVAL_BOOL(ddtrace_coms_init_and_start_writer());
@@ -2176,13 +2204,6 @@ PHP_FUNCTION(dd_trace_internal_fn) {
         } else if (FUNCTION_NAME_MATCHES("test_msgpack_consumer")) {
             ddtrace_coms_test_msgpack_consumer();
             RETVAL_TRUE;
-        } else if (FUNCTION_NAME_MATCHES("synchronous_flush")) {
-            uint32_t timeout = 100;
-            if (params_count == 1) {
-                timeout = Z_LVAL_P(ZVAL_VARARG_PARAM(params, 0));
-            }
-            ddtrace_coms_synchronous_flush(timeout);
-            RETVAL_TRUE;
 #endif
         } else if (FUNCTION_NAME_MATCHES("test_logs")) {
             ddog_logf(DDOG_LOG_WARN, false, "foo");
@@ -2244,9 +2265,9 @@ PHP_FUNCTION(dd_trace_close_all_spans_and_flush) {
 
 /* {{{ proto void dd_trace_synchronous_flush(int) */
 PHP_FUNCTION(dd_trace_synchronous_flush) {
-    zend_long timeout;
+    zend_long timeout = 100;
 
-    if (zend_parse_parameters(ZEND_NUM_ARGS(), "l", &timeout) == FAILURE) {
+    if (zend_parse_parameters(ZEND_NUM_ARGS(), "|l", &timeout) == FAILURE) {
         RETURN_THROWS();
     }
 
@@ -2257,8 +2278,15 @@ PHP_FUNCTION(dd_trace_synchronous_flush) {
     }
 
 #ifndef _WIN32
-    ddtrace_coms_synchronous_flush(timeout);
+    if (!get_global_DD_TRACE_SIDECAR_TRACE_SENDER()) {
+        if (dd_rinit_once_done) {
+            ddtrace_coms_synchronous_flush(timeout);
+        }
+    } else
 #endif
+    if (ddtrace_sidecar) {
+        ddog_sidecar_flush_traces(&ddtrace_sidecar);
+    }
     RETURN_NULL();
 }
 

diff --git a/ext/ddtrace.stub.php b/ext/ddtrace.stub.php
@@ -1033,12 +1033,12 @@ function dd_trace_method(
      * @param string|null $className In the case of a method, its respective class should be provided as well
      * @return bool 'true' if the un-tracing process was successful, else 'false'
      */
-    function dd_untrace(string $functionName, string $className = null): bool {}
+    function dd_untrace(string $functionName, string|null $className = null): bool {}
 
     /**
      * Blocking-call synchronously flushing all spans to the agent
      *
      * @param int $timeout Timeout in milliseconds to wait for the flush to complete
      */
-    function dd_trace_synchronous_flush(int $timeout): void {}
+    function dd_trace_synchronous_flush(int $timeout = 100): void {}
 }
diff --git a/ext/ddtrace_arginfo.h b/ext/ddtrace_arginfo.h
@@ -1,5 +1,5 @@
 /* This is a generated file, edit the .stub.php file instead.
- * Stub hash: 3a83516da1c795bbe41abd106fcc3e069bf3d946 */
+ * Stub hash: 190a99779c21033f0812df6365a7ebdd87057727 */
 
 ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_DDTrace_trace_method, 0, 3, _IS_BOOL, 0)
 	ZEND_ARG_TYPE_INFO(0, className, IS_STRING, 0)
@@ -264,11 +264,11 @@ ZEND_END_ARG_INFO()
 
 ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_dd_untrace, 0, 1, _IS_BOOL, 0)
 	ZEND_ARG_TYPE_INFO(0, functionName, IS_STRING, 0)
-	ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, className, IS_STRING, 0, "null")
+	ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, className, IS_STRING, 1, "null")
 ZEND_END_ARG_INFO()
 
-ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_dd_trace_synchronous_flush, 0, 1, IS_VOID, 0)
-	ZEND_ARG_TYPE_INFO(0, timeout, IS_LONG, 0)
+ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_dd_trace_synchronous_flush, 0, 0, IS_VOID, 0)
+	ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, timeout, IS_LONG, 0, "100")
 ZEND_END_ARG_INFO()
 
 ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_DDTrace_SpanLink_jsonSerialize, 0, 0, IS_MIXED, 0)

diff --git a/ext/hook/uhook_legacy.c b/ext/hook/uhook_legacy.c
@@ -433,6 +433,7 @@ PHP_FUNCTION(dd_untrace) {
     LOG(HOOK_TRACE, "Removing all hook functions installed by hook&trace_%s at %s:%d on %s %s%s%s",
         class_name ? "method" : "function",
         zend_get_executed_filename(), zend_get_executed_lineno(),
+        class_name ? "method" : "function",
         class_name ? ZSTR_VAL(class_name) : "",
         class_name ? "::" : "",
         ZSTR_VAL(method_name));

diff --git a/ext/sidecar.c b/ext/sidecar.c
@@ -70,7 +70,7 @@ ddog_SidecarTransport *dd_sidecar_connection_factory(void) {
     ddog_CharSlice session_id = (ddog_CharSlice) {.ptr = (char *) dd_sidecar_formatted_session_id, .len = sizeof(dd_sidecar_formatted_session_id)};
     ddog_sidecar_session_set_config(&sidecar_transport, session_id, ddtrace_endpoint, dogstatsd_endpoint,
                                     get_global_DD_TRACE_AGENT_FLUSH_INTERVAL(),
-                                    get_global_DD_TRACE_AGENT_MAX_PAYLOAD_SIZE() * get_DD_TRACE_BETA_HIGH_MEMORY_PRESSURE_PERCENT() / 100,
+                                    get_global_DD_TRACE_BUFFER_SIZE(),
                                     get_global_DD_TRACE_AGENT_STACK_BACKLOG() * get_global_DD_TRACE_AGENT_MAX_PAYLOAD_SIZE(),
                                     get_global_DD_TRACE_DEBUG() ? DDOG_CHARSLICE_C("debug") : dd_zend_string_to_CharSlice(get_global_DD_TRACE_LOG_LEVEL()),
                                     (ddog_CharSlice){ .ptr = logpath, .len = strlen(logpath) });

diff --git a/libdatadog b/libdatadog
diff --git a/package.xml b/package.xml
@@ -96,6 +96,7 @@ This changeset is on top of 1.0.0beta1.
 - Link libpthread into the spawn_worker trampoline Datadog/libdatadog#452
 - Make use of the sidecar thread safe #2671
 - Send a correct size hint to the sidecar trace flusher #2686
+- Allow synchronous sidecar flushes and reduce flush limit #2689
 
 ### Internal
 - Update to maintain compatibility with libdatadog #2634, #2661