feat: add process tags to tracing payloads #3436
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3436 +/- ##
==========================================
- Coverage 61.87% 61.85% -0.02%
==========================================
Files 141 141
Lines 12481 12481
Branches 1630 1630
==========================================
- Hits 7722 7720 -2
- Misses 4038 4041 +3
+ Partials 721 720 -1 see 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Benchmarks [ tracer ]Benchmark execution time: 2025-10-01 08:39:04 Comparing candidate commit a5638f5 in PR branch Found 1 performance improvements and 6 performance regressions! Performance is the same for 187 metrics, 0 unstable metrics. scenario:ComposerTelemetryBench/benchTelemetryParsing-opcache
scenario:MessagePackSerializationBench/benchMessagePackSerialization
scenario:SamplingRuleMatchingBench/benchRegexMatching1
scenario:SamplingRuleMatchingBench/benchRegexMatching2
scenario:SamplingRuleMatchingBench/benchRegexMatching3
scenario:SamplingRuleMatchingBench/benchRegexMatching4
scenario:TraceSerializationBench/benchSerializeTrace
|
ext/process_tags.c
Outdated
| *ptr = '\0'; | ||
|
|
||
| // Create a persistent zend_string | ||
| process_tags.serialized = zend_string_init(buffer, total_len, 1); |
There was a problem hiding this comment.
The better way would be
process_tags.serialized = zend_string_alloc(total_len, 1);
char *ptr = ZSTR_VAL(process_tags.serialized);
rather than creating a temporary buffer, to copy and free it; right away allocate the zend_string and manipulate its contents.
ext/process_tags.c
Outdated
| const process_tag_entry_t *tag_a = (const process_tag_entry_t *)a; | ||
| const process_tag_entry_t *tag_b = (const process_tag_entry_t *)b; |
There was a problem hiding this comment.
| const process_tag_entry_t *tag_a = (const process_tag_entry_t *)a; | |
| const process_tag_entry_t *tag_b = (const process_tag_entry_t *)b; | |
| const process_tag_entry_t *tag_a = a; | |
| const process_tag_entry_t *tag_b = b; |
Needing an explicit cast of void pointers is so C++ :-D
ext/process_tags.c
Outdated
| // Try to get executable path | ||
| #ifdef _WIN32 | ||
| char exe_path[PATH_MAX]; | ||
| DWORD len = GetModuleFileNameA(NULL, exe_path, PATH_MAX); | ||
| if (len > 0 && len < PATH_MAX) { | ||
| entrypoint_name = get_basename(exe_path); | ||
| entrypoint_basedir = get_dirname(exe_path); | ||
| } | ||
| #else | ||
| char exe_path[PATH_MAX]; | ||
| ssize_t len = readlink("/proc/self/exe", exe_path, sizeof(exe_path) - 1); | ||
| if (len != -1) { | ||
| exe_path[len] = '\0'; | ||
| entrypoint_name = get_basename(exe_path); | ||
| entrypoint_basedir = get_dirname(exe_path); | ||
| } else { | ||
| // Fallback: use argv[0] or executable_location if available | ||
| if (sapi_module.executable_location) { | ||
| entrypoint_name = get_basename(sapi_module.executable_location); | ||
| entrypoint_basedir = get_dirname(sapi_module.executable_location); | ||
| } | ||
| } | ||
| #endif |
There was a problem hiding this comment.
This is not what we want to actually collect. We are never interested in /proc/self/exe, which always is the php (or webserver) executable.
For PHP specifically the entrypoint is the primary script executed, and actually not a process-global in non-CLI cases. ... which makes no sense for non-cli process tags, so they should be omitted then (???).
This one though, annoyingly, is not available at minit time (but at first_rinit, which is good enough).
There was a problem hiding this comment.
You are absolutely right. Changed it to use SG(request_info).path_translated for the actual script, moved everything to first_rinit() like you suggested, and ditched the MINIT hook entirely.
But then, I went down a rabbit hole on your 'makes no sense for non-CLI" comment. I looked a bit deeper at the Java Implementation and the Collected Tags Table, and they do collect process tags for web servers like Tomcat and JBoss: they get server.type (the vendor) and other metadata. So I figured PHP-FPM etc. is kind of similar - one pool usually serves one app, and the SAPI name is basically our version of server.type: tomcat.
So right now, it collects full entrypoint info for CLI (script name, basedir, etc.) but for web SAPIs it just gets server.type (the SAPI name) and entrypoint.workdir. No script name for web since different requests = different scripts, like you said.
My thinking was that most production PHP is not CLI, so excluding web would mean this only helps a small portion of workloads.
But if you think we should keep it CLI-only for simplicity, I'm happy to revert that part.
This is, anyway, something that will have to be perhaps discussed w/ the folks knowing more about that feature.
3 participants
Description
Reviewer checklist