[Symfony] Fix: Set span status according to otel convention #295
Conversation
The span status was always set to error for responses with status code 400 and up. According to the otel semantic conventions, for 4xx status, the span status MUST be left unset for spans with SpanKind.SERVER This commit adds the logic to implement this behaviour. Ref: https://opentelemetry.io/docs/specs/semconv/http/http-spans/#status
|
Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one. |
technimad-splunk
changed the title
|
PR #290 fixed a similar issue for Laravel. |
| @@ -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) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
looks ok to me, can it be tested by adding/modifying our existing tests?
Internal spans could have an error status, while the accompanying server span didn't have an error status. This changes this behaviour to ONLY set error status on requests with status code >= 500 regardless of the span kind.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #295 +/- ##
=========================================
Coverage 82.68% 82.68%
Complexity 949 949
=========================================
Files 89 89
Lines 3807 3807
=========================================
Hits 3148 3148
Misses 659 659
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
|
We would love to see this merged (and released) how can I help to make this happen? |
|
@mvanduijker merged. This package has been in beta for a long time, I think it's ready for a 1.0 release? Perhaps you could test it out and report back? |
|
we gonna migrate to opentelemetry, with a pretty big project, this month. I can report back how it behaves. |
The span status was always set to error for responses with status code 400 and up.
According to the otel semantic conventions, for 4xx status, the span status MUST be left unset for spans with SpanKind.SERVER This commit adds the logic to implement this behaviour.
Ref: https://opentelemetry.io/docs/specs/semconv/http/http-spans/#status