From bc54d0e304081a389a778904b846c376b43e0fac Mon Sep 17 00:00:00 2001 From: Bob Weinand Date: Tue, 5 Mar 2024 00:32:22 +0100 Subject: [PATCH] Move collecting and flushing telemetry to after final request flush Signed-off-by: Bob Weinand --- components-rs/ddtrace.h | 2 + components-rs/telemetry.rs | 4 ++ ext/ddtrace.c | 109 +++++++++++++++++++++++++++++++------ ext/ddtrace.h | 1 + ext/telemetry.c | 42 ++++++++++---- ext/telemetry.h | 1 + 6 files changed, 130 insertions(+), 29 deletions(-) diff --git a/components-rs/ddtrace.h b/components-rs/ddtrace.h index f6b82f542d..4f920502ab 100644 --- a/components-rs/ddtrace.h +++ b/components-rs/ddtrace.h @@ -171,6 +171,8 @@ ddog_MaybeError ddog_sidecar_telemetry_buffer_flush(ddog_SidecarTransport **tran const ddog_QueueId *queue_id, struct ddog_SidecarActionsBuffer *buffer); +void ddog_sidecar_telemetry_buffer_drop(struct ddog_SidecarActionsBuffer*); + ddog_MaybeError ddog_sidecar_connect_php(ddog_SidecarTransport **connection, const char *error_path, ddog_CharSlice log_level, diff --git a/components-rs/telemetry.rs b/components-rs/telemetry.rs index 3ca54b1e3a..7f7e88e6b3 100644 --- a/components-rs/telemetry.rs +++ b/components-rs/telemetry.rs @@ -127,3 +127,7 @@ pub extern "C" fn ddog_sidecar_telemetry_buffer_flush( MaybeError::None } + +#[no_mangle] +pub extern "C" fn ddog_sidecar_telemetry_buffer_drop(_: Box) { +} diff --git a/ext/ddtrace.c b/ext/ddtrace.c index 8214e38a94..9e6a18277a 100644 --- a/ext/ddtrace.c +++ b/ext/ddtrace.c @@ -1017,6 +1017,8 @@ static void dd_disable_if_incompatible_sapi_detected(void) { } } +static void dd_post_sapi_deactivate_minit(void); + static PHP_MINIT_FUNCTION(ddtrace) { UNUSED(type); @@ -1042,6 +1044,7 @@ static PHP_MINIT_FUNCTION(ddtrace) { #if ZAI_JIT_BLACKLIST_ACTIVE zai_jit_minit(); #endif + dd_post_sapi_deactivate_minit(); #if PHP_VERSION_ID >= 80100 ddtrace_setup_fiber_observers(); #endif @@ -1374,13 +1377,6 @@ void dd_force_shutdown_tracing(void) { DDTRACE_G(in_shutdown) = false; } -static void dd_finalize_telemetry(void) { - if (DDTRACE_G(telemetry_queue_id)) { - ddtrace_telemetry_finalize(); - DDTRACE_G(telemetry_queue_id) = 0; - } -} - static PHP_RSHUTDOWN_FUNCTION(ddtrace) { UNUSED(module_number, type); @@ -1401,7 +1397,19 @@ static PHP_RSHUTDOWN_FUNCTION(ddtrace) { DDTRACE_G(active_stack) = NULL; } - dd_finalize_telemetry(); + if (DDTRACE_G(telemetry_queue_id)) { + ddtrace_telemetry_collect_config(); + } + + return SUCCESS; +} + +static void dd_post_sapi_deactivate_do(void) { + if (DDTRACE_G(telemetry_queue_id)) { + ddtrace_telemetry_finalize(); + DDTRACE_G(telemetry_queue_id) = 0; + } + if (DDTRACE_G(last_flushed_root_service_name)) { zend_string_release(DDTRACE_G(last_flushed_root_service_name)); DDTRACE_G(last_flushed_root_service_name) = NULL; @@ -1411,7 +1419,14 @@ static PHP_RSHUTDOWN_FUNCTION(ddtrace) { DDTRACE_G(last_flushed_root_env_name) = NULL; } - return SUCCESS; + zai_interceptor_deactivate(); + + // we can only actually free our hooks hashtables in post_deactivate or later, as within RSHUTDOWN some user code may still run + zai_hook_rshutdown(); + zai_uhook_rshutdown(); + + // zai config may be accessed indirectly via other modules RSHUTDOWN, so delay this until the last possible time + zai_config_rshutdown(); } #if PHP_VERSION_ID < 80000 @@ -1419,15 +1434,71 @@ int ddtrace_post_deactivate(void) { #else zend_result ddtrace_post_deactivate(void) { #endif - zai_interceptor_deactivate(); + if (sapi_module.deactivate == NULL) { // e.g. in preloading + dd_post_sapi_deactivate_do(); + } + return SUCCESS; +} - // we can only actually free our hooks hashtables in post_deactivate, as within RSHUTDOWN some user code may still run - zai_hook_rshutdown(); - zai_uhook_rshutdown(); +static int (*dd_prev_sapi_deactivate)(void); - // zai config may be accessed indirectly via other modules RSHUTDOWN, so delay this until the last possible time - zai_config_rshutdown(); - return SUCCESS; +// Start of request_rec from httpd.h, this has been unchanged forever, so we can just use this. +struct apache_request_rec { + void *pool; + void *connection; + void *server; + void *next; + void *prev; + void *main; +}; +static void (*dd_ap_finalize_request_protocol)(struct apache_request_rec *r); + +static int dd_post_apache_deactivate(void) { + int result = dd_prev_sapi_deactivate ? dd_prev_sapi_deactivate() : SUCCESS; + + if (SG(sapi_started)) { + struct apache_request_rec *r = ((struct { + int state; + struct apache_request_rec *r; + } *) SG(server_context))->r; + // Check whether this is not an apache sub-request, otherwise we may not alter the state of the connection, i.e. here we don't gain any latency benefit + if (!r->main) { + dd_ap_finalize_request_protocol(r); + } + + dd_post_sapi_deactivate_do(); + } + + return result; +} + +static int dd_post_sapi_deactivate(void) { + int result = dd_prev_sapi_deactivate ? dd_prev_sapi_deactivate() : SUCCESS; + if (SG(sapi_started)) { // only if a proper request happened; it's also called during minit + dd_post_sapi_deactivate_do(); + } + return result; +} + +static void dd_post_sapi_deactivate_minit(void) { + dd_prev_sapi_deactivate = sapi_module.deactivate; + + if (!ddtrace_disable && ddtrace_active_sapi == DATADOG_PHP_SAPI_APACHE2HANDLER) { +#ifndef _WIN32 + void *handle = dlopen(NULL, RTLD_LAZY); +#else + void *handle = GetModuleHandle(NULL); // not refcounted, must not be closed +#endif + dd_ap_finalize_request_protocol = DL_FETCH_SYMBOL(handle, "ap_finalize_request_protocol"); +#ifndef _WIN32 + dlclose(handle); +#endif + if (dd_ap_finalize_request_protocol) { + sapi_module.deactivate = dd_post_apache_deactivate; + return; + } + } + sapi_module.deactivate = dd_post_sapi_deactivate; } void ddtrace_disable_tracing_in_current_request(void) { @@ -2056,7 +2127,11 @@ PHP_FUNCTION(dd_trace_internal_fn) { RETVAL_FALSE; if (ZSTR_LEN(function_val) > 0) { if (FUNCTION_NAME_MATCHES("finalize_telemetry")) { - dd_finalize_telemetry(); + if (DDTRACE_G(telemetry_queue_id)) { + ddtrace_telemetry_collect_config(); + ddtrace_telemetry_finalize(); + DDTRACE_G(telemetry_queue_id) = 0; + } RETVAL_TRUE; } else if (params_count == 1 && FUNCTION_NAME_MATCHES("detect_composer_installed_json")) { ddog_CharSlice path = dd_zend_string_to_CharSlice(Z_STR_P(ZVAL_VARARG_PARAM(params, 0))); diff --git a/ext/ddtrace.h b/ext/ddtrace.h index efa257e3b4..de217d4f8d 100644 --- a/ext/ddtrace.h +++ b/ext/ddtrace.h @@ -101,6 +101,7 @@ ZEND_BEGIN_MODULE_GLOBALS(ddtrace) char *cgroup_file; ddog_QueueId telemetry_queue_id; + ddog_SidecarActionsBuffer *telemetry_buffer; ddog_AgentRemoteConfigReader *remote_config_reader; HashTable *agent_rate_by_service; zend_string *last_flushed_root_service_name; diff --git a/ext/telemetry.c b/ext/telemetry.c index e6fb5daab1..51587d342e 100644 --- a/ext/telemetry.c +++ b/ext/telemetry.c @@ -25,23 +25,16 @@ void ddtrace_telemetry_first_init(void) { dd_composer_hook_id = zai_hook_install((zai_str)ZAI_STR_EMPTY, (zai_str)ZAI_STR_EMPTY, dd_check_for_composer_autoloader, NULL, ZAI_HOOK_AUX_UNUSED, 0); } -void ddtrace_telemetry_finalize(void) { +void ddtrace_telemetry_collect_config(void) { if (!ddtrace_sidecar || !get_global_DD_INSTRUMENTATION_TELEMETRY_ENABLED()) { return; } - ddog_SidecarActionsBuffer *buffer = ddog_sidecar_telemetry_buffer_alloc(); + if (DDTRACE_G(telemetry_buffer)) { + ddog_sidecar_telemetry_buffer_drop(DDTRACE_G(telemetry_buffer)); + } - zend_module_entry *module; - char module_name[261] = { 'e', 'x', 't', '-' }; - ZEND_HASH_FOREACH_PTR(&module_registry, module) { - size_t namelen = strlen(module->name); - memcpy(module_name + 4, module->name, MIN(256, strlen(module->name))); - const char *version = module->version ? module->version : ""; - ddog_sidecar_telemetry_addDependency_buffer(buffer, - (ddog_CharSlice) {.len = namelen + 4, .ptr = module_name}, - (ddog_CharSlice) {.len = strlen(version), .ptr = version}); - } ZEND_HASH_FOREACH_END(); + ddog_SidecarActionsBuffer *buffer = DDTRACE_G(telemetry_buffer) = ddog_sidecar_telemetry_buffer_alloc(); for (uint8_t i = 0; i < zai_config_memoized_entries_count; i++) { zai_config_memoized_entry *cfg = &zai_config_memoized_entries[i]; @@ -69,6 +62,31 @@ void ddtrace_telemetry_finalize(void) { ddog_sidecar_telemetry_addIntegration_buffer(buffer, integration_name, (ddog_CharSlice)DDOG_CHARSLICE_C(""), false); } } +} + +void ddtrace_telemetry_finalize(void) { + if (!ddtrace_sidecar || !get_global_DD_INSTRUMENTATION_TELEMETRY_ENABLED()) { + return; + } + + ddog_SidecarActionsBuffer *buffer = DDTRACE_G(telemetry_buffer); + if (buffer) { + DDTRACE_G(telemetry_buffer) = NULL; + } else { + buffer = ddog_sidecar_telemetry_buffer_alloc(); + } + + zend_module_entry *module; + char module_name[261] = { 'e', 'x', 't', '-' }; + ZEND_HASH_FOREACH_PTR(&module_registry, module) { + size_t namelen = strlen(module->name); + memcpy(module_name + 4, module->name, MIN(256, strlen(module->name))); + const char *version = module->version ? module->version : ""; + ddog_sidecar_telemetry_addDependency_buffer(buffer, + (ddog_CharSlice) {.len = namelen + 4, .ptr = module_name}, + (ddog_CharSlice) {.len = strlen(version), .ptr = version}); + } ZEND_HASH_FOREACH_END(); + ddog_sidecar_telemetry_buffer_flush(&ddtrace_sidecar, ddtrace_sidecar_instance_id, &DDTRACE_G(telemetry_queue_id), buffer); ddog_CharSlice service_name = DDOG_CHARSLICE_C("unnamed-php-service"); diff --git a/ext/telemetry.h b/ext/telemetry.h index 69ec068285..849728ff66 100644 --- a/ext/telemetry.h +++ b/ext/telemetry.h @@ -6,6 +6,7 @@ void ddtrace_telemetry_first_init(void); ddog_TelemetryWorkerHandle *ddtrace_build_telemetry_handle(void); void ddtrace_telemetry_notify_integration(const char *name, size_t name_len); +void ddtrace_telemetry_collect_config(void); void ddtrace_telemetry_finalize(void); #endif // DDTRACE_TELEMETRY_H