otelgrpc: Add peer attributes to spans recorded by stats handlers #4539
Conversation
|
@fatsheep9146 PTAL if this implementation makes sense. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4539 +/- ##
=======================================
- Coverage 81.1% 81.1% -0.1%
=======================================
Files 150 150
Lines 10759 10757 -2
=======================================
- Hits 8731 8729 -2
Misses 1872 1872
Partials 156 156
|
|
I think the total implementation makes sense to me! |
pellared
changed the title
pellared
changed the title
| span := trace.SpanFromContext(ctx) | ||
| attrs := peerAttr(peerFromCtx(ctx)) | ||
| span.SetAttributes(attrs...) | ||
| h.peerAttr = peerAttr(info.RemoteAddr.String()) |
There was a problem hiding this comment.
How will this work if the Handler has two connections at once?
There was a problem hiding this comment.
Good question. I will carefully test it tomorrow.
There was a problem hiding this comment.
I will also double-check that the client handles concurrent calls without races.
There was a problem hiding this comment.
I also confirm that there can be a race on the client:
==================
WARNING: DATA RACE
Write at 0x00c0000a5048 by goroutine 12:
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc.(*clientHandler).TagConn()
/home/rpajak/repos/opentelemetry-go-contrib/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go:137 +0x84
Previous read at 0x00c0000a5048 by goroutine 18:
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc.(*clientHandler).TagRPC()
/home/rpajak/repos/opentelemetry-go-contrib/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go:119 +0x1e7
There was a problem hiding this comment.
I also found out that TagConn may be also called after TagRPC (or even not at all according to my current testing).
I think that currently it may be impossible to implement.
|
I have currently no time to work on this. I try to go back to this in ~2weeks. |
|
I created grpc/grpc-go#6897 |
|
Closed per #4873 |
Fixes #4531