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

Add requestid to context #91

Merged
merged 1 commit into from
Jan 18, 2024
Merged

Add requestid to context #91

merged 1 commit into from
Jan 18, 2024

Conversation

rafiramadhana
Copy link
Collaborator

This PR is a proof of concept. So, it is very open for discussions.

Resolves #22

Add requestid to context, so we will have unique identifier for each request.

As starter, we are implementing it in KeyHandler.Create

Add requestid in KeyHandlerCreate with chi middleware.
@@ -49,6 +51,7 @@ func main() {
uploadMiddleware := middlewares.NewUploadMiddleware(appLogger)

r := chi.NewRouter()
r.Use(chiMiddleware.RequestID)
Copy link
Collaborator Author

@rafiramadhana rafiramadhana Jan 17, 2024

Choose a reason for hiding this comment

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

Here, we are adding the request ID into incoming context (r.Context()).

The process is abstracted by chi's chiMiddleware.RequestID.

@@ -45,14 +46,15 @@ func generateRandomString(length int) string {
}

func (h *KeyHandler) Create(w http.ResponseWriter, r *http.Request) {
log := h.logger.With("requestid", chiMiddleware.GetReqID(r.Context()))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here, we are getting the context with chiMiddleware.GetReqID.

Again, the process is abstracted by chi.

Copy link
Owner

@joseph0x45 joseph0x45 left a comment

Choose a reason for hiding this comment

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

Nice, this was simpler than I thought it would be

@joseph0x45
Copy link
Owner

Also one thing that I meant by

This middleware should only log requests either coming from the Frontend or authenticated requests going to the API.

is that, when I originally hosted the first version of Visio on a linode instance, I used Chi's logger middleware but my logs were bloated because of bot requests coming to the server. So I think passing an id down to the middlewares part can be handled by Chi, but we need another middleware that could tell if a request is coming either from our frontend or from a user, a very simple way I think we can solve this is by having a special value that would be passed in a special header in all requests coming from the frontend and as for the requests going to the API itself we could just check for the existence of the x-api-key header and reject the request if it is not set. Do you think it's a good approach?

@rafiramadhana
Copy link
Collaborator Author

Also one thing that I meant by

This middleware should only log requests either coming from the Frontend or authenticated requests going to the API.

is that, when I originally hosted the first version of Visio on a linode instance, I used Chi's logger middleware but my logs were bloated because of bot requests coming to the server. So I think passing an id down to the middlewares part can be handled by Chi, but we need another middleware that could tell if a request is coming either from our frontend or from a user, a very simple way I think we can solve this is by having a special value that would be passed in a special header in all requests coming from the frontend and as for the requests going to the API itself we could just check for the existence of the x-api-key header and reject the request if it is not set. Do you think it's a good approach?

i think that's a good idea

in addition, if the bots exhaust your server (i.e. our server processes bots' requests and returns HTTP 200 OK) we can also do rate limitting

@joseph0x45
Copy link
Owner

yep, rate limiting is definitely something I want to implement :)

@joseph0x45 joseph0x45 merged commit 4617669 into main Jan 18, 2024
2 checks passed
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.

feat: Requests logger middleware
2 participants