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

adding stream interceptor for logging middleware #3359

Merged
merged 11 commits into from
Sep 18, 2024

Conversation

akoserwal
Copy link
Contributor

@akoserwal akoserwal commented Jul 5, 2024

Description (what this PR does / why we need it):

Update the stream interceptor to handle stream use-case

Which issue(s) this PR fixes (resolves / be part of):

fixes #3351

Other special notes for the reviewers:

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jul 5, 2024
@xuyang2
Copy link
Contributor

xuyang2 commented Jul 9, 2024

@akoserwal
Copy link
Contributor Author

akoserwal commented Jul 9, 2024

It seems that you can use it directly"github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/logging"

Using the above can cause conflict between using kratos middleware for unary and using grpc-middleware for stream server

Does the above apply to all the middlewares: auth, circuit breaker, metadata, metrics, rate limit, recovery, selector, tracing, validate etc?

@akoserwal
Copy link
Contributor Author

@shenqidebaozi I updated the PR to have a middleware abstraction for the streaming middleware. grpc-gateway middleware also configure the unary and stream as separate interceptors configs
https://github.com/grpc-ecosystem/go-grpc-middleware/blob/main/examples/server/main.go#L136

@akoserwal
Copy link
Contributor Author

@go-kratos/contributor please take a look and provide feedback. Another proposal to unify the unary and stream interface: akoserwal#1

@shenqidebaozi
Copy link
Sponsor Member

ok, Let's discuss this design

@akoserwal
Copy link
Contributor Author

@shenqidebaozi updated the interceptor based on your suggestions

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Aug 7, 2024
@akoserwal akoserwal force-pushed the stream-compatible-middlewares branch from d70260b to ac1b5c5 Compare August 7, 2024 06:38
@akoserwal akoserwal force-pushed the stream-compatible-middlewares branch 2 times, most recently from 932ed31 to c182e94 Compare August 7, 2024 10:09
@dosubot dosubot bot added the LGTM label Aug 28, 2024
@shenqidebaozi
Copy link
Sponsor Member

@go-kratos/contributor PTAL.

transport/grpc/client.go Outdated Show resolved Hide resolved
demoManito
demoManito previously approved these changes Sep 2, 2024
@akoserwal
Copy link
Contributor Author

@shenqidebaozi Is there anything remaining? please take a look

@shenqidebaozi
Copy link
Sponsor Member

@akoserwal I think it's ready now, but I need to test it again

@shenqidebaozi shenqidebaozi merged commit e1f5dc4 into go-kratos:main Sep 18, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kratos middleware seems to only apply to unary gRPC operations
6 participants