gRPC instrumentation fix and refactoring #2067
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪💚 Flaky test reportTests succeeded. Expand to view the summary
Test stats 🧪
|
SylvainJuge left a comment
SylvainJuge left a comment There was a problem hiding this comment.
LGTM, only a few questions/comments.
| } | ||
| } | ||
| | ||
| if (spanToMap != null && !spanToMap.isDiscarded()) { |
There was a problem hiding this comment.
[question] in which case does the span would have been discarded ?
There was a problem hiding this comment.
Not sure if it's a real possibility, but since we are discarding spans I think it worth the check - we don't need to continue with mappings for discarded spans.
Theoretical option: two internal gRPC channels delegate newCall() from one to the other, where the nested call already sets the created call as the real call in the corresponding DelayedClientCall, causing the active span to be discarded already before the outer call exits.
| } | ||
| | ||
| @Test | ||
| @Disabled // disabled for now as it's flaky on CI |
| <dependency> | ||
| <groupId>io.grpc</groupId> | ||
| <artifactId>grpc-netty-shaded</artifactId> | ||
| <version>${grpc.version}</version> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
[question] why do we need this extra dependency for testing ? is it required only for newer versions ?
There was a problem hiding this comment.
I used this for getting sources so I can debug an entire version. Since it's in test scope, I think worth keeping it for the same purpose.
2 participants
What does this PR do?
This PR started as a fix to an issue reported in the forum.
The issue is related to a change we did when tests of gRPC version 1.35 started failing due to duplicated client spans. In order to get rid of the extra span, we excluded
io.grpc.internal.ManagedChannelImpl$RealChannelfrom ourChannelInstrumentation. However, this broke other scenarios, so this PR restores instrumentation of said channel and adds special handling for theio.grpc.internal.DelayedClientCall(added in 1.32) instead. This new client call type is created as a placeholder and starts going through the client call lifecycle, but at some point (different between 1.32 and 1.35) it starts referring to the real client call and delegate invocations to it. The new lifecycle management allows creation of duplicated spans, but whenio.grpc.internal.DelayedClientCalls are used, their corresponding spans replace the ones created for the "real" calls once they are set as the delegate of the placeholder call, and the spans created for the real calls are discarded.In addition, we now use
ensureInstrumentedto instrument client calls, server calls, client listeners and server listeners. By doing that, we can both be very efficient in type matching and still support custom types. While customer client/server calls are sort of edge case, custom listeners are probably more expected. Our tests now include custom client call and client/server listeners.Lastly, I refactored all instrumentations to extract advice methods into separate (inner) classes, so to ensure proper dependencies visibility.
In a separate PR I intend to apply some enhancements to
SpanConcurrentHashMapand then replace all maps withinGrpcHelperwith such.Checklist