-
Notifications
You must be signed in to change notification settings - Fork 149
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
Changes from 1 commit
f6b2b9f
e56fc3f
738495c
bf27730
33e5f1d
75d6e5d
741b610
96dfe24
410f194
04b301a
6479abc
93a5470
c3c7673
312faa1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ | |
|
||
use DDTrace\HookData; | ||
use DDTrace\Integrations\Integration; | ||
use DDTrace\SpanData; | ||
use DDTrace\SpanStack; | ||
use DDTrace\Tag; | ||
use DDTrace\Type; | ||
use DDTrace\Util\Normalizer; | ||
|
@@ -23,20 +23,20 @@ public function getName() | |
return self::NAME; | ||
} | ||
|
||
public function addTraceAnalyticsIfEnabled(SpanData $span) | ||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function requiresExplicitTraceAnalyticsEnabling() | ||
{ | ||
if (!$this->configuration->isTraceAnalyticsEnabled()) { | ||
return; | ||
} | ||
$span->metrics[Tag::ANALYTICS_KEY] = $this->configuration->getTraceAnalyticsSampleRate(); | ||
return false; | ||
} | ||
|
||
public function instrumentRequestStart(callable $callback, SwooleIntegration $integration) | ||
public function instrumentRequestStart(callable $callback, SwooleIntegration $integration, Server $server) | ||
{ | ||
\DDTrace\install_hook( | ||
$callback, | ||
function (HookData $hook) use (&$rootSpan, $integration) { | ||
$rootSpan = \DDTrace\start_trace_span(); | ||
function (HookData $hook) use ($integration, $server) { | ||
$rootSpan = $hook->span(new SpanStack()); | ||
$rootSpan->name = "web.request"; | ||
$rootSpan->service = \ddtrace_config_app_name('swoole'); | ||
$rootSpan->type = Type::WEB_SERVLET; | ||
|
@@ -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://'; | ||
$host = $headers['host'] ?? ($request->server['remote_addr'] . ':' . $request->server['server_port']); | ||
$path = $request->server['request_uri'] ?? $request->server['path_info'] ?? ''; | ||
$query = isset($request->server['query_string']) ? '?' . $request->server['query_string'] : ''; | ||
$url = $scheme . $host . $path . $query; | ||
$rootSpan->meta[Tag::HTTP_URL] = Normalizer::uriNormalizeincomingPath($url); | ||
}, | ||
function (HookData $hook) use (&$rootSpan, $integration) { | ||
if ($hook->exception) { | ||
$rootSpan->exception = $hook->exception; | ||
} | ||
|
||
\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 commentThe 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 commentThe 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. |
||
} | ||
); | ||
} | ||
|
@@ -139,7 +132,7 @@ function ($server, $scope, $args, $retval) use ($integration) { | |
list($eventName, $callback) = $args; | ||
|
||
if ($eventName === 'request') { | ||
$integration->instrumentRequestStart($callback, $integration); | ||
$integration->instrumentRequestStart($callback, $integration, $server); | ||
} | ||
} | ||
); | ||
|
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.