Thanos: HTTP Middleware Interface in Thanos

[yashrsharma44]

Use case

Currently, Thanos uses only two middleware, instr the instrumentation middleware for tracking all the requests for metricizing the data. Similarily, we have tracing middleware, for tracing purposes. However, in future if we want to add in more middlewares in Thanos, it would make the codebase hard to read, and we would have different implementation of middlewares based on authors implementation.
A solution for this would be to have a middleware interface that any new middleware should implement, so that we could plug in our interface in the HTTP server options, without worrying about the different implementation.
cc @kakkoyun :nerd_face:

#

Here is my rough interface for implementing the HTTP middleware -

HTTP Middleware in Thanos

• The HTTP Middleware Interface
  We can reuse http.Handler interface for implementing our middleware. We can create a new struct which would contain the interface http.Handler and implement the ServeHTTP which would make it quite simple for abstraction purposes.

• Here is pseudo HTTP middleware implementation

type RequestLoggerMiddleware struct {
    http.Handler
    logger log.Logger
}

func(m *RequestLoggerMiddleware) ServeHTTP(w http.ResponseWriter, r *http.Request){
    startTime := time.Now()

        // For demonstration, we add in the request id and log the start of handling of request
    log.Info(m.logger).Log(fmt.Sprintf("request handling started %s %s", time,req))
    reqID := getNewRequestID(r)
        r = r.WithContext(context.WithValue(r.Context(), "request-id",reqID))

    l.handler.ServeHTTP(w,r.WithContext(ctx))

    // Post call after handling the request
        log.Info(m.logger).Log(fmt.Sprintf("finished call %s", time))
}

func NewRequestLoggerMiddleware(handler http.Handler, logger *log.Logger) *RequestLoggerMiddleware {
    return &RequestLoggerMiddleware{
        http.Handler : handler,
        logger : logger,
    }
}

• Chaining of middlewares

  For chaining of middlewares, we can use the adapter pattern -

  type Middleware func(http.Handler) http.Handler
  

  func FooMiddleware() Middleware {
  return func(h http.Handler) http.Handler {
    return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
      h.ServeHTTP(w, r)
    }
  }
  }
  

  func ChainMiddleware(h http.Handler, adapters ...Middleware) http.Handler {
  n := len(adapters)
  for i := n - 1; i >= 0; i-- {
      h = adapters[i](h)
  } 
  
  return h
  }
  

  http.Handle("/", ChainMiddleware(indexHandler, ins.NewHandler(name),
                                     tracing(name,tracer, logger),
                                     RequestLogger(name, logger),
                                   )
  

[kakkoyun]

Thanks for starting this. I like the chaining the middleware idea and the pattern.

However, I think we can simplify things a lot here. We can just re-use http.Handler instead of HTTPMiddleware. Actions in PreCall and PostCall can easily be handled in a ServeHTTP And we can easily rename Adapter type to Middleware. So things would be a lot simpler. What do you think?

[yashrsharma44]

However, I think we can simplify things a lot here. We can just re-use http.Handler instead of HTTPMiddleware. Actions in PreCall and PostCall can easily be handled in a ServeHTTP

Yeah, the Precall and Postcall can be simply implemented in the ServeHTTP for simplifying things.

And we can easily rename Adapter type to Middleware. So things would be a lot simpler.

Yeah, renamed to Middleware.

@kakkoyun Shall I go about implementing the same, having agreed with the implementation details? :stuck_out_tongue:

[kakkoyun]

👍 Go ahead. If we discovered any other issues we can discuss it in the PR.

[stale[bot]]

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[yashrsharma44]

I will have a stab at this issue, this week 😛

[yashrsharma44]

@kakkoyun Is this issue relevant in the current context? I can have a look :stuck_out_tongue:

[kakkoyun]

I guess we can still refine our API configuration by using this pattern. Nothing urgent or vital, it would just increase readability if you're still interested go ahead.