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 #308

Merged
merged 2 commits into from
Oct 22, 2024

Conversation

technimad-splunk
Copy link
Contributor

Only set error status on span if we know the response code is 500 or higher.

Addresses open-telemetry/opentelemetry-php#1403

In Symfony any error is handled as an exception. The instrumentation added an error status to any span when an exception occurred. This is not in line with the spec. For span kinds of server, the error status must ONLY be set for response codes of 500 and higher.
Earlier improvement was done in PR #295, but that fix wasn't sufficient.

@technimad-splunk technimad-splunk requested a review from a team as a code owner October 18, 2024 11:25
Copy link

codecov bot commented Oct 18, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 80.17%. Comparing base (78a04ce) to head (c3ecfb8).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...rumentation/Symfony/src/SymfonyInstrumentation.php 66.66% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #308      +/-   ##
============================================
- Coverage     80.33%   80.17%   -0.16%     
- Complexity     1026     1028       +2     
============================================
  Files            98       98              
  Lines          4114     4268     +154     
============================================
+ Hits           3305     3422     +117     
- Misses          809      846      +37     
Flag Coverage Δ
Aws 85.55% <ø> (+0.03%) ⬆️
Context/Swoole 0.00% <ø> (ø)
Instrumentation/CakePHP 20.00% <ø> (ø)
Instrumentation/CodeIgniter 73.77% <ø> (-0.18%) ⬇️
Instrumentation/ExtAmqp 89.26% <ø> (-0.33%) ⬇️
Instrumentation/Guzzle 69.51% <ø> (-0.23%) ⬇️
Instrumentation/HttpAsyncClient 81.25% <ø> (-0.09%) ⬇️
Instrumentation/IO 70.68% <ø> (-0.22%) ⬇️
Instrumentation/MongoDB 76.31% <ø> (-1.02%) ⬇️
Instrumentation/OpenAIPHP 87.31% <ø> (+0.49%) ⬆️
Instrumentation/PDO 89.95% <ø> (+0.39%) ⬆️
Instrumentation/Psr14 77.14% <ø> (-0.99%) ⬇️
Instrumentation/Psr15 93.82% <ø> (+0.32%) ⬆️
Instrumentation/Psr16 97.56% <ø> (+0.06%) ⬆️
Instrumentation/Psr18 81.15% <ø> (-0.94%) ⬇️
Instrumentation/Psr3 59.49% <ø> (-0.77%) ⬇️
Instrumentation/Psr6 97.67% <ø> (+0.05%) ⬆️
Instrumentation/Slim 86.89% <ø> (-0.06%) ⬇️
Instrumentation/Symfony 88.70% <66.66%> (-0.37%) ⬇️
Instrumentation/Yii 77.68% <ø> (-0.10%) ⬇️
Logs/Monolog 100.00% <ø> (ø)
Propagation/ServerTiming 100.00% <ø> (ø)
Propagation/TraceResponse 100.00% <ø> (ø)
ResourceDetectors/Container 93.02% <ø> (ø)
Sampler/RuleBased 33.51% <ø> (+1.37%) ⬆️
Shims/OpenTracing 92.45% <ø> (-0.55%) ⬇️
Symfony 87.94% <ø> (-0.27%) ⬇️

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 83.48% <66.66%> (-1.42%) ⬇️

... and 49 files with indirect coverage changes


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 78a04ce...c3ecfb8. Read the comment docs.

@brettmc
Copy link
Collaborator

brettmc commented Oct 18, 2024

Thanks, @technimad-splunk . It looks like there is a linting error in Symfony (see checks), can you please fix that?

@brettmc brettmc merged commit cf610d5 into open-telemetry:main Oct 22, 2024
109 of 120 checks passed
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.

3 participants