Skip to content
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

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

Merged
merged 2 commits into from
Oct 1, 2024

Conversation

technimad-splunk
Copy link
Contributor

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

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
@technimad-splunk technimad-splunk requested a review from a team September 18, 2024 15:07
Copy link

welcome bot commented Sep 18, 2024

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 technimad-splunk changed the title Fix: Set span status according to otel convention [Symfony] Fix: Set span status according to otel convention Sep 18, 2024
@technimad-splunk
Copy link
Contributor Author

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) {
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
Collaborator

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?

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.
@technimad-splunk technimad-splunk requested a review from a team as a code owner September 19, 2024 08:15
Copy link

codecov bot commented Sep 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.68%. Comparing base (c549d0f) to head (121869c).
Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #295   +/-   ##
=========================================
  Coverage     82.68%   82.68%           
  Complexity      949      949           
=========================================
  Files            89       89           
  Lines          3807     3807           
=========================================
  Hits           3148     3148           
  Misses          659      659           
Flag Coverage Δ
Aws 85.75% <ø> (ø)
Context/Swoole 0.00% <ø> (ø)
Instrumentation/CodeIgniter 73.94% <ø> (ø)
Instrumentation/ExtAmqp 89.58% <ø> (ø)
Instrumentation/Guzzle 69.73% <ø> (ø)
Instrumentation/HttpAsyncClient 81.33% <ø> (ø)
Instrumentation/IO 70.90% <ø> (ø)
Instrumentation/MongoDB 77.33% <ø> (ø)
Instrumentation/OpenAIPHP 86.82% <ø> (ø)
Instrumentation/PDO 89.56% <ø> (ø)
Instrumentation/Psr14 78.12% <ø> (ø)
Instrumentation/Psr15 93.50% <ø> (ø)
Instrumentation/Psr18 82.08% <ø> (ø)
Instrumentation/Slim 86.95% <ø> (ø)
Instrumentation/Symfony 89.03% <100.00%> (ø)
Instrumentation/Yii 77.77% <ø> (ø)
Logs/Monolog 100.00% <ø> (ø)
Propagation/ServerTiming 100.00% <ø> (ø)
Propagation/TraceResponse 100.00% <ø> (ø)
ResourceDetectors/Container 93.02% <ø> (ø)
Sampler/RuleBased 32.14% <ø> (ø)
Shims/OpenTracing 92.99% <ø> (ø)
Symfony 88.20% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...rumentation/Symfony/src/SymfonyInstrumentation.php 84.90% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c549d0f...121869c. Read the comment docs.

@mvanduijker
Copy link

We would love to see this merged (and released) how can I help to make this happen?

@brettmc brettmc merged commit 6123ef4 into open-telemetry:main Oct 1, 2024
96 of 120 checks passed
@brettmc
Copy link
Collaborator

brettmc commented Oct 1, 2024

@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?

@mvanduijker
Copy link

we gonna migrate to opentelemetry, with a pretty big project, this month. I can report back how it behaves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants