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

fix gRPC timeout (#1498) #1499

Merged
merged 2 commits into from
Feb 3, 2025

Conversation

spadger
Copy link
Contributor

@spadger spadger commented Jan 31, 2025

Fixes #1498 (grpc timeouts not being respected)

The timeout seems to work correctly for long and short variants of the bug with this code but I wasn't able to create meaningful tests. Whilst it was possible to get the calculated timeval from the transport instance using reflection, actually calling functions on \Grpc\Timeval (such as similar() just crashes the test runner

No additional unit tests fail compared to main (35 vs 35)
Happy to make any adjustments according to feedback

@spadger spadger requested a review from a team as a code owner January 31, 2025 14:27
Copy link

welcome bot commented Jan 31, 2025

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.

Copy link

codecov bot commented Feb 2, 2025

Codecov Report

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

Project coverage is 70.78%. Comparing base (96ce422) to head (922e120).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/Contrib/Grpc/GrpcTransport.php 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1499      +/-   ##
============================================
+ Coverage     70.76%   70.78%   +0.01%     
  Complexity     2762     2762              
============================================
  Files           408      408              
  Lines          8343     8344       +1     
============================================
+ Hits           5904     5906       +2     
+ Misses         2439     2438       -1     
Flag Coverage Δ
8.1 70.53% <66.66%> (+0.05%) ⬆️
8.2 70.64% <66.66%> (-0.01%) ⬇️
8.3 70.72% <66.66%> (+0.07%) ⬆️
8.4 70.75% <66.66%> (+0.07%) ⬆️
8.5 70.66% <66.66%> (-0.01%) ⬇️

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

Files with missing lines Coverage Δ
src/Contrib/Grpc/GrpcTransportFactory.php 71.79% <100.00%> (ø)
src/Contrib/Grpc/GrpcTransport.php 47.22% <50.00%> (+1.50%) ⬆️

... and 5 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 96ce422...922e120. Read the comment docs.

@brettmc
Copy link
Collaborator

brettmc commented Feb 3, 2025

LGTM - with no collector available I was able to test that the timeout is accurate for <5s values. Beyond 5s, I got a host lookup error which occurred at 5s if the configured timeout doesn't kick in first.

Can you please fix the linting error (make style locally, if you're using our test image etc)

@brettmc brettmc merged commit 31d902b into open-telemetry:main Feb 3, 2025
11 checks passed
spadger added a commit to spadger/opentelemetry-php that referenced this pull request Feb 5, 2025
spadger added a commit to spadger/opentelemetry-php that referenced this pull request Feb 5, 2025
brettmc pushed a commit that referenced this pull request Feb 5, 2025
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.

The gRPC exporter does not respect timeout settings
2 participants