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

proposal: net/http: log errors during serve() #56028

Open
geofffranks opened this issue Oct 4, 2022 · 3 comments
Open

proposal: net/http: log errors during serve() #56028

geofffranks opened this issue Oct 4, 2022 · 3 comments
Labels
Milestone

Comments

@geofffranks
Copy link

geofffranks commented Oct 4, 2022

If this should be categorized as something other than a proposal, I apologize, feel free to change as necessary. It didn't seem quite like a bug, and nothing else seemed like a good fit.

Current State

If http.Serve encounters an error while serving a request (but before sending it to any handlers via ServeHTTP), there are cases where HTTP responses are sent to clients, with no logging performed to inform the operator that a request even occurred. Notable cases include:

  • unsupported transfer encoding in the http request (not response)
  • unsupported/malformed HTTP protocol version
  • missing/malformed host header
  • invalid header name/value
  • malformed method/http request
  • HTTP Request Header's max length exceeded

http.Server has no hooks in place to allow for custom access log messages for these requests, but does have an ErrorLog attribute. However, it is not used to log any of these failures. It is used to log other per-request errors, such as the following:

  • invalid context lengths
  • superfluous/invalid attempts write calls for responses
  • TLS handshake failures
  • recovered panics encountered while serving a request
  • if enabled, warnings regarding query params being delimited with semicolons

Proposals

Quick and Easy

We add logf() calls to the serve() function that log the failures being encountered prior to sending responses.

Thorough and Flexible

  1. Add a new http.AccessLogger interface with one function: ServeHTTP(rw http.ResponseWriter, r *http.Request, next http.HandlerFunc)
  2. Provide a built-in AccessLog handler for people to use if they do not wish to implement their own.
  3. Add an AccessLog http.AccessLogger attribute to http.Server (defaulting to nil) that if present is called when writing any responses to the client. Currently consumers of http.Server need to provide access logging functionality on their own, but are unable to catch all responses sent. This would allow for a realitively easy drop-in + opt-in replacement for existing access log handlers.
@gopherbot gopherbot added this to the Proposal milestone Oct 4, 2022
@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Oct 4, 2022

CC @neild @bradfitz

@AlexanderYastrebov
Copy link
Contributor

AlexanderYastrebov commented Oct 12, 2022

If these are logged via ErrorLog some users may prefer to mute it, see #26918
Other users may prefer to collect metrics instead of logging so alternative option could be some kind of RequestErrorHandler hook.
Since internal errors are not exported this hook should receive status code and maybe a status text.

@neild
Copy link
Contributor

neild commented Oct 21, 2022

http2.Server has a CountErr hook which is called when various errors are encountered:

// CountError, if non-nil, is called on HTTP/2 server errors.
// It's intended to increment a metric for monitoring, such
// as an expvar or Prometheus metric.
// The errType consists of only ASCII word characters.
CountError func(errType string)

Would a CountError on http.Server work for this case? (Called on each of these error conditions.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

5 participants