feat: support JSON structured log formatting #179
Conversation
pehlicd
changed the title
There was a problem hiding this comment.
Thanks for the contribution (and the typo fixes)!
In the past, I've been reluctant to make go-httpbin too opinionated about logging, given the wide variety of possible use cases. Instead, we added a generic mechanism for pluggable observers to enable users to add whatever instrumentation they needed (see #11, #74, etc).
However, now that slog is in the stdlib of the two most recent go versions it is now safe for us to adopt, and JSON structured logging seems "standardized enough" to warrant easy support here, so I think a change like this makes sense.
Left a couple of small comments below, plus I think there are some test failures to address, but LGTM at a high level!
httpbin/middleware.go
Outdated
| var logLevel = slog.LevelInfo | ||
|
|
||
| return func(result Result) { | ||
| l.Printf( | ||
| logFmt, | ||
| time.Now().Format(dateFmt), | ||
| result.Status, | ||
| result.Method, | ||
| result.URI, | ||
| result.Size, | ||
| result.Duration.Seconds()*1e3, // https://github.com/golang/go/issues/5491#issuecomment-66079585 | ||
| result.UserAgent, | ||
| result.ClientIP, | ||
| if result.Status >= 500 { | ||
| logLevel = slog.LevelError | ||
| } else if result.Status >= 400 && result.Status < 500 { | ||
| logLevel = slog.LevelWarn | ||
| } |
There was a problem hiding this comment.
This isn't safe, we're modifying a logLevel var defined outside this function on every invocation. I think that means a single 5xx response followed by a 2xx response will be logged at error level.
I'd probably do something like this instead:
logLevel := slog.InfoLevel
if result.Status >= 500 {
logLevel = slog.LevelError
} else if result.Status >= 400 {
logLevel = slog.LevelWarn
}Yep, confirmed locally with a /status/500 then /status/200 request, where the latter is logged at ERROR level:
$ make run
mkdir -p dist
CGO_ENABLED=0 go build -ldflags="-s -w" -o dist/go-httpbin ./cmd/go-httpbin
HOST=127.0.0.1 PORT=8080 dist/go-httpbin
time=2024-08-28T14:18:06.611-04:00 level=INFO msg="go-httpbin listening on http://127.0.0.1:8080"
time=2024-08-28T14:18:14.150-04:00 level=ERROR msg="500 GET /status/500 0.0ms" status=500 method=GET uri=/status/500 size_bytes=0 duration_ms=0.01675 user_agent=curl/8.7.1 client_ip=127.0.0.1:60335
time=2024-08-28T14:18:24.516-04:00 level=ERROR msg="200 GET /status/200 0.1ms" status=200 method=GET uri=/status/200 size_bytes=0 duration_ms=0.08220799999999999 user_agent=curl/8.7.1 client_ip=127.0.0.1:60338
There was a problem hiding this comment.
Oh yeah true, sorry about that, will fix it in my next commit.
No need to apologize 🙂. It's completely understandable that maintaining high-quality code requires thorough review. |
mccutchen
changed the title
This pr aims to give option to select one of the log formats text or json. It also introduces new flag
-log-formatto achieve.I modified the tests so it should work out of the box. Also fixed some typos that I saw in general.
Please let me know if you also want me to modify README.
Closes #178