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.

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

Jump to bottom

Support Custom Metric Attributes Per Request #6281

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+237 −68
Open

Support Custom Metric Attributes Per Request #6281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 @@ -15,6 +15,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Add the `WithSource` option to the `go.opentelemetry.io/contrib/bridges/otelslog` log bridge to set the `code.*` attributes in the log record that includes the source location where the record was emitted. (#6253)
- Add `ContextWithStartTime` and `StartTimeFromContext` to `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`, which allows setting the start time using go context. (#6137)
- Set the `code.*` attributes in `go.opentelemetry.io/contrib/bridges/otelzap` if the `zap.Logger` was created with the `AddCaller` or `AddStacktrace` option. (#6268)
- Add new `WithMetricsAttributesFn` option to allow setting dynamic, per-request metric attributes in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`. (#6281)

### Changed

Expand Down
38 changes: 30 additions & 8 deletions instrumentation/google.golang.org/grpc/otelgrpc/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
package otelgrpc // import "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc"

import (
"context"

"google.golang.org/grpc/stats"

"go.opentelemetry.io/otel"
Expand All @@ -29,21 +31,26 @@ const (
// Deprecated: Use stats handlers instead.
type InterceptorFilter func(*InterceptorInfo) bool

// MetricAttributesFn is a optional function which will be called per request returning a slice
// of attribute.KeyValue to be appended to metric attributes recorded by the stats.Handler.
type MetricAttributesFn func(ctx context.Context, payload any) []attribute.KeyValue

// Filter is a predicate used to determine whether a given request in
// should be instrumented by the attached RPC tag info.
// A Filter must return true if the request should be instrumented.
type Filter func(*stats.RPCTagInfo) bool

// config is a group of options for this instrumentation.
type config struct {
Filter Filter
InterceptorFilter InterceptorFilter
Propagators propagation.TextMapPropagator
TracerProvider trace.TracerProvider
MeterProvider metric.MeterProvider
SpanStartOptions []trace.SpanStartOption
SpanAttributes []attribute.KeyValue
MetricAttributes []attribute.KeyValue
Filter Filter
InterceptorFilter InterceptorFilter
Propagators propagation.TextMapPropagator
TracerProvider trace.TracerProvider
MeterProvider metric.MeterProvider
SpanStartOptions []trace.SpanStartOption
SpanAttributes []attribute.KeyValue
MetricAttributes []attribute.KeyValue
MetricAttributesFn MetricAttributesFn

ReceivedEvent bool
SentEvent bool
Expand Down Expand Up @@ -303,3 +310,18 @@ func (o metricAttributesOption) apply(c *config) {
func WithMetricAttributes(a ...attribute.KeyValue) Option {
return metricAttributesOption{a: a}
}

type metricAttributesFnOption struct{ f MetricAttributesFn }

func (o metricAttributesFnOption) apply(c *config) {
if o.f != nil {
c.MetricAttributesFn = o.f
}
}

// WithMetricAttributesFn returns an Option to set the MetricAttributesFn function which
// will be called on each gRPC request to append metric attributes to recorded instruments
// by the stats.Handler.
func WithMetricAttributesFn(f MetricAttributesFn) Option {
return metricAttributesFnOption{f: f}
}
3 changes: 1 addition & 2 deletions instrumentation/google.golang.org/grpc/otelgrpc/internal/test/test_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,10 @@ import (
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/grpclog"
testpb "google.golang.org/grpc/interop/grpc_testing"
"google.golang.org/grpc/metadata"
"google.golang.org/grpc/status"
"google.golang.org/protobuf/proto"

testpb "google.golang.org/grpc/interop/grpc_testing"
)

var (
Expand Down
10 changes: 10 additions & 0 deletions instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,16 @@ func (c *config) handleRPC(ctx context.Context, rs stats.RPCStats, isServer bool
case *stats.InPayload:
if gctx != nil {
messageId = atomic.AddInt64(&gctx.inMessages, 1)

// If a MetricAttributesFn is defined call this function and update the gRPCContext with the metric attributes
// returned by ths function.
if f := c.MetricAttributesFn; f != nil {
attrs := f(ctx, rs.Payload)
Copy link
Member

Choose a reason for hiding this comment

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

What about stats.OutPayload?


gctx.metricAttrs = append(gctx.metricAttrs, attrs...)
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't seem like we need to modify gctx here?
This also causes a race problem.

Copy link
Author

Choose a reason for hiding this comment

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

So the #6092 also modified this, otherwise the metric attributes won't be propagated on the *stats.End call where the majority of the metrics are recorded, since I don't think *stats.End has access to the Payload although I will check that.

Copy link
Author

Choose a reason for hiding this comment

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

InPayload is the only stats.RPCStats implementation that has access to the request payload. So the this is only time we can call the MetricAttributesFn to be able to append too gRPCContext.metricAttrs, I can put a lock around metricAttrs protect against races?

Copy link
Member

Choose a reason for hiding this comment

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

I'm very hesitant about updating a pointer to struct that is stored as a context value. That really looks like a smell.
Unfortunately, we can't do things cleanly (provide a new context with the updated value), as we can't provide a new context as return value.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, this doesn't seem like it's going to be possible then due to the limitations in how the stats.Handler works.

Copy link
Author

Choose a reason for hiding this comment

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

@dmathieu any thoughts on if we should continue with this or not?

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to have other opinions.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I'll leave it with you @dmathieu 👍

Copy link
Author

Choose a reason for hiding this comment

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

Any updates on this?

metricAttrs = append(metricAttrs, attrs...)
}

c.rpcInBytes.Record(ctx, int64(rs.Length), metric.WithAttributeSet(attribute.NewSet(metricAttrs...)))
}

Expand Down
Loading
Loading