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[bot]]

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%)	:arrow_down:
...ion/google.golang.org/grpc/otelgrpc/interceptor.go	84.2% <66.6%> (-2.4%)	:arrow_down:
...google.golang.org/grpc/otelgrpc/filters/filters.go	94.6% <94.6%> (ø)

[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]

@pellared @Aneurysm9 thank you both for the comments and suggestions. I updated the branch and it should look better now.

[ymotongpoo]

@Aneurysm9 @dashpole I'm trying to move filters to the subsidiary filters package and keep newXXXInterceptorInfo private in otelgrpc, but that requires either of the followings to achieve the restructuring:

1. to expose all fields of filters.InterceptorInfo to create the object in newXXXInterceptorInfo
2. to prepare a public constructor method to create the filters.InterceptorInfo object

Either way make things uglier, and either of them has pros/cons.

• Option 1:
  • Implementation is straight forward, just replacing private field names with public ones
  • It unnecessary exposes fields
• Option 2:
  • We can keep the fields of filter.InterceptorInfo private
  • The constructor requires the values of all fields as arguments regardless of actual fields used (eg. for StreamServer, it only uses ssinfo and typ, so they need to explicitly fill the other arguments with zero values when calling the constructor.)
  • The original motivation we keep newXXXInterceptorInfo functions private is not to expose the unnecessary functions for users, but the intention will be a bit ruined.

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.

[ymotongpoo]

@Aneurysm9 Kindly ping for your comments on the visibility of the fields IntercepterInfo, as I can't move forward without your comments. It's been 3 weeks since the beginning of the thread and it would be great if I can make this PR get ready for the merge in this week.

[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? https://github.com/open-telemetry/opentelemetry-go-contrib/pull/2572#issuecomment-1197595873

[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.

[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 https://github.com/open-telemetry/opentelemetry-go-contrib/pull/512#discussion_r602181665.

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
}

[ymotongpoo]

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