Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2595 +/- ##
============================================
- Coverage 75.78% 75.47% -0.32%
- Complexity 2564 2590 +26
============================================
Files 241 242 +1
Lines 27055 27168 +113
Branches 976 976
============================================
+ Hits 20503 20504 +1
- Misses 6032 6144 +112
Partials 520 520
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
BenchmarksBenchmark execution time: 2024-03-27 13:04:19 Comparing candidate commit 312faa1 in PR branch Found 3 performance improvements and 0 performance regressions! Performance is the same for 179 metrics, 0 unstable metrics. scenario:PDOBench/benchPDOBaseline
scenario:PDOBench/benchPDOOverhead
scenario:PDOBench/benchPDOOverheadWithDBM
|
| $url = $scheme . $host . $path . $query; | ||
| $rootSpan->meta[Tag::HTTP_URL] = Normalizer::uriNormalizeincomingPath($url); | ||
| }, | ||
| function (HookData $hook) use (&$rootSpan, $integration) { |
There was a problem hiding this comment.
This doesn't look right to me. &$rootSpan will not necessarily be the root span the Request was started with - multiple requests may run in parallel with swoole.
In the begin hook, use $rootSpan = $hook->span(new \DDTrace\SpanStack);. Then you also don't have to manually close the span in the end hook. The span can be accessed in the end span with $hook->span() (but this is unnecessary, as it's now a hook span, which will inherit the exception anyway).
There was a problem hiding this comment.
Changed 👍 That makes me wonder whether we should do the same for the RoadRunner integration 🤔
There was a problem hiding this comment.
Makes sense there too, but for roadrunner it's not a bug at least.
| $rootSpan->meta[Tag::HTTP_METHOD] = $request->server['request_method']; | ||
|
|
||
| $serverProtocol = $request->server['server_protocol'] ?? 'HTTP/1.1'; | ||
| $scheme = strpos($serverProtocol, 'HTTPS') !== false ? 'https://' : 'http://'; |
There was a problem hiding this comment.
really? TLS should be independent from the protocol, unless that's something special swoole does?
There was a problem hiding this comment.
I changed this part to use the server's SSL property. Since I seemingly don't have access to the URL, I think that's the best way of infering the scheme
There was a problem hiding this comment.
Yes, checking ssl vs non-ssl is the right way to do that.
Co-authored-by: Bob Weinand <[email protected]>
Co-authored-by: Bob Weinand <[email protected]>
|
|
||
| $serverProtocol = $request->server['server_protocol'] ?? 'HTTP/1.1'; | ||
| $scheme = strpos($serverProtocol, 'HTTPS') !== false ? 'https://' : 'http://'; | ||
| $scheme = $server->ssl ? 'https://' : 'http://'; |
There was a problem hiding this comment.
Can you move that comparison before the install_hook call? and use ($scheme) instead? Apart from the minuscule micro-optimization, it also avoids having a circular dependency. Not sure if it makes any difference, but I think we should.
|
|
||
| \DDTrace\close_spans_until($rootSpan); | ||
| \DDTrace\close_span(); | ||
| unset($rootSpan->meta['closure.declaration']); |
There was a problem hiding this comment.
Since we are tracing the callback, we would always have a redundant, non-really-useful tag with the location of this closure's declaration. In the case of Laravel Octane, for instance, we would always have this tag pointing to swoole-server.php, which is not really helpful.
Additionally, but to a lesser extent, this is inconsistent with other frameworks/libraries. If we are setting the location of the request callback declaration from Swoole, why aren't we setting the same for Laravel and Kernel::handle, for instance?
bwoebi
left a comment
bwoebi
left a comment
There was a problem hiding this comment.
LGTM now - thanks Alexandre! :-)
|
@PROFeNoM sorry for bothering you but could you talk about how to instrumentate the laravel octane with apm using the latest version (0.99.1)? |
|
Hey @sneycampos 👋 I'm sorry for the delay; I totally forgot to answer your message on the initial issue. All my apologies. I've answered on the Github Issue |
5 participants
Description
At last... #704
As done for Roadrunner, this PR automatically creates surrounding spans to Swoole requests.
AIT-5007
Sample Requests:
Important Notes to be clearly stated in the release notes and in the documentation:
For legacy and to help the review, here are some samples showing what's accessible from some given instances and variables at the time of a request:
Swoole\Http\Server(HTTP){ "setting": { "worker_num": 1, "task_worker_num": 0, "output_buffer_size": 4294967295, "max_connection": 100000, "open_http_protocol": true, "open_mqtt_protocol": false, "open_eof_check": false, "open_length_check": false }, "connections": {}, "host": "0.0.0.0", "port": 9999, "type": 1, "ssl": false, "mode": 1, "ports": [ { "host": "0.0.0.0", "port": 9999, "type": 1, "sock": 10, "ssl": false, "setting": null, "connections": {} } ], "master_pid": 72918, "manager_pid": 0, "worker_id": 0, "taskworker": false, "worker_pid": 72918, "stats_timer": null, "admin_server": null }Swoole\Http\Server(HTTPS){ "setting": { "enable_coroutine": false, "daemonize": false, "log_file": "\/var\/www\/html\/storage\/logs\/swoole_http.log", "log_level": 2, "max_request": "500", "package_max_length": 10485760, "reactor_num": 10, "send_yield": true, "socket_buffer_size": 10485760, "task_max_request": "500", "task_worker_num": 10, "worker_num": 10, "ssl_cert_file": "\/etc\/swoole\/ssl\/certs\/sail-selfsigned.crt", "ssl_key_file": "\/etc\/swoole\/ssl\/private\/sail-selfsigned.key", "output_buffer_size": 4294967295, "max_connection": 100000, "open_http_protocol": true, "open_mqtt_protocol": false, "open_eof_check": false, "open_length_check": false }, "connections": {}, "host": "0.0.0.0", "port": 80, "type": 1, "ssl": true, "mode": 2, "ports": [ { "host": "0.0.0.0", "port": 80, "type": 1, "sock": 3, "ssl": true, "setting": { "enable_coroutine": false, "daemonize": false, "log_file": "\/var\/www\/html\/storage\/logs\/swoole_http.log", "log_level": 2, "max_request": "500", "package_max_length": 10485760, "reactor_num": 10, "send_yield": true, "socket_buffer_size": 10485760, "task_max_request": "500", "task_worker_num": 10, "worker_num": 10, "ssl_cert_file": "\/etc\/swoole\/ssl\/certs\/sail-selfsigned.crt", "ssl_key_file": "\/etc\/swoole\/ssl\/private\/sail-selfsigned.key" }, "connections": {} } ], "master_pid": 0, "manager_pid": 35, "worker_id": 2, "taskworker": false, "worker_pid": 62, "stats_timer": null, "admin_server": null }Server + Headers variable (HTTPS)
{ "REQUEST_METHOD": "GET", "REQUEST_URI": "\/", "PATH_INFO": "\/", "REQUEST_TIME": 1711531156, "REQUEST_TIME_FLOAT": 1711531156.854609, "SERVER_PROTOCOL": "HTTP\/1.1", "SERVER_PORT": 80, "REMOTE_PORT": 36731, "REMOTE_ADDR": "192.168.65.1", "MASTER_TIME": 1711531156, "HTTP_HOST": "localhost:8000", "HTTP_CONNECTION": "keep-alive", "HTTP_CACHE_CONTROL": "max-age=0", "HTTP_SEC_CH_UA": "\"Chromium\";v=\"122\", \"Not(A:Brand\";v=\"24\", \"Brave\";v=\"122\"", "HTTP_SEC_CH_UA_MOBILE": "?0", "HTTP_SEC_CH_UA_PLATFORM": "\"macOS\"", "HTTP_UPGRADE_INSECURE_REQUESTS": "1", "HTTP_USER_AGENT": "Mozilla\/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit\/537.36 (KHTML, like Gecko) Chrome\/122.0.0.0 Safari\/537.36", "HTTP_ACCEPT": "text\/html,application\/xhtml+xml,application\/xml;q=0.9,image\/avif,image\/webp,image\/apng,*\/*;q=0.8", "HTTP_SEC_GPC": "1", "HTTP_SEC_FETCH_SITE": "none", "HTTP_SEC_FETCH_MODE": "navigate", "HTTP_SEC_FETCH_USER": "?1", "HTTP_SEC_FETCH_DEST": "document", "HTTP_ACCEPT_ENCODING": "gzip, deflate, br", "HTTP_ACCEPT_LANGUAGE": "en-GB,en-US;q=0.9,en;q=0.8" }Swoole\Http\Request(HTTP){ "fd": 2, "streamId": 0, "header": { "host": "localhost:9999", "accept": "*\/*", "user-agent": "Test", "x-header": "somevalue", "content-type": "application\/json", "x-datadog-sampling-priority": "1", "x-datadog-tags": "_dd.p.tid=660154ef00000000,_dd.p.user_id=42,_dd.p.dm=-0", "x-datadog-trace-id": "3328808867233106786", "x-datadog-parent-id": "496235574048588437", "traceparent": "00-660154ef000000002e324e0728998b62-06e2fba01c140295-01", "tracestate": "dd=p:06e2fba01c140295;t.user_id:42;t.dm:-0", "content-length": "147" }, "server": { "request_method": "POST", "request_uri": "\/", "path_info": "\/", "request_time": 1711363311, "request_time_float": 1711363311.457934, "server_protocol": "HTTP\/1.1", "server_port": 9999, "remote_port": 44100, "remote_addr": "127.0.0.1", "master_time": 1711363311 }, "cookie": null, "get": null, "files": null, "post": null, "tmpfiles": null }Swoole\Http\Request(HTTPS){ "fd": 2, "streamId": 0, "header": { "host": "localhost:8000", "connection": "keep-alive", "cache-control": "max-age=0", "sec-ch-ua": "\"Chromium\";v=\"122\", \"Not(A:Brand\";v=\"24\", \"Brave\";v=\"122\"", "sec-ch-ua-mobile": "?0", "sec-ch-ua-platform": "\"macOS\"", "upgrade-insecure-requests": "1", "user-agent": "Mozilla\/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit\/537.36 (KHTML, like Gecko) Chrome\/122.0.0.0 Safari\/537.36", "accept": "text\/html,application\/xhtml+xml,application\/xml;q=0.9,image\/avif,image\/webp,image\/apng,*\/*;q=0.8", "sec-gpc": "1", "sec-fetch-site": "none", "sec-fetch-mode": "navigate", "sec-fetch-user": "?1", "sec-fetch-dest": "document", "accept-encoding": "gzip, deflate, br", "accept-language": "en-GB,en-US;q=0.9,en;q=0.8" }, "server": { "request_method": "GET", "request_uri": "\/", "path_info": "\/", "request_time": 1711530047, "request_time_float": 1711530047.287378, "server_protocol": "HTTP\/1.1", "server_port": 80, "remote_port": 29634, "remote_addr": "192.168.65.1", "master_time": 1711530047 }, "cookie": { "laminas-hidden": "0", "wordpress_test_cookie": "WP Cookie check", "tk_ai": "jetpack:No8FXdkwlXTziCfVHY7Tt4A4", "store_notice1630af9d4f3fdeaacb6b4f04b646c14f": "hidden", "wp_lang": "en_US", "wordpress_logged_in_57d2885bc075042a6f19613b9e08d15f": "admin|1692879227|YnsFpQojAUlGy1kOAqWaxzZvrwsKWpXZ94mwXDSswtm|e10b9b59dd4ee2902a255ab2e5a88c0276d4e81f583d72b435e00834307ec300", "mage-translation-storage": "{}", "mage-translation-file-version": "{}", "woocommerce_items_in_cart": "1", "woocommerce_cart_hash": "a4c1ab5ecf31d4b7115714a2dbaca1c3", "XSRF-TOKEN": "eyJpdiI6Im11cVJzeXYwSXdYNmRVNExYbE9zMWc9PSIsInZhbHVlIjoiNXRMaEtBa0liRjJuMXhERlNxVXI4RXhQWStwb3lwbnRFb3pCWWRZdFVlNzdHOXVpWmNiUzBOUHM1RkhsWUorandYL3pKemFJS3lPQ0h1bzJDNm9vbFRKOFdHWWVSeHRoMlQ0emN6K2x4Z0VzbDhycDB4V1VoT2FvcDlaYndOc0ciLCJtYWMiOiJjMGE5Y2RjYWEwZDMzYTM0OGJiZTE1Y2E2YmZlZDBhYTQ3YmM1YWQ0MzdhNDJlMjRmOTA5ODAyOTAxNWJlZTI4IiwidGFnIjoiIn0=", "laravel_session": "eyJpdiI6InB1WjJsR3ZPYjgxaDVmU3pIaTRibmc9PSIsInZhbHVlIjoiSm5MNHZ2a3lXelpKanlUcmwyQzZZcWJHam1LR29uTnUxaC9nTWViakNJUUxVVGZrVmVIT3ZpNGFoWnZxS3VYSTd5L25HdG52NkpnNFprc1NxSjVFRG1hd2J2RFNuMUNpUW9UNWVabXM0UFI3amZLWEljZ2hkd09uMUs1b3FKd1YiLCJtYWMiOiJiYjAxOWZkNjQwODhiYjNmODQyZTUwODEzNDlmMGQzMWRkNWM3NzlmNWEzY2ZmMWU2YmUyZjAzZjJhYjU5MmNlIiwidGFnIjoiIn0=" }, "get": null, "files": null, "post": null, "tmpfiles": null }application/json POST Request
Swoole\Http\Request::rawContent{ "pass word": "should_redact", "foo": { "password": "should_not_redact" }, "bar": { "key1": "value1", "key2": { "baz": "value2", "password": "should_not_redact" } } }Swoole\Http\Request{ "fd": 2, "streamId": 0, "header": { "host": "localhost:9999", "accept": "*\/*", "user-agent": "Test", "x-header": "somevalue", "content-type": "application\/json", "x-datadog-sampling-priority": "1", "x-datadog-tags": "_dd.p.tid=6601555f00000000,_dd.p.user_id=42,_dd.p.dm=-0", "x-datadog-trace-id": "4004550172342947583", "x-datadog-parent-id": "16402762076155285145", "traceparent": "00-6601555f000000003793052a2bbb0eff-e3a25133e47c0e99-01", "tracestate": "dd=p:e3a25133e47c0e99;t.user_id:42;t.dm:-0", "content-length": "147" }, "server": { "request_method": "POST", "request_uri": "\/", "path_info": "\/", "request_time": 1711363423, "request_time_float": 1711363423.161815, "server_protocol": "HTTP\/1.1", "server_port": 9999, "remote_port": 46204, "remote_addr": "127.0.0.1", "master_time": 1711363423 }, "cookie": null, "get": null, "files": null, "post": null, "tmpfiles": null }application/x-www-form-urlencoded POST Request
Swoole\Http\Request::rawContentpassword=should_redact&username=should_not_redact&foo[bar]=should_not_redactSwoole\Http\Request{ "fd": 1, "streamId": 0, "header": { "host": "localhost:9999", "accept": "*\/*", "user-agent": "Test", "x-header": "somevalue", "content-type": "application\/x-www-form-urlencoded", "x-datadog-sampling-priority": "1", "x-datadog-tags": "_dd.p.tid=6601556e00000000,_dd.p.user_id=42,_dd.p.dm=-0", "x-datadog-trace-id": "1754043491208180662", "x-datadog-parent-id": "1754043491208180662", "traceparent": "00-6601556e0000000018579d430e0cb3b6-18579d430e0cb3b6-01", "tracestate": "dd=p:18579d430e0cb3b6;t.user_id:42;t.dm:-0", "content-length": "76" }, "server": { "request_method": "POST", "request_uri": "\/", "path_info": "\/", "request_time": 1711363438, "request_time_float": 1711363438.029595, "server_protocol": "HTTP\/1.1", "server_port": 9999, "remote_port": 37134, "remote_addr": "127.0.0.1", "master_time": 1711363438 }, "cookie": null, "get": null, "files": null, "post": { "password": "should_redact", "username": "should_not_redact", "foo": { "bar": "should_not_redact" } }, "tmpfiles": null }Reviewer checklist