-
Notifications
You must be signed in to change notification settings - Fork 5
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
add HTTP Client tracing #18
Conversation
LordSimal
commented
Jul 27, 2024
I am confused - how does sentry know how long the request took and/or when the span should be started? It looks to me like it starts the span at the end of when the response was already received. |
src/Http/CakeHttpEventListener.php
Outdated
if ($parentSpan !== null) { | ||
$context = SpanContext::make() | ||
->setOp('http.client'); | ||
$span = $parentSpan->startChild($context); |
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.
You need to start the span on HttpClient.beforeSend
and end it on HttpClient.afterSend
.
src/Http/CakeHttpEventListener.php
Outdated
->setData([ | ||
'http.request.method' => $request->getMethod(), | ||
'http.response.body.size' => $response?->getBody()?->getSize(), | ||
'http.response.status_code' => $response?->getStatusCode() ?? 0, | ||
]) |
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.
https://develop.sentry.dev/sdk/performance/modules/requests/ for all supported data attributes.
Have a look at https://github.com/getsentry/sentry-laravel/blob/master/src/Sentry/Laravel/Features/HttpClientIntegration.php for some inspiration. |
thanks, that definitely helped. |
@@ -76,6 +77,7 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface | |||
$this->addQueryData(); | |||
$listener = new EventListener(); | |||
EventManager::instance()->on($listener); | |||
EventManager::instance()->on(new HttpEventListener()); |
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.
In the next major realease of this plugin I would like to unify the listener classes into their own folder/namespace. Didn't expect that I already need multiple implementations of different event listeners already.
49a181d
to
fecccdc
Compare
e1cf247
to
f11c5cb
Compare