Add ability to add custom attributes#199
Add ability to add custom attributes#199s-barr-fetch wants to merge 1 commit intoconnectrpc:mainfrom
Conversation
jhump
left a comment
There was a problem hiding this comment.
Sorry that this went so long without a review. I somehow missed the GitHub notifications from back in October.
This change seems reasonable to me, but I think we need some changes before we can merge it.
| func WithCustomAttributes(customAttributes CustomAttributes) Option { | ||
| return &customAttributesOption{customAttributes: customAttributes} | ||
| } | ||
|
|
There was a problem hiding this comment.
I think there should be separate options for tracing span attributes vs. metrics attributes. Metrics attributes result in more time series data, so there's a non-trivial concern with the number and cardinality of attributes for metrics that is not so much a concern with traces.
Also, exported functions need a Go doc comment.
| // kept else it will be removed. AttributeFilter must be safe to call concurrently. | ||
| type AttributeFilter func(connect.Spec, attribute.KeyValue) bool | ||
|
|
||
| type CustomAttributes func(context.Context, connect.AnyRequest) []attribute.KeyValue |
There was a problem hiding this comment.
This signature is not amenable to support for streaming RPCs. I think you instead want to pass all the components of AnyRequest other than the actual request message:
| type CustomAttributes func(context.Context, connect.AnyRequest) []attribute.KeyValue | |
| type CustomAttributes func(ctx context.Context, httpMethod string, requestHeaders http.Header, spec connect.Spec, peer connect.Peer) []attribute.KeyValue |
All of these same things are available for streams, which means you can use this same callback function to get custom metrics for streaming RPCs, too. (The stream interface doesn't provide an HTTP method, but that's okay because it always http.MethodPost for streams.)
|
@s-barr-fetch, the DCO check failed. In order to make it pass, you need to revise the commit with a "signed off" message. Since there's just one commit, you can easily fix it with |
This PR adds a
WithCustomAttributesoption to the interceptor that allows you to dynamically add custom attributes to spans and metrics based on the request and context.