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

gRPC interceptors #483

Merged
merged 5 commits into from
Aug 11, 2022
Merged

gRPC interceptors #483

merged 5 commits into from
Aug 11, 2022

Conversation

emadolsky
Copy link
Contributor

@emadolsky emadolsky commented Aug 3, 2022

HTTP Middlewares for observability and ratelimiting were not implemented for go-carbon gRPC service. This PR adds those functionality alongside small mandatory refactors in order to have less code copies.

In order to implement middlewares, gRPC interceptors are used, and as currently only Render RPC is implemented which is a stream rpc, stream server interceptors are defined and used. For following RPCs (i.e. Find, Info, etc.) we should have unary stream interceptors as well. Which wouldn't be much different with the current implementations.

Three interceptors are created:

  • StreamServerTimeHandler: Equivalent to httputil.TimeHandler for gRPC stream server.
  • StreamServerStatusMetricHandler: Implements status-related part of TraceHandler. Tracing is not yet implemented for gRPC endpoints.
  • StreamServerRatelimitHandler: Equivalent to CarbonserverListener.rateLimitRequest for gRPC stream server. Some refactor is done here.

A few parts of the middleware logic are still absent for gRPC:

  • Tracing
  • Connection Tracking (Which seems unused?)

bucket := listener.bucketRequestTimes(t)
if bucket >= listener.buckets {
listener.logger.Info("slow request",
zap.String("url", req.URL.RequestURI()),

Check failure

Code scanning / CodeQL

Log entries created from user input

This log write receives unsanitized user input from [here](1).
@emadolsky emadolsky marked this pull request as ready for review August 8, 2022 09:26
@emadolsky emadolsky merged commit 307d623 into master Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants