Skip to content

Commit

Permalink
[instrumentation/google.golang.org/grpc/otelgrpc] Do not assume Handl…
Browse files Browse the repository at this point in the history
…eRPC receives a gRPCContext (#4825)

* [instrumentation/google.golang.org/grpc/otelgrpc] Add safeguard to HandleRPC

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

* Add changelog entry

* Create and handle error

* Make check a bit more clear

* Update instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>

* Remove unused code

* additional check for gctx

---------

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
  • Loading branch information
mx-psi and Aneurysm9 committed Jan 18, 2024
1 parent a28c68b commit 40290ea
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 5 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
### Fixed

- Fix `NewServerHandler` in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc` to correctly set the span status depending on the gRPC status. (#4587)
- The `stats.Handler` from `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc` now does not crash when receiving an unexpected context. (#4825)
- Update `go.opentelemetry.io/contrib/detectors/aws/ecs` to fix the task ARN when it is not valid. (#3583)
- Do not panic in `go.opentelemetry.io/contrib/detectors/aws/ecs` when the container ARN is not valid. (#3583)

Expand Down
16 changes: 11 additions & 5 deletions instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,14 @@ func (h *clientHandler) HandleConn(context.Context, stats.ConnStats) {

func (c *config) handleRPC(ctx context.Context, rs stats.RPCStats, isServer bool) { // nolint: revive // isServer is not a control flag.
span := trace.SpanFromContext(ctx)
gctx, _ := ctx.Value(gRPCContextKey{}).(*gRPCContext)
var metricAttrs []attribute.KeyValue
var messageId int64
metricAttrs := make([]attribute.KeyValue, 0, len(gctx.metricAttrs)+1)
metricAttrs = append(metricAttrs, gctx.metricAttrs...)

gctx, _ := ctx.Value(gRPCContextKey{}).(*gRPCContext)
if gctx != nil {
metricAttrs = make([]attribute.KeyValue, 0, len(gctx.metricAttrs)+1)
metricAttrs = append(metricAttrs, gctx.metricAttrs...)
}

switch rs := rs.(type) {
case *stats.Begin:
Expand Down Expand Up @@ -199,8 +203,10 @@ func (c *config) handleRPC(ctx context.Context, rs stats.RPCStats, isServer bool
elapsedTime := float64(rs.EndTime.Sub(rs.BeginTime)) / float64(time.Millisecond)

c.rpcDuration.Record(ctx, elapsedTime, metric.WithAttributes(metricAttrs...))
c.rpcRequestsPerRPC.Record(ctx, atomic.LoadInt64(&gctx.messagesReceived), metric.WithAttributes(metricAttrs...))
c.rpcResponsesPerRPC.Record(ctx, atomic.LoadInt64(&gctx.messagesSent), metric.WithAttributes(metricAttrs...))
if gctx != nil {
c.rpcRequestsPerRPC.Record(ctx, atomic.LoadInt64(&gctx.messagesReceived), metric.WithAttributes(metricAttrs...))
c.rpcResponsesPerRPC.Record(ctx, atomic.LoadInt64(&gctx.messagesSent), metric.WithAttributes(metricAttrs...))
}
default:
return
}
Expand Down

0 comments on commit 40290ea

Please sign in to comment.