Skip to content

[Symfony] Fix: Set span status according to otel convention #295

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Oct 1, 2024
Merged

[Symfony] Fix: Set span status according to otel convention #295

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fec2ccd
Set span status according to otel convention
technimad-splunk Sep 18, 2024
121869c
Do not set error status on internal spans with status 4xx
technimad-splunk Sep 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/Instrumentation/Symfony/src/SymfonyInstrumentation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,12 @@ public static function register(): void
return;
}

if ($response->getStatusCode() >= Response::HTTP_BAD_REQUEST) {
// Do not set error status on server spans for 4xx responses
if (!$span->getKind(SpanKind::KIND_SERVER) && $response->getStatusCode() >= Response::HTTP_BAD_REQUEST) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only seems possible if this is a subrequest (internal redirect) in which case span kind is INTERNAL. I assume it was an intentional choice to use error status specifically for 400 codes from internal redirects, as the specification does not cover this scenario at all. Since you already have a comment here, perhaps it could mention that this case is for subrequests/internal spans.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. This case can only happen with span kind Internal. The intention was to have this case mainly for client span kinds. Which can't happen here.
I will remove this case, to ONLY set error status on >= 500 status codes. This is the same behaviour as implemented in Laravel.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks ok to me, can it be tested by adding/modifying our existing tests?

$span->setStatus(StatusCode::STATUS_ERROR);
}
// Do set error status on all spans for 5xx responses
if ($response->getStatusCode() >= Response::HTTP_INTERNAL_SERVER_ERROR) {
$span->setStatus(StatusCode::STATUS_ERROR);
}
$span->setAttribute(TraceAttributes::HTTP_RESPONSE_STATUS_CODE, $response->getStatusCode());
Expand Down