WIP: contrib/google.golang.org/grpc: trace grpc encoding #2959
Conversation
github-actions
bot
added
the
apm:ecosystem
rarguelloF
force-pushed
the
rarguelloF/trace-grpc-encoding
branch
from
c5d00c6
to
6028bfe
Compare
BenchmarksBenchmark execution time: 2024-10-31 16:54:21 Comparing candidate commit 3081731 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 58 metrics, 0 unstable metrics. scenario:BenchmarkTracerAddSpans-24
|
| google.golang.org/grpc v1.64.1 // indirect | ||
| google.golang.org/protobuf v1.34.2 // indirect | ||
| google.golang.org/genproto/googleapis/rpc v0.0.0-20241021214115-324edc3d5d38 // indirect | ||
| google.golang.org/grpc v1.67.1 // indirect |
There was a problem hiding this comment.
@rarguelloF Is this upgrade (and related) needed? If yes, we'll have to clearly state we support this integration from a specific version.
There was a problem hiding this comment.
Is this upgrade (and related) needed?
encoding.RegisterCodecV2 was not available on the previous version (in the one we were using, there was encoding.RegisterCodec). I am unsure what happens in someone uses the latest grpc, which registers the default codec using the V2 function, and we register ours using the V1.
we'll have to clearly state we support this integration from a specific version.
I think this is implicit from the go.mod right? since user's grpc version will automatically upgrade when they use this dd-trace-go version.
There was a problem hiding this comment.
We could add a comment just to make sure we also know from which version is available. WDYT?
| var ( | ||
| clientMarshalSpan = spans[0] | ||
| clientUnmarshalSpan = spans[1] | ||
| serverSpan = spans[2] | ||
| serverMarshalSpan = spans[3] | ||
| serverUnmarshalSpan = spans[4] | ||
| clientSpan = spans[5] | ||
| rootSpan = spans[6] | ||
| ) |
There was a problem hiding this comment.
Is this order always stable? It looks like a potential flaky test.
There was a problem hiding this comment.
I think it should be, the sequence of the operations should be this. The existing tests assume the order of the server / client spans and don't seem to be flaky, but if it turns out to be flaky we can change this to find the spans by name.
What does this PR do?
Motivation
Reviewer's Checklist
Unsure? Have a question? Request a review!