GrpcTransportFactory fails to create when max retries is set to 0

[barell]

Describe your environment
PHP 8.3
open-telemetry/transport-grpc v1.1.2

Steps to reproduce
Set maxRetries to 0 when creating grpc transport.

What is the expected behavior?
Grpc transport is working and it does not attempt to retry on fail (the same configuration work on http transport).

What is the actual behavior?
Got error

ArithmeticError {#173
  #message: "Bit shift by negative number"
  #code: 0
  #file: "/app/vendor/open-telemetry/transport-grpc/GrpcTransportFactory.php"
  #line: 124
  trace: {
    /app/vendor/open-telemetry/transport-grpc/GrpcTransportFactory.php:124 { …}

I am not familiar with Otel ecosystem and do not know much grpc but I guess we should allow maxRetries = 0?

[brettmc]

I am not familiar with Otel ecosystem and do not know much grpc but I guess we should allow maxRetries = 0?

I think you're right, and it should mean only one attempt is made. The test coverage could be better for GrpcTransportFactory :(

[barell]

I might have spot another related issue - after setting max retries to 1 another error popped out:

channel stack builder failed: INVALID_ARGUMENT: errors validating service config: [field:methodConfig[0].retryPolicy.maxAttempts error:must be at least 2]

I guess it comes from grpc directly https://github.com/grpc/grpc-proto/blob/master/grpc/service_config/service_config.proto

[brettmc]

I guess it comes from grpc directly

Right, then it seems like we could at least add some validation (and a test!) that maxRetries >= 2. Do you feel like contributing a PR for this?

[barell]

I think adding a check and exception is a little improvement to developer experience as this will pop up earlier with clearer message. But I really would like to understand where does this grpc restriction come from.

I'm not familiar with grpc concepts and maybe I miss some context. I also tried to look around but couldn't find any explanation.

In the mean time I will try to put together a little PR.

Edit:
I found this document https://github.com/grpc/proposal/blob/master/A6-client-retries.md#limits-on-retries-and-hedges and as I understand, maxAttempts does include the original request. So this would explain why grpc requires at least one attempt but maybe the second attempt is required when retry policy is enabled.

So in other words, when we create grpc transport and set maxRetries to 3, we should set maxAttempts inside grpc retry policy to 4. Because it will make first attempt, then it will retry it 3 times. Does it make sense @brettmc ?

I think we could also support maxRetries with 0 and it would mean that we do not use any retry policy in grpc. Then we only make one attempt. But I want to test this as this is just my assumption.

[brettmc]

That makes sense, @barell - I think that the PR doesn't break anything new, and it could be an enhancement where maxRetries=0 drops the retry policy.