-
Notifications
You must be signed in to change notification settings - Fork 439
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
WIP: contrib/google.golang.org/grpc: trace grpc encoding #2959
base: main
Are you sure you want to change the base?
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this order always stable? It looks like a potential flaky test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!