stats/opentelemetry: add trace event for name resolution delay #8074

vinothkumarr227 · 2025-02-10T10:40:32Z

stats/opentelemetry: add trace event for name resolution delay.

RELEASE NOTES:

stats/opentelemetry: add trace event for name resolution delay.

codecov · 2025-02-10T10:50:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.03%. Comparing base (775150f) to head (3974776).
Report is 18 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8074      +/-   ##
==========================================
- Coverage   82.25%   82.03%   -0.23%     
==========================================
  Files         393      410      +17     
  Lines       39143    40248    +1105     
==========================================
+ Hits        32197    33016     +819     
- Misses       5616     5867     +251     
- Partials     1330     1365      +35

Files with missing lines	Coverage Δ
clientconn.go	`90.56% <100.00%> (+<0.01%)`	⬆️
internal/internal.go	`100.00% <100.00%> (ø)`
stats/opentelemetry/client_metrics.go	`89.39% <100.00%> (ø)`
stats/opentelemetry/client_tracing.go	`84.00% <100.00%> (+7.52%)`	⬆️
stats/opentelemetry/opentelemetry.go	`75.00% <ø> (ø)`
stream.go	`81.55% <100.00%> (+0.03%)`	⬆️

... and 57 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

clientconn.go

stats/handlers.go

stats/opentelemetry/opentelemetry.go

stream.go

test/clientconn_test.go

…few doc issues

test/clientconn_test.go

clientconn.go

stats/opentelemetry/e2e_test.go

test/clientconn_test.go

dfawley · 2025-04-01T21:48:02Z

internal/internal.go

+	// When set, the function should be called before the stream enters
+	// the blocking state.


Suggested change

// When set, the function should be called before the stream enters

// the blocking state.

// When set, the function will be called before the stream enters

// the blocking state.

Nit: "Should" implies uncertainty.

dfawley · 2025-04-01T21:49:25Z

clientconn.go

+	if internal.NewStreamWaitingForResolver != nil {
+		internal.NewStreamWaitingForResolver()
+	}


Suggested change

if internal.NewStreamWaitingForResolver != nil {

internal.NewStreamWaitingForResolver()

}

internal.NewStreamWaitingForResolver()

(Combine with suggestion below)

dfawley · 2025-04-01T21:49:37Z

internal/internal.go

+	// used in tests to synchronize resolver updates and avoid race conditions.
+	// When set, the function should be called before the stream enters
+	// the blocking state.
+	NewStreamWaitingForResolver func()


Suggested change

NewStreamWaitingForResolver func()

NewStreamWaitingForResolver = func() {}

dfawley · 2025-04-01T21:50:50Z

test/clientconn_test.go

+	// Add grpcsync.Event to coordinate timing
+	resolutionWait := grpcsync.NewEvent()
+	prevHook := internal.NewStreamWaitingForResolver
+	internal.NewStreamWaitingForResolver = func() { _ = resolutionWait.Fire() }


Suggested change

internal.NewStreamWaitingForResolver = func() { _ = resolutionWait.Fire() }

internal.NewStreamWaitingForResolver = func() { resolutionWait.Fire() }

I think this won't fail vet?

Yeah, it won’t fail vet. I’ve made the changes.

dfawley · 2025-04-01T21:52:40Z

stats/opentelemetry/e2e_test.go

+				if err != nil {
+					return err
+				}
+				if err := stream.Send(&testpb.StreamingOutputCallRequest{}); err != nil && err != io.EOF {


Why the io.EOF check here? The server should never exit until the client does CloseSend.

Thanks for the suggestion; I have removed it.

dfawley · 2025-04-01T21:55:33Z

stats/opentelemetry/e2e_test.go

+			time.AfterFunc(100*time.Millisecond, func() {
+				rb.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: ss.Address}}})
+			})


This will have the same race as in the other tests. It should inject something to hook the resolver blocking, too.

I’ve updated the test to set this to a grpcsync.Event.Fire

dfawley · 2025-04-01T21:55:46Z

stats/opentelemetry/e2e_test.go

+	if !match {
+		t.Errorf("Expected span not found: %q (kind: %s)", want.name, want.spanKind)
+	}
+


Suggested change

dfawley · 2025-04-01T21:58:06Z

stats/opentelemetry/e2e_test.go

+						headerAttempts := len(md["grpc-previous-rpc-attempts"])
+						if headerAttempts >= 1 {
+							return &testpb.SimpleResponse{}, nil
+						}
+						return nil, status.Error(codes.Unavailable, fmt.Sprintf("retry (%d)", headerAttempts))


I think you want something more like this

Suggested change

headerAttempts := len(md["grpc-previous-rpc-attempts"])

if headerAttempts >= 1 {

return &testpb.SimpleResponse{}, nil

}

return nil, status.Error(codes.Unavailable, fmt.Sprintf("retry (%d)", headerAttempts))

headerAttempts := 0

if h := md["grpc-previous-rpc-attempts"]; len(h) > 0 {

headerAttempts = h[0]

}

if headerAttempts < 2 {

return nil, status.Errorf(codes.Unavailable, "retry (%d)", headerAttempts)

}

return &testpb.SimpleResponse{}, nil

dfawley · 2025-04-01T21:58:21Z

stats/opentelemetry/e2e_test.go

+				return &stubserver.StubServer{
+					FullDuplexCallF: func(stream testgrpc.TestService_FullDuplexCallServer) error {
+						md, _ := metadata.FromIncomingContext(stream.Context())
+						headerAttempts := len(md["grpc-previous-rpc-attempts"])


Similar to above.

dfawley · 2025-04-01T22:00:36Z

test/clientconn_test.go

+	}
+}
+
+func waitForReady(cc *grpc.ClientConn) error {


Use:

https://pkg.go.dev/google.golang.org/grpc/internal/testutils#AwaitState

instead please

dfawley · 2025-04-03T20:41:07Z

stats/opentelemetry/e2e_test.go

+							if val, err := strconv.Atoi(h[0]); err == nil {
+								headerAttempts = val
+							}


This is test code, so you don't need to even look at the error (and zero is returned on a parse error anyway).

Suggested change

if val, err := strconv.Atoi(h[0]); err == nil {

headerAttempts = val

}

headerAttempts, _ = strconv.Atoi(h[0])

dfawley · 2025-04-03T20:42:39Z

stats/opentelemetry/e2e_test.go

-							return status.Error(codes.Unavailable, fmt.Sprintf("retry (%d)", headerAttempts))
+						headerAttempts := 0
+						if h := md["grpc-previous-rpc-attempts"]; len(h) > 0 {
+							if val, err := strconv.Atoi(h[0]); err == nil {


…8074)

vinothkumarr227 added 2 commits February 10, 2025 10:38

Added add trace event for name resolution delay

d773e44

small tweaks

126b42f

vinothkumarr227 added 15 commits February 10, 2025 10:52

Removed unused parameter

dad6621

Removed unused parameter

37e1803

Fixed the duplicate import issues

9ae5cc8

refactor: simplify tests, fix SubConn deprecation, and add docs

f9b00e9

refactor: add function documentation and update variable names

3ce7799

Add test for 'Name resolution completed with delay' event check

76dc143

small tweaks

82f3ede

small tweaks

2ec49a9

Fixed the test cases

3af52f4

Added add trace event for name resolution delay

c0affc5

small tweaks

323b663

small tweaks

b76ed24

small tweaks

bc99c93

Fixed the name resolutions delay event test issues

e86b142

small tweaks

27914a2

arjan-bal requested a review from purnesh42H February 17, 2025 07:16

arjan-bal assigned purnesh42H Feb 17, 2025

arjan-bal added this to the 1.71 Release milestone Feb 17, 2025

arjan-bal added Type: Feature New features or improvements in behavior Area: Observability Includes Stats, Tracing, Channelz, Healthz, Binlog, Reflection, Admin, GCP Observability labels Feb 17, 2025

purnesh42H reviewed Feb 19, 2025

View reviewed changes

purnesh42H assigned vinothkumarr227 and unassigned purnesh42H Feb 19, 2025

arjan-bal modified the milestones: 1.71 Release, 1.72 Release Feb 19, 2025

vinothkumarr227 added 2 commits February 19, 2025 13:09

Refactor the integration of nameResolutionDelay in RPCInfo and fix a …

63bb794

…few doc issues

small tweaks

a4b6281

dfawley reviewed Mar 28, 2025

View reviewed changes

purnesh42H reviewed Mar 28, 2025

View reviewed changes

test/clientconn_test.go Outdated Show resolved Hide resolved

purnesh42H requested a review from dfawley March 28, 2025 15:35

purnesh42H assigned dfawley and unassigned vinothkumarr227 Mar 28, 2025

dfawley mentioned this pull request Mar 28, 2025

stats/opentelemetry: separate out interceptors for tracing and metrics #8063

Merged

dfawley assigned vinothkumarr227 and unassigned dfawley Mar 28, 2025

vinothkumarr227 added 3 commits April 1, 2025 07:27

Fixed the review changes

92175ab

small tweaks

200b04d

Fixed the stub issues

1eb3583

purnesh42H assigned dfawley and unassigned vinothkumarr227 Apr 1, 2025

dfawley reviewed Apr 1, 2025

View reviewed changes

dfawley assigned vinothkumarr227 and unassigned dfawley Apr 1, 2025

Fixed the review changes

fc51963

vinothkumarr227 requested a review from dfawley April 2, 2025 11:38

eshitachandwani modified the milestones: 1.72 Release, 1.73 Release Apr 3, 2025

purnesh42H assigned dfawley and unassigned vinothkumarr227 Apr 3, 2025

dfawley approved these changes Apr 3, 2025

View reviewed changes

dfawley assigned vinothkumarr227 and unassigned dfawley Apr 3, 2025

small tweaks

3974776

purnesh42H merged commit ce35fd4 into grpc:master Apr 4, 2025
15 checks passed

purnesh42H pushed a commit to purnesh42H/grpc-go that referenced this pull request Apr 13, 2025

stats/opentelemetry: add trace event for name resolution delay (grpc#…

7bf3c8e

…8074)

janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Apr 23, 2025

stats/opentelemetry: add trace event for name resolution delay (grpc#…

1f03b3e

…8074)

vinothkumarr227 mentioned this pull request May 28, 2025

stats/opentelemetry: record retry attempts from clientStream #8342

Open

		// When set, the function should be called before the stream enters
		// the blocking state.

	NewStreamWaitingForResolver func()
	NewStreamWaitingForResolver = func() {}

	internal.NewStreamWaitingForResolver = func() { _ = resolutionWait.Fire() }
	internal.NewStreamWaitingForResolver = func() { resolutionWait.Fire() }

stats/opentelemetry: add trace event for name resolution delay #8074

stats/opentelemetry: add trace event for name resolution delay #8074

Conversation

vinothkumarr227 commented Feb 10, 2025 • edited by purnesh42H Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vinothkumarr227 commented Feb 10, 2025 •

edited by purnesh42H

Loading

codecov bot commented Feb 10, 2025 •

edited

Loading