New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Swoole Integration #2595
feat: Swoole Integration #2595
Conversation
Codecov Report
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.
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed 👍 That makes me wonder whether we should do the same for the RoadRunner integration 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really? TLS should be independent from the protocol, unless that's something special swoole does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, checking ssl vs non-ssl is the right way to do that.
Co-authored-by: Bob Weinand <bob.weinand@datadoghq.com>
Co-authored-by: Bob Weinand <bob.weinand@datadoghq.com>
@@ -97,21 +97,14 @@ function (HookData $hook) use (&$rootSpan, $integration) { | |||
$rootSpan->resource = $request->server['request_method'] . ' ' . $normalizedPath; | |||
$rootSpan->meta[Tag::HTTP_METHOD] = $request->server['request_method']; | |||
|
|||
$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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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)Swoole\Http\Server
(HTTPS)Server + Headers variable (HTTPS)
Swoole\Http\Request
(HTTP)Swoole\Http\Request
(HTTPS)application/json POST Request
Swoole\Http\Request::rawContent
Swoole\Http\Request
application/x-www-form-urlencoded POST Request
Swoole\Http\Request::rawContent
password=should_redact&username=should_not_redact&foo[bar]=should_not_redact
Swoole\Http\Request
Reviewer checklist