Skip to content

[instrumentation/google.golang.org/grpc/otelgrpc] Do not assume HandleRPC receives a gRPCContext #4825

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.

Sign up for GitHub

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

Merged
MadVikingGod merged 8 commits into from
Jan 18, 2024
+12 −5
Merged

[instrumentation/google.golang.org/grpc/otelgrpc] Do not assume HandleRPC receives a gRPCContext #4825

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bbff530
[instrumentation/google.golang.org/grpc/otelgrpc] Add safeguard to Ha…
mx-psi Jan 17, 2024
d263c04
Add changelog entry
mx-psi Jan 17, 2024
d67730c
Create and handle error
mx-psi Jan 17, 2024
377c541
Make check a bit more clear
mx-psi Jan 17, 2024
a4c3363
Update instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go
mx-psi Jan 17, 2024
60fbb4e
Remove unused code
mx-psi Jan 17, 2024
2eb2a49
Merge remote-tracking branch 'upstream/main' into mx-psi/grpc-safeguard
mx-psi Jan 18, 2024
9be483c
additional check for gctx
mx-psi Jan 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
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
10 changes: 7 additions & 3 deletions instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go
View file
Open in desktop
Comment thread
mx-psi marked this conversation as resolved.
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, ok := ctx.Value(gRPCContextKey{}).(*gRPCContext)
if ok && gctx != nil {
metricAttrs = make([]attribute.KeyValue, 0, len(gctx.metricAttrs)+1)
Comment thread
mx-psi marked this conversation as resolved.
metricAttrs = append(metricAttrs, gctx.metricAttrs...)

@seh seh Jan 17, 2024

Copy link
Copy Markdown

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.

@mx-psi mx-psi Jan 18, 2024

Copy link
Copy Markdown
Member Author

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

}
wctx := withoutCancel(ctx)

switch rs := rs.(type) {
Expand Down