-
Notifications
You must be signed in to change notification settings - Fork 538
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
[instrumentation/google.golang.org/grpc/otelgrpc] Do not assume HandleRPC receives a gRPCContext #4825
[instrumentation/google.golang.org/grpc/otelgrpc] Do not assume HandleRPC receives a gRPCContext #4825
Conversation
…ndleRPC To help prevent crashes such as open-telemetry/opentelemetry-collector/issues/9296, check if the context is nil before proceeding. This is inspired by: https://github.com/grpc/grpc-go/blob/ddd377f19841eae70862559c854d957d61b3b692/stats/opencensus/opencensus.go#L204-L207
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4825 +/- ##
=====================================
Coverage 81.3% 81.3%
=====================================
Files 150 150
Lines 10885 10890 +5
=====================================
+ Hits 8853 8858 +5
Misses 1873 1873
Partials 159 159
|
instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
Is it not an effect of bug in https://github.com/grpc/grpc-go? If so could you additionally report it there? References:
It would be also great to reproduce the issue and have a test that would help avoiding regression bugs. Side note: I am not against having a "defensive" code. |
gctx, ok := ctx.Value(gRPCContextKey{}).(*gRPCContext) | ||
if ok && gctx != nil { | ||
metricAttrs = make([]attribute.KeyValue, 0, len(gctx.metricAttrs)+1) | ||
metricAttrs = append(metricAttrs, gctx.metricAttrs...) |
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.
You can use copy
instead of you replace the zero on the preceding line with the length of the source slice. Alternately, consider using the slices.Clone
function if you don't really need that extra slot in the backing array. It's implemented by append
today, without the preceding call to make
.
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.
Since this was already there and I am only moving it inside the if
block, I would prefer to do this change on a separate PR if that's okay
It's not clear to me why this was not encountered before, but the examples in the grpc repo does suggest that defensive code is the way to go here: https://github.com/grpc/grpc-go/blob/ddd377f19841eae70862559c854d957d61b3b692/examples/features/stats_monitoring/statshandler/handler.go#L73 I'm not sure if this expected behaviour or a new behaviour though. |
I agree. Therefore, I am not against the change here. Still, the docs suggests that it should not happen. |
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.
@MadVikingGod IMO we can consider adding it to the upcoming release.
@mx-psi Can you please still follow-up and try to reproduce it and eventually create an issue in https://github.com/grpc/grpc-go?
Filed grpc/grpc-go/issues/6928. I still don't have a way to reproduce the issue, but I at least shared all we know so far. Feel free to comment if anything is missing there. |
To help prevent crashes such as open-telemetry/opentelemetry-collector/issues/9296, check if the context is nil before proceeding.
This is inspired by: https://github.com/grpc/grpc-go/blob/ddd377f19841eae70862559c854d957d61b3b692/stats/opencensus/opencensus.go#L204-L207