Skip to content

stats/opentelemetry: record retry attempts from clientStream #8342

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

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

vinothkumarr227
Copy link
Contributor

@vinothkumarr227 vinothkumarr227 commented May 19, 2025

Fixes: #8299

RELEASE NOTES:

  • stats/opentelemetry: Retry attempts (grpc.previous-rpc-attempts) are now recorded as span attributes for non-transparent client retries.

Copy link

codecov bot commented May 19, 2025

Codecov Report

Attention: Patch coverage is 84.61538% with 4 lines in your changes missing coverage. Please review.

Project coverage is 82.26%. Comparing base (0100d21) to head (8de4d5e).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
stats/opentelemetry/client_tracing.go 60.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8342      +/-   ##
==========================================
- Coverage   82.27%   82.26%   -0.02%     
==========================================
  Files         413      414       +1     
  Lines       40430    40437       +7     
==========================================
  Hits        33264    33264              
- Misses       5796     5803       +7     
  Partials     1370     1370              
Files with missing lines Coverage Δ
stats/opentelemetry/client_metrics.go 85.16% <100.00%> (+0.08%) ⬆️
stats/opentelemetry/opentelemetry.go 77.90% <ø> (ø)
stats/opentelemetry/server_tracing.go 74.19% <100.00%> (+0.86%) ⬆️
stats/opentelemetry/trace.go 90.32% <100.00%> (+1.43%) ⬆️
stats/opentelemetry/client_tracing.go 83.09% <60.00%> (-4.41%) ⬇️

... and 18 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@purnesh42H purnesh42H added Type: Bug Area: Observability Includes Stats, Tracing, Channelz, Healthz, Binlog, Reflection, Admin, GCP Observability labels May 19, 2025
@purnesh42H purnesh42H added this to the 1.73 Release milestone May 19, 2025
Copy link
Contributor

@purnesh42H purnesh42H left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember we had separate tests for retries. This change should only affect that test. This change shouldn't affect tests which are doing only single attempt.

Copy link
Contributor

@purnesh42H purnesh42H left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vinothkumarr227 have you tested this to ensure its working correctly? I was under impression that TestTraceSpan_WithRetriesAndNameResolutionDelay will need changes for expected values. I think its not working as expected because you are not setting the count back to ctx after incrementing.

@vinothkumarr227 vinothkumarr227 removed their assignment Jun 10, 2025
@arjan-bal arjan-bal self-assigned this Jun 11, 2025
@vinothkumarr227 vinothkumarr227 removed their assignment Jun 12, 2025
@arjan-bal arjan-bal self-assigned this Jun 16, 2025
// increment previous rpc attempts applicable for next attempt
atomic.AddUint32(&ai.previousRPCAttempts, 1)
}
if rs.Client {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to stop setting the retry attributes in the server side?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes — per gRPC OpenTelemetry tracing proposal (A72), retry information is tracked only on the client side. On the server side, there is a single span per RPC (Recv..), and no per-attempt retries are recorded. Therefore, we do not set retry-related attributes like previous-rpc-attempts or transparent-retry on server spans.

// Increment retry count for the next attempt if not a transparent
// retry.
if !rs.IsTransparentRetryAttempt && rs.Client {
ci := getCallInfo(ai.ctx)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to get ci again. The reason to use the pointer is that we can increment ai.previousRPCAttempts and the change gets reflected in ci.previousRPCAttempts automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback! I’ve removed the redundant getCallInfo call.

span.SetAttributes(attrs...)
// Increment retry count for the next attempt if not a transparent
// retry.
if !rs.IsTransparentRetryAttempt && rs.Client {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we want to increment attempts for only non-transparent retries? Why did we stop incrementing the counter for transparent retries?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only increment the attempts count for non-transparent retries because, per the gRPC OpenTelemetry tracing spec (A72), the previous-rpc-attempts.

@vinothkumarr227 vinothkumarr227 removed their assignment Jun 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Observability Includes Stats, Tracing, Channelz, Healthz, Binlog, Reflection, Admin, GCP Observability Type: Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Retry Attempts are not available in client-stats
4 participants