Refactor Middleware to a single set of functions for all routes #3534

ryanhall07 · 2021-06-02T02:51:07Z

The multiple sets of Middleware functions was getting confusing and
difficult to reason about. Now there is a single set of Middleware
functions that run for every route before the handler is dispatched.

A route can provide an OptionsOverride function to change the
middleware.Options passed to the Middleware for a route. This
allows a route to tweak how the Middleware runs for a route (e.g
response logging).

The set of Middleware is completely configurable on the
HandlerOptions. If not set, a set of Default middleware is used.

A CustomHandler can also provide an OptionOverride function, in addition
to the OptionOverride already set on the previous handler. This allows
further configuring the options for the CustomHandler.

The multiple sets of Middleware functions was getting confusing and difficult to reason about. Now there is a single set of Middleware functions that run for every route before the handler is dispatched. A route can provide an OptionsOverride function to change the middleware.Options passed to the Middleware for a route. This allows a route to tweak how the Middleware runs for a route (e.g response logging). The set of Middleware is completely configurable on the HandlerOptions. If not set, a set of Default middleware is used. A CustomHandler can also provide an OptionOverride function, in addition to the OptionOverride already set on the previous handler. This allows further configuring the options for the CustomHandler.

codecov · 2021-06-02T15:03:43Z

Codecov Report

Merging #3534 (279565a) into master (279565a) will not change coverage.
The diff coverage is n/a.

❗ Current head 279565a differs from pull request most recent head d8624a8. Consider uploading reports for the commit d8624a8 to get more accurate results

@@          Coverage Diff           @@
##           master   #3534   +/-   ##
======================================
  Coverage    55.9%   55.9%           
======================================
  Files         549     549           
  Lines       61437   61437           
======================================
  Hits        34373   34373           
  Misses      23964   23964           
  Partials     3100    3100

Flag	Coverage Δ
aggregator	`57.1% <0.0%> (ø)`
cluster	`∅ <0.0%> (∅)`
collector	`54.3% <0.0%> (ø)`
dbnode	`60.4% <0.0%> (ø)`
m3em	`46.4% <0.0%> (ø)`
metrics	`19.8% <0.0%> (ø)`
msg	`74.6% <0.0%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 279565a...d8624a8. Read the comment docs.

ryanhall07 · 2021-06-03T02:58:19Z

src/query/api/v1/handler/prometheus/native/middleware.go

-	InstrumentOpts instrument.Options
-	Clock          clockwork.Clock
+// WithQueryParams adds the query request parameters to the middleware options.
+var WithQueryParams middleware.OverrideOptions = func(opts middleware.Options) middleware.Options {


the query response logging and non-query response logging were merged into a single middleware func that does all response logging. this has the added benefit of improving the non-query response logging with things we only added to the query logging, like status code.

now the query endpoints need to register this option to add the query params as log fields. while I could have hacked and just looked for query, this seemed cleaner. the middleware pkg should know nothing about the native pkg.

additionally this override style is used for the metrics middleware as well for similar reasons. in order to extract the query params from the request.

ryanhall07 · 2021-06-03T03:00:26Z

src/query/api/v1/httpd/handler.go

+		if override != nil {
+			opts = override(opts)
+		}
+		if customMiddle[route] != nil {


now we support a custom handler overriding the middleware options of a prev handler.

ryanhall07 · 2021-06-03T03:01:09Z

src/query/api/v1/middleware/logging.go

+				}
+				fields = appendM3Headers(r.Header, fields)
+				fields = appendM3Headers(w.Header(), fields)
+				if opts.Logging.Fields != nil {


this will make it much easier to add custom headers to the log fields in the future

ryanhall07 · 2021-06-03T03:01:39Z

src/query/api/v1/middleware/middleware.go

+
+// Default is the default list of middleware functions applied if no middleware functions are set in the
+// HandlerOptions.
+func Default(opts Options) []mux.MiddlewareFunc {


now there is a single set of middleware for all routes.

ryanhall07 · 2021-06-03T03:02:13Z

src/query/api/v1/middleware/middleware.go

-	return []mux.MiddlewareFunc{
-		Cors(),
-		Tracing(opentracing.GlobalTracer(), opts.InstrumentOpts),
+		Source(opts),


this means Source runs for every route. the impl of Source changes to short circuit with a M3-Source header present

ryanhall07 · 2021-06-03T03:04:14Z

src/query/api/v1/middleware/source.go

+// Source adds the headers.SourceHeader value to the request context.
+// Installing this middleware function allows application code to access the typed source value using FromContext.
+// Additionally a source log field is added to the request scope logger.
+func Source(opts Options) mux.MiddlewareFunc {


this had to move to the middleware pkg so the SourceOptions could be part of the MiddlewareOptions.

This is a tradeoff for allowing routes to overrides options for middleware. All middleware will need to be the middleware pkg, instead of individual pkgs. I think this is fine, since middleware fns will just delegate to exposed functions from other pkgs, like source.NewContext

wesleyk · 2021-06-03T03:55:22Z

src/query/util/queryhttp/queryhttp.go

 }

 // Register registers an endpoint.
 func (r *EndpointRegistry) Register(opts RegisterOptions) error {
 	route := r.router.NewRoute()
+	r.middlewareOpts[route] = opts.MiddlewareOverride


wesleyk

LGTM

nbroyles

Nice! LG

arnikola · 2021-06-03T15:01:56Z

src/query/api/v1/handler/prometheus/native/middleware.go

-	Clock          clockwork.Clock
+// WithQueryParams adds the query request parameters to the middleware options.
+var WithQueryParams middleware.OverrideOptions = func(opts middleware.Options) middleware.Options {
+	opts.Logging.Fields = func(r *http.Request, start time.Time) []zap.Field {


Is it always ok to blow away whatever logging options may already exist? Maybe we need to do something like capture the existing opts.Logging.Fields(), running that method, and then appending the new fields to those?

that's a good point.

ryanhall07 force-pushed the rhall-middleware-changes branch from d071610 to 821328a Compare June 2, 2021 14:29

ryanhall07 force-pushed the rhall-middleware-changes branch from 821328a to 62ffe2a Compare June 2, 2021 14:49

Merge branch 'master' into rhall-middleware-changes

70a6d3f

ryanhall07 changed the title ~~WIP~~ Refactor Middleware to a single set of functions for all routes Jun 3, 2021

ryanhall07 requested review from arnikola, robskillington, wesleyk and nbroyles June 3, 2021 02:55

ryanhall07 commented Jun 3, 2021

View reviewed changes

wesleyk reviewed Jun 3, 2021

View reviewed changes

wesleyk approved these changes Jun 3, 2021

View reviewed changes

nbroyles approved these changes Jun 3, 2021

View reviewed changes

ryanhall07 marked this pull request as ready for review June 3, 2021 14:21

Merge branch 'master' into rhall-middleware-changes

e108eec

ryanhall07 enabled auto-merge (squash) June 3, 2021 14:21

arnikola reviewed Jun 3, 2021

View reviewed changes

ryanhall07 added 2 commits June 3, 2021 10:29

append fields

fdb3721

Merge branch 'master' into rhall-middleware-changes

d8624a8

ryanhall07 disabled auto-merge June 3, 2021 17:29

ryanhall07 enabled auto-merge (squash) June 3, 2021 17:30

ryanhall07 merged commit a9dda99 into master Jun 3, 2021

ryanhall07 deleted the rhall-middleware-changes branch June 3, 2021 18:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Middleware to a single set of functions for all routes #3534

Refactor Middleware to a single set of functions for all routes #3534

ryanhall07 commented Jun 2, 2021 •

edited

Loading

codecov bot commented Jun 2, 2021 •

edited

Loading

ryanhall07 Jun 3, 2021

ryanhall07 Jun 3, 2021

ryanhall07 Jun 3, 2021

ryanhall07 Jun 3, 2021

ryanhall07 Jun 3, 2021

ryanhall07 Jun 3, 2021

ryanhall07 Jun 3, 2021

wesleyk Jun 3, 2021

wesleyk left a comment

nbroyles left a comment

arnikola Jun 3, 2021

ryanhall07 Jun 3, 2021

Refactor Middleware to a single set of functions for all routes #3534

Refactor Middleware to a single set of functions for all routes #3534

Conversation

ryanhall07 commented Jun 2, 2021 • edited Loading

codecov bot commented Jun 2, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesleyk left a comment

Choose a reason for hiding this comment

nbroyles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanhall07 commented Jun 2, 2021 •

edited

Loading

codecov bot commented Jun 2, 2021 •

edited

Loading