Add gRPC Filter

[ymotongpoo]

After the discussion in #2571 and looking up the whole discussion in #512, I changed the direction just to focus on solving the issue #355 and other possible demand to filter the request to trace.

Most of the implementation are from #512. I tried to follow the package structure to match otelhttp.Filter but because of the difference between http.Request and the objects passed to four types of interceptors, some clumsy structs/functions remain to the surface.

[codecov]

Codecov Report

Merging #2572 (0c89101) into main (bfa71cf) will increase coverage by 0.1%.
The diff coverage is 81.1%.

@@           Coverage Diff           @@
##            main   #2572     +/-   ##
=======================================
+ Coverage   74.3%   74.4%   +0.1%     
=======================================
  Files        144     145      +1     
  Lines       6569    6681    +112     
=======================================
+ Hits        4883    4973     +90     
- Misses      1543    1561     +18     
- Partials     143     147      +4     

Impacted Files	Coverage Δ
...ation/google.golang.org/grpc/otelgrpc/grpctrace.go	50.9% <0.0%> (-6.6%)	⬇️
...ion/google.golang.org/grpc/otelgrpc/interceptor.go	84.2% <66.6%> (-2.4%)	⬇️
...google.golang.org/grpc/otelgrpc/filters/filters.go	94.6% <94.6%> (ø)

[dashpole]

lgtm otherwise

[dashpole]

It feels a bit strange to make this and NewXInterceptorInfo functions public, as we don't expect users to ever need to use or call them. Would it be better to leave filters in the otelgrpc package? It also eliminates the filters.Filter stutter for naming.

[ymotongpoo]

Yes, I totally agree! This is because I created filters package to follow the package structure in otelhttp. (It has otelhttp.filters) I moved around the NewXXXInterceptors and InterceptorInfo back and forth among otelgrpc, otelgrpc/internal and otelgrpc/filters to eliminate unnecessary public members, but I gave up and settled down to the current state. If it's totally ok to move everything under otelgrpc and otelgrpc/internal directories, then I'm happy to do it.

[Aneurysm9]

I put the filter implementations in a separate package because most often they'll be referred to by users and filters.MethodPrefix() makes more sense to me than otelhttp.MethodPrefix(), doubly so for the meta-filters. otelhttp.Any() is less indicative of behavior than filters.Any().

As for exporting the interceptor info struct, I think it might be worthwhile to do so as without it users cannot implement their own filters. That would also necessitate exporting its fields. I think I'm fine with not exporting it for now, but if there's a way to ensure this isn't a one-way door and we can change our minds later that would be great.

[dashpole]

I think:

• filters.MethodPrefix feels better than otelgrpc.MethodPrefix
• filters.Any feels better than otelgrpc.Any (or otelgrpc.FilterAny)
• otelgrpc.Filter feels better than filters.Filter
• otelgrpc.InterceptorInfo feels better than filters.InterceptorInfo

To get that, we would need to make InterceptorInfo (and its fields) public, but could keep NewXInterceptorInfo private, which I'm OK with. Would that work @Aneurysm9?

[ymotongpoo]

Thanks for the comment, @Aneurysm9. For now, I moved everything under otelgrpc based on David's comment, but I'm happy to move those back to subsidiary filters package. Let me know what I should do.

[ymotongpoo]

@dashpole thanks for the comment.

@Aneurysm9 does it make sense to you to have everything for filter under otelgrpc and otelgrpc/internal to eliminate unnecessary public functions and struct? I'm asking you because you submit otelhttp/filters and also suggested the common object for filters in #512.

[ymotongpoo]

@dashpole I moved everything under otelgrpc and now unnecessary public members are eliminated. Could you approve this?

[ymotongpoo]

@dashpole Thank you David for the review and approval!

To maintainers: should I change anything else?

[ymotongpoo]

@MrAlias thank you for arranging this PR as the milestone. Could you help accelerate the review? I'm ready to change the code but I can't without the review from @Aneurysm9 but I have no idea how I can get help but for waiting.

[ymotongpoo]

@Aneurysm9 could you reply to this comment so that I can move forward to the acceptable change?
#2572 (comment)

[Aneurysm9]

Apologies for the delay, I was on vacation.

All filters and InterceptorInfo struct need to be in the same package, i.e. filters because it causes cyclic dependency if we keep InterceptorInfo in otelgrpc, so we need to figure out some ways to construct filters.InterceptorInfo objects out of the package.

Not necessarily. There could be a third package that contains InterceptorInfo. This would be similar to the otelhttp instrumentation that uses the http.Request as an argument to its filters.

It might also be possible to, as @dashpole suggested, use otelgrpc.Filter as the type and filter.* implement that type. I think that would then only have otelgrpc depending on itself, filter depending on otelgrpc, and then user code depending on both, as required.

[ymotongpoo]

@Aneurysm9 I appreciate your comment. I hope you had a great vacation.

Not necessarily. There could be a third package that contains InterceptorInfo. This would be similar to the otelhttp instrumentation that uses the http.Request as an argument to its filters.

The reason http.Request works in otelhttp is because it exposes its fields. If it's okay for IntercepterInfo to expose its fields as well, I'm +1 to the decision.

Also per declaring otelgrpc.Filter type, filter.* is referring to interceptorInfo's fields as of current implementation, so also from this point of view, InterceptorInfo's fields needs to be public or accessible from public methods.

Given, does it sound good to expose the fields of InterceptorInfo (I think preparing public methods is better)?

[ymotongpoo]

@Aneurysm9 I moved all filters under filters package. Those filters are in otelgrpc.Filter type and uses otelgrpc.InterceptorInfo. Also, because InterceptorInfo got smaller than what it was, I removed newXXXX constructors and directly instantiated InterceptorInfo, which are enabled by exposing its fields. This expose was required for fitler_test.go as well.

[dashpole]

Great work, and thanks for your patience, @ymotongpoo! Just a few nits.

[ymotongpoo]

@Aneurysm9 Kindly ping because it's been a week since your last comment and I updated the code regarding it.

[petenorth]

Open Telemetry in Go for grpc is unusable without this PR for our organization.

[ymotongpoo]

@Aneurysm9 @MadVikingGod @MrAlias hi, all maintainers, let me know how I can proceed from here. It's been two weeks since @Aneurysm9's last comment and I'm totally stalling.

For interface simplicity, if we don't think about abstraction for future, we can simply use method name as common input for otelgrpc.Filter and eliminate InterceptorInfo itself, as commented in #512 (comment).

Of course, if we are to use context.Context for more filters, we can add it in the current implementation of InterceptorInfo.

type InterceptorInfo struct {
	Ctx              context.Context
	Method           string
	UnaryServerInfo  *grpc.UnaryServerInfo
	StreamServerInfo *grpc.StreamServerInfo
	Typ              InterceptorType
}

Another option is just to keep method name and context when we instantiate InterceptorInfo, to eliminate InterceptorType switch. This enables InterceptorInfo to be simpler:

type InterceptorInfo struct {
    Ctx    context.Context
    // Method should be either of the followings:
    // - method passed to unary/stream client interceptor function
    // - info.FullMethodName, where info is Unary/StreamServerInfo passed to server interceptor function
    Method string
}

[Aneurysm9]

Proposed a function name change. Otherwise LGTM. Implement the change now or as a follow-on PR (or tell me I'm crazy and bring receipts :) )

[ymotongpoo]

@Aneurysm9 Thank you for the approval! Yes, I'm happy to create a follow-up PR for better implementation :)