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

Adds middleware to insert X-Request-Start unix millis timestamp #70

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

AxelTheGerman
Copy link
Contributor

@AxelTheGerman AxelTheGerman commented Nov 18, 2024

Implements #69

Note: I never wrote a line of Go before. I based everything on the X-Request-ID middleware implementation.

PS: make test passes for me locally

@AxelTheGerman
Copy link
Contributor Author

@kevinmcconnell if you have a moment, I would appreciate your take on this feature/PR. Thank you so much for bringing all this to life, I was looking for something like Kamal for years 🚀

Comment on lines 27 to 28
var now = time.Now().UnixMilli()
return strconv.FormatInt(now, 10)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either:

Suggested change
var now = time.Now().UnixMilli()
return strconv.FormatInt(now, 10)
now := time.Now().UnixMilli()
return strconv.FormatInt(now, 10)

Or:

Suggested change
var now = time.Now().UnixMilli()
return strconv.FormatInt(now, 10)
return strconv.FormatInt(time.Now().UnixMilli(), 10)

Comment on lines 13 to 14
id := r.Header.Get("X-Request-Start")
assert.NotEmpty(t, id)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
id := r.Header.Get("X-Request-Start")
assert.NotEmpty(t, id)
timestamp := r.Header.Get("X-Request-Start")
assert.NotEmpty(t, timestamp)

Comment on lines 26 to 27
id := r.Header.Get("X-Request-Start")
assert.Equal(t, "1234", id)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
id := r.Header.Get("X-Request-Start")
assert.Equal(t, "1234", id)
timestamp := r.Header.Get("X-Request-Start")
assert.Equal(t, "1234", timestamp)

Comment on lines 20 to 26
if r.Header.Get("X-Request-Start") == "" {
r.Header.Set("X-Request-Start", h.nowStr())
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add this at the top:

const (
	requestStartHeader = "X-Request-Start"
)
Suggested change
if r.Header.Get("X-Request-Start") == "" {
r.Header.Set("X-Request-Start", h.nowStr())
}
if r.Header.Get(requestStartHeader) == "" {
r.Header.Set(requestStartHeader, h.nowStr())
}

You can then also reuse this in tests.

h.next.ServeHTTP(w, r)
}

func (h *RequestStartMiddleware) nowStr() string {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either:

Suggested change
func (h *RequestStartMiddleware) nowStr() string {
func (h *RequestStartMiddleware) nowString() string {

Or:

Suggested change
func (h *RequestStartMiddleware) nowStr() string {
func (h *RequestStartMiddleware) timestamp() string {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also ditch this helper completely and use the following directly:

strconv.FormatInt(time.Now().UnixMilli(), 10)

@@ -120,6 +120,7 @@ func (s *Server) buildHandler() http.Handler {
var handler http.Handler

handler = s.router
handler = WithRequestStartMiddleware(handler)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably should add this before or after the WithRequestIDMiddleware() handler so that this is injected as early as possible, given that handlers are executed in the inverse order.

An option would be to augment the WithRequestIDMiddleware() handler with the new header it would inject.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, didn't realize they get executed in reverse. Wanted to add this one first. Thank you.

Copy link

@kwilczynski kwilczynski Nov 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries. @AxelTheGerman, you can see this with a simpler example:

  package main

  import (
          "fmt"
          "net/http"
  )

  func One(h http.Handler) http.Handler {
          return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
                  fmt.Println("One")
                  h.ServeHTTP(w, r)
                  fmt.Println("One")
          })
  }

  func Two(h http.Handler) http.Handler {
          return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
                  fmt.Println("Two")
                  h.ServeHTTP(w, r)
                  fmt.Println("Two")
          })
  }

  func Main(h http.Handler) http.Handler {
          return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
                  fmt.Println("Main")
          })
  }

  func main() {
          mux := http.NewServeMux()

          handler := Main(mux)
          handler = Two(handler)
          handler = One(handler)

          mux.Handle("/", handler)
          panic(http.ListenAndServe(":3000", mux))
  }

Then, sending a single request against TCP/3000, per:

$ go run main.go
^Z
[1]+  Stopped                 go run main.go
$ bg
[1]+ go run main.go &
$ curl -v 0:3000
*   Trying 0.0.0.0:3000...
* Connected to 0.0.0.0 (0.0.0.0) port 3000
* using HTTP/1.x
> GET / HTTP/1.1
> Host: 0.0.0.0:3000
> User-Agent: Mozilla/5.0 (compatible; MSIE 9.0; Windows NT 6.1; Trident/5.0)
> Accept: */*
>
* Request completely sent off
One
Two
Main
Two
One
< HTTP/1.1 200 OK
< Date: Sun, 24 Nov 2024 15:40:48 GMT
< Content-Length: 0
<
* Connection #0 to host 0.0.0.0 left intact
$ fg
go run main.go
^Csignal: interrupt

You chain the execution of each handler, which becomes a callback, so to speak, which runs the handler you passed into it, hence the inverse order in which these run. I hope this makes sense.

There are some nice packages such as the justinas/alice that aim to make this pattern a little bit more grounded.

@kwilczynski
Copy link

@AxelTheGerman, feel free to squash the commits into one.

@AxelTheGerman
Copy link
Contributor Author

@AxelTheGerman, feel free to squash the commits into one.

Done!

Thanks again for the review and additional information on how everything works.

@kwilczynski
Copy link

@AxelTheGerman, feel free to squash the commits into one.

Done!

Thanks again for the review and additional information on how everything works.

@AxelTheGerman, my pleasure. Thank you for your contribution! Much appreciated. 🎉

We now need to wait for one of the maintainers to do a final round of reviews, etc.

@kevinmcconnell
Copy link
Collaborator

kevinmcconnell commented Nov 25, 2024

This is great, thanks @AxelTheGerman for the contribution, and @kwilczynski for the review. 🙏

Nice work! 👏

@kevinmcconnell kevinmcconnell merged commit 9b1fb5f into basecamp:main Nov 25, 2024
1 check passed
@AxelTheGerman AxelTheGerman deleted the x-request-start branch November 25, 2024 15:37
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.

3 participants