Skip to content
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

otelgrpc: Implement grpc.StatsHandler for trace instrumentation #3002

Merged
merged 22 commits into from
Sep 19, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
2a4bde0
[otelgrpc] refactor otelgrpc to use grpc.StatsHandler
fatsheep9146 Nov 16, 2022
7567e0d
Update instrumentation/google.golang.org/grpc/otelgrpc/README.md
fatsheep9146 Jul 26, 2023
92110c0
Update instrumentation/google.golang.org/grpc/otelgrpc/README.md
fatsheep9146 Jul 26, 2023
afebd44
Update instrumentation/google.golang.org/grpc/otelgrpc/README.md
fatsheep9146 Jul 26, 2023
abea6d6
Merge branch 'main' into otelgrpc-use-statshandler
fatsheep9146 Jul 31, 2023
03fcd32
Merge branch 'main' into otelgrpc-use-statshandler
fatsheep9146 Aug 1, 2023
51cff71
Merge branch 'main' into otelgrpc-use-statshandler
pellared Aug 10, 2023
b49dac3
Merge branch 'main' into otelgrpc-use-statshandler
fatsheep9146 Aug 11, 2023
1468039
Update instrumentation/google.golang.org/grpc/otelgrpc/README.md
fatsheep9146 Aug 30, 2023
3cd5fd5
Merge branch 'main' into otelgrpc-use-statshandler
hanyuancheung Sep 1, 2023
356c723
Merge branch 'main' into otelgrpc-use-statshandler
pellared Sep 6, 2023
7c9c665
move doc to doc.go
fatsheep9146 Sep 6, 2023
7d24364
fix failed lint check
fatsheep9146 Sep 6, 2023
550afc5
Merge branch 'main' into otelgrpc-use-statshandler
fatsheep9146 Sep 6, 2023
c22bea0
Merge branch 'main' into otelgrpc-use-statshandler
pellared Sep 7, 2023
87dbe38
format doc.go
fatsheep9146 Sep 7, 2023
3258d1d
Update instrumentation/google.golang.org/grpc/otelgrpc/doc.go
fatsheep9146 Sep 7, 2023
0213f6b
fix typo
fatsheep9146 Sep 7, 2023
0b66e2e
Merge branch 'main' into otelgrpc-use-statshandler
pellared Sep 12, 2023
1c4fb80
Merge branch 'main' into otelgrpc-use-statshandler
pellared Sep 18, 2023
494eb36
Update instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go
fatsheep9146 Sep 18, 2023
256b144
Update CHANGELOG.md
pellared Sep 18, 2023
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
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Add the new `go.opentelemetry.io/contrib/instrgen` package to provide auto-generated source code instrumentation. (#3068, #3108)
- The `go.opentelemetry.io/contrib/exporters/autoexport` package to provide configuration of trace exporters with useful defaults and envar support. (#2753, #4100, #4129, #4132, #4134)
- `WithRouteTag` in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` adds HTTP route attribute to metrics. (#615)
- [otelgrpc] implement `grpc.StatsHandler` for grpc instrumentation. (#3002)
- Add `WithSpanOptions` option in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`. (#3768)

### Fixed
Expand Down
20 changes: 20 additions & 0 deletions instrumentation/google.golang.org/grpc/otelgrpc/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# gRPC instrumentation

This library is the instrumentation library for `google.golang.org/grpc`

For now you can instrument your program which use `google.golang.org/grpc` in two ways:

- by gRPC Interceptors
- [experimental] by gPRC `stats.Handler`
pellared marked this conversation as resolved.
Show resolved Hide resolved

You can see the example of both ways in directory `./example`

fatsheep9146 marked this conversation as resolved.
Show resolved Hide resolved
**notice**: **Do not use both interceptors and stats handlers at the same time!** If so, you will get duplicated spans and the parent/child relationships between spans will also be broken.

Although the implementation `stats.Handler` in experimental stage, we strongly still recommand you to use `stats.Handler`, mainly for two reasons:
- **Functional advantages**: `stats.Handler`` has more information for user to build more flexible and granular metric, for example
fatsheep9146 marked this conversation as resolved.
Show resolved Hide resolved
- multiple different types of represent "data length": In [InPayLoad](https://pkg.go.dev/google.golang.org/grpc/stats#InPayload), there exists `Length`, `CompressedLength`, `WireLength` to denote the size of uncompressed, compressed payload data, with or without framing data. But in Interceptors, we can only got uncompressed data, and this feature is also removed due to performance problem. [#3168](https://github.com/open-telemetry/opentelemetry-go-contrib/pull/3168)
- more accurate timestamp: `InPayload.RecvTime` and `OutPayload.SentTime` records more accurate timestamp that server got and sent the message, the timestamp recorded by interceptors depends on the location of this interceptors in the total interceptor chain.
- some other use cases: for example [catch failure of decoding message](https://github.com/open-telemetry/opentelemetry-go-contrib/issues/197#issuecomment-668377700)
- **Performance advantages**: If too many interceptors are registered in a service, the interceptor chain can become too long, which increases the latency and processing time of the entire RPC call.

187 changes: 187 additions & 0 deletions instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package otelgrpc // import "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc"

import (
"context"
"sync/atomic"

grpc_codes "google.golang.org/grpc/codes"
fatsheep9146 marked this conversation as resolved.
Show resolved Hide resolved
"google.golang.org/grpc/stats"
"google.golang.org/grpc/status"

"go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/internal"
"go.opentelemetry.io/otel/codes"
semconv "go.opentelemetry.io/otel/semconv/v1.12.0"
fatsheep9146 marked this conversation as resolved.
Show resolved Hide resolved
"go.opentelemetry.io/otel/trace"
)

type gRPCContextKey struct{}

type gRPCContext struct {
messagesReceived int64
messagesSent int64
}

// NewServerHandler creates a stats.Handler for gRPC server.
func NewServerHandler(opts ...Option) stats.Handler {
h := &serverHandler{
config: newConfig(opts),
}

h.tracer = h.config.TracerProvider.Tracer(
instrumentationName,
trace.WithInstrumentationVersion(SemVersion()),
)
return h
}

type serverHandler struct {
*config
tracer trace.Tracer
}

// TagRPC can attach some information to the given context.
func (h *serverHandler) TagRPC(ctx context.Context, info *stats.RPCTagInfo) context.Context {
ctx = extract(ctx, h.config.Propagators)

name, attrs := internal.ParseFullMethod(info.FullMethodName)
attrs = append(attrs, RPCSystemGRPC)
ctx, _ = h.tracer.Start(
trace.ContextWithRemoteSpanContext(ctx, trace.SpanContextFromContext(ctx)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I suggest to implement options to configure this behavior. Similar to how net/http instrumentation works, see:

// WithPublicEndpoint configures the Handler to link the span with an incoming
// span context. If this option is not provided, then the association is a child
// association instead of a link.
func WithPublicEndpoint() Option {
return optionFunc(func(c *config) {
c.PublicEndpoint = true
})
}
// WithPublicEndpointFn runs with every request, and allows conditionnally
// configuring the Handler to link the span with an incoming span context. If
// this option is not provided or returns false, then the association is a
// child association instead of a link.
// Note: WithPublicEndpoint takes precedence over WithPublicEndpointFn.
func WithPublicEndpointFn(fn func(*http.Request) bool) Option {
return optionFunc(func(c *config) {
c.PublicEndpointFn = fn
})
}

I.e. default to private endpoints (i.e. parent relationship between spans/traces) and switch to public (i.e. link relationship) when explicitly asked to.

p.s. I think maintainers should probably decide and document how this should work across all integrations to provide consistent experience to users.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This maintains the behavior of the interceptor grpc implementation:

trace.ContextWithRemoteSpanContext(ctx, trace.SpanContextFromContext(ctx)),
. I think we should tackle this in a separate issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does maintain the existing behavior but at the same time now is the chance to introduce new default behavior, consistent with net/http instrumentation. If not now, then it'll be a breaking change for anyone who switches to the new implementation. Breaking change may be fine if this implementation is marked as experimental, I don't mind that or addressing it separately, I just need such an option. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does maintain the existing behavior but at the same time now is the chance to introduce new default behavior, consistent with net/http instrumentation. If not now, then it'll be a breaking change for anyone who switches to the new implementation. Breaking change may be fine if this implementation is marked as experimental, I don't mind that or addressing it separately, I just need such an option. Thanks!

I will open another issue to tackle this problem after this pr is merged.

name,
trace.WithSpanKind(trace.SpanKindServer),
trace.WithAttributes(attrs...),
)

gctx := gRPCContext{}
return context.WithValue(ctx, gRPCContextKey{}, &gctx)
}

// HandleRPC processes the RPC stats.
func (h *serverHandler) HandleRPC(ctx context.Context, rs stats.RPCStats) {
handleRPC(ctx, rs)
}

// TagConn can attach some information to the given context.
func (h *serverHandler) TagConn(ctx context.Context, info *stats.ConnTagInfo) context.Context {
span := trace.SpanFromContext(ctx)
attrs := peerAttr(peerFromCtx(ctx))
span.SetAttributes(attrs...)
return ctx
}

// HandleConn processes the Conn stats.
func (h *serverHandler) HandleConn(ctx context.Context, info stats.ConnStats) {
}

// NewClientHandler creates a stats.Handler for gRPC client.
func NewClientHandler(opts ...Option) stats.Handler {
h := &clientHandler{
config: newConfig(opts),
}

h.tracer = h.config.TracerProvider.Tracer(
instrumentationName,
trace.WithInstrumentationVersion(SemVersion()),
)

return h
}

type clientHandler struct {
*config
tracer trace.Tracer
}

// TagRPC can attach some information to the given context.
func (h *clientHandler) TagRPC(ctx context.Context, info *stats.RPCTagInfo) context.Context {
name, attrs := internal.ParseFullMethod(info.FullMethodName)
attrs = append(attrs, RPCSystemGRPC)
ctx, _ = h.tracer.Start(
ctx,
name,
trace.WithSpanKind(trace.SpanKindClient),
trace.WithAttributes(attrs...),
)

gctx := gRPCContext{}

return inject(context.WithValue(ctx, gRPCContextKey{}, &gctx), h.config.Propagators)
}

// HandleRPC processes the RPC stats.
func (h *clientHandler) HandleRPC(ctx context.Context, rs stats.RPCStats) {
handleRPC(ctx, rs)
}

// TagConn can attach some information to the given context.
func (h *clientHandler) TagConn(ctx context.Context, cti *stats.ConnTagInfo) context.Context {
span := trace.SpanFromContext(ctx)
attrs := peerAttr(cti.RemoteAddr.String())
span.SetAttributes(attrs...)
return ctx
}

// HandleConn processes the Conn stats.
func (h *clientHandler) HandleConn(context.Context, stats.ConnStats) {
// no-op
}

func handleRPC(ctx context.Context, rs stats.RPCStats) {
span := trace.SpanFromContext(ctx)
gctx, _ := ctx.Value(gRPCContextKey{}).(*gRPCContext)
var messageId int64

switch rs := rs.(type) {
case *stats.Begin:
case *stats.InPayload:
if gctx != nil {
messageId = atomic.AddInt64(&gctx.messagesReceived, 1)
}
span.AddEvent("message",
trace.WithAttributes(
semconv.MessageTypeReceived,
semconv.MessageIDKey.Int64(messageId),
semconv.MessageCompressedSizeKey.Int(rs.CompressedLength),
semconv.MessageUncompressedSizeKey.Int(rs.Length),
),
fatsheep9146 marked this conversation as resolved.
Show resolved Hide resolved
)
case *stats.OutPayload:
if gctx != nil {
messageId = atomic.AddInt64(&gctx.messagesSent, 1)
}

span.AddEvent("message",
trace.WithAttributes(
semconv.MessageTypeSent,
semconv.MessageIDKey.Int64(messageId),
semconv.MessageCompressedSizeKey.Int(rs.CompressedLength),
semconv.MessageUncompressedSizeKey.Int(rs.Length),
),
)
case *stats.End:
if rs.Error != nil {
s, _ := status.FromError(rs.Error)
span.SetStatus(codes.Error, s.Message())
span.SetAttributes(statusCodeAttr(s.Code()))

Check warning on line 179 in instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go

View check run for this annotation

Codecov / codecov/patch

instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go#L177-L179

Added lines #L177 - L179 were not covered by tests
} else {
span.SetAttributes(statusCodeAttr(grpc_codes.OK))
}
span.End()
default:
return
}
}
Loading
Loading