server/requestlog: Add Request Context in requestlog.Entry

[mickeyreiss]

I would like to have access to the request's Context in the request logger, so that I can include metadata and also reference logging implementation details that might be stashed away in the context.

This is particularly useful for request loggers that aim to perform tracing with frameworks other than opencensus. It's also helpful for integrating with certain logging frameworks. I've also noticed that it is a good way to pass information from the http.Servers (e.g. BaseContext) down to the request logger.

To solve this, I've added a new field to Entry that is set based on the context from the request. I believe this change is relatively simple and backwards compatible.

[codecov]

Codecov Report

Merging #2983 (857fc15) into master (6f954e2) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2983      +/-   ##
==========================================
+ Coverage   69.60%   69.66%   +0.06%     
==========================================
  Files         111      111              
  Lines       11556    11560       +4     
==========================================
+ Hits         8043     8053      +10     
+ Misses       2851     2844       -7     
- Partials      662      663       +1     

Impacted Files	Coverage Δ
server/requestlog/requestlog.go	73.33% <100.00%> (+1.50%)	⬆️
pubsub/rabbitpubsub/rabbit.go	78.94% <0.00%> (-0.76%)	⬇️
server/health/sqlhealth/sqlhealth.go	78.37% <0.00%> (+21.62%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f954e2...857fc15. Read the comment docs.

[mickeyreiss]

I am not familiar with the test frameworks in this library. If this is PR is looking good, I would be happy to add a line of code somewhere to maintain the test coverage.

[vangent]

@zombiezen thoughts?

Add a context.Context to a struct is generally discouraged:
https://blog.golang.org/context-and-structs

[mickeyreiss]

Add a context.Context to a struct is generally discouraged:
https://blog.golang.org/context-and-structs

Fair enough, I generally agree. I did it this way based on the Exception to the rule: preserving backwards compatibility recommendation in that blog post. Is there different design that will work with the Logger interface?

[jba]

I think it's okay here, but how about a change that's both more general and less controversial: adding the Request to the Entry? After all, it is a request logger.

[mickeyreiss]

I think it's okay here, but how about a change that's both more general and less controversial: adding the Request to the Entry? After all, it is a request logger.

@jba That would solve everything I am looking for. Would you recommend deprecating the redundant fields (Referer, UserAgent, etc.)? Do you have something in mind for handling the Body, which is in an undefined state at this point (depending on whether or not it's been read)?

[jba]

You can document the redundant fields as deprecated. Also document that the implementor of Logger shouldn't touch the body.

This is all subject to @vangent's OK, BTW.

[zombiezen]

It's been a while, but IIRC the original reason we did not take in a Context in the Logger interface was to push for implementations to not do something long-running because Log does block request completion currently. It also permits the requestlog.Handler implementation to log asynchronously in the future.

I'll throw in another couple of implementation ideas here:

1. Rather than having a field that stores a Context, have the field type be interface { Value(interface{}) interface{} } so as to disentangle the Done channel. This does unfortunately prevent future attempts at logging asynchronously and is a little awkward to use with APIs that access a Context (but can be worked around with struct embedding).
2. Add a method to requestlog.Handler like SetContextTransform(f func(context.Context) interface{}) that invokes a callback to snapshot the data from the Context and then adds it to a new interface{} field in Entry. This would fit within the existing design goals while not running afoul of the Context-within-struct guidelines.

As an aside, migrating to OpenTelemetry would probably be a good idea to avoid users needing to deal with this in the common case. I believe OpenTelemetry is gaining more traction than OpenCensus did. I see this is already tracked as #2877.

[vangent]

I like @jba's suggestion; it's the most flexible. We can document the Request field with the caveats (don't touch Body, don't modify anything, and note that Log is synchronous so don't do anything long-running).

[mickeyreiss]

Thanks for the feedback, folks. Please take another look, as I've switched from Context to Request, added unit tests and deprecated the redundant fields with comments.