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 support for top level middleware and setting headers #1584

Closed
wants to merge 1 commit into from

Conversation

hooksie1
Copy link

We are embarking on some new internal tooling and would like to use this micro package since it simplifies a lot of work. One thing we are missing is top level middleware we can use. We could technically do this without middleware in the service, but we would have to wrap every function in it separately.

This also allows for setting a header value so that in the middleware a header value can be set on the request and then passed through.

Hopefully this doesn't stray too far from the idea of this package.

@yordis
Copy link
Contributor

yordis commented Mar 14, 2024

I would rather see people creating their Middleware abstraction that works however they prefer, keeping a simple client to deal with and promoting experimenting!

handler := func(req micro.Request) {
  log.Println(req.Headers().Get("request-id"))
  req.Respond(req.Data())
}

pipeline := NewPipeline(
  log,
  authorize,
  handler
)

config := micro.Config{
		Name:    "MiddlewareExample",
		Version: "0.1.0",
		Endpoint: &micro.EndpointConfig{
			Subject: "middleware",
			Handler: pipeline,
		}
	}

@hooksie1
Copy link
Author

Can you share a little more of what you mean? From what I can tell, it's not currently possible to set headers in the current micro package so how would your example work?

@ripienaar
Copy link
Contributor

ripienaar commented Mar 14, 2024

What we support today wrt middleware is on par with http handler right? That doesnt seem too onerous, we've often thought of adding higher order abstraction similar to gRPC where they do have this kind of middleware setup, but for the lower level library I suspect http like chained middleware is the right way to go that keeps as much options open as possible

As for headers, I am not sure why we dont support them today but if I had to think about it I'd say it is important that your payload contains all thats needed. Down the line we would like to re-use these headers for other purposes where messages may come from other mediums like jetstream or elsewhere it's important that in that scenario payloads be the entire request. That said I am not against supporting headers at a low level given that they are the canonical transport for otel etc

@hooksie1
Copy link
Author

You're right that you can currently wrap anything with middleware, this is a convenience for certain functions that would be run on every request. Like a logger for example, if you have multiple groups and some singular endpoints then you have to be cognizant of always wrapping them individually. For example group a might have 3 wrappers, group b might have 2 wrappers, and then single endpoints don't have any. Your logger would have to wrap each of those combinations. If you modify the layout later or add an endpoint, it feels like it would be easy to forget to ensure this is set up.

This is similar to Chi being able to do this at the initial router level, not just a subrouter (group).

r := chi.NewRouter()
	r.Use(middleware.Logger)
	r.Get("/", func(w http.ResponseWriter, r *http.Request) {
		w.Write([]byte("welcome"))
	})

@hooksie1
Copy link
Author

As for the headers, yeah it's more for things like otel. Maybe Add() instead of Set() makes more sense to ensure headers aren't ever overwritten? Headers felt like a safer way to inject secondary information since we don't have access to the raw message. The context handler exists but I hate adding values to contexts because that can be a giant footgun.

@hooksie1
Copy link
Author

I think maybe the underlying difference is that an http.ServeMux also satisfies the http.Handler interface but a micro.Service doesn't satisfy a micro.Handler. I don't think it necessarily should but to get the same level of wrapping without something like this PR it would have to.

@hooksie1
Copy link
Author

Selfishly for our immediate use case, the headers can be ignored and I can use subject names for things like request-ids. This was mainly around something like a general logging middleware for every handler. I creeped the scope a bit with the header stuff.

@ripienaar
Copy link
Contributor

ripienaar commented Mar 15, 2024

Thanks @hooksie1 after some internal discussions we feel its important to be able to keep parity in design on the core micro package level - and later do language idiomatic designs at a higher level for things like Middleware. We also have a general effort underway to add middleware to the clients so we think adding this now would be bad timing for us. It might be we end up in exactly this design, but we need to consider our many languages etc

For headers we'd like to add that, but we have to start with the ADR so all our language maintainers have a chance to review etc. Would you be open to sending a PR to https://github.com/nats-io/nats-architecture-and-design/blob/main/adr/ADR-32.md proposing the header design there?

@hooksie1
Copy link
Author

Yeah makes total sense. I appreciate how intentional you all are with design.

I'll get a PR in for that ADR. I should have started there but truthfully didn't realize that this same design was even outside of Go. I looked at Python and didn't see it and just assumed.

Thanks!

hooksie1 added a commit to hooksie1/nats-architecture-and-design that referenced this pull request Mar 17, 2024
Referring to this PR nats-io/nats.go#1584
this includes an add() method to add a header key and value. This
would facilitate extra context since the service doesn't have
direct access to the request.
@hooksie1
Copy link
Author

Ok PR for the ADR was opened here nats-io/nats-architecture-and-design#274

I'll close this. Thanks @ripienaar

@hooksie1 hooksie1 closed this Mar 17, 2024
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

3 participants