Skip to content

feat: add debug-level request logging #923

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

Merged
merged 2 commits into from
Apr 8, 2022
Merged

feat: add debug-level request logging #923

merged 2 commits into from
Apr 8, 2022

Conversation

johnstcn
Copy link
Member

@johnstcn johnstcn commented Apr 8, 2022

Logs all requests at DEBUG level.
I found it useful, so I figured you all might also.

@johnstcn johnstcn self-assigned this Apr 8, 2022
@codecov
Copy link

codecov bot commented Apr 8, 2022

Codecov Report

Merging #923 (ad86f37) into main (38f0742) will decrease coverage by 9.67%.
The diff coverage is 100.00%.

❗ Current head ad86f37 differs from pull request most recent head d5425ca. Consider uploading reports for the commit d5425ca to get more accurate results

@@            Coverage Diff             @@
##             main     #923      +/-   ##
==========================================
- Coverage   65.78%   56.10%   -9.68%     
==========================================
  Files         226      227       +1     
  Lines       13991    13998       +7     
  Branches      105      105              
==========================================
- Hits         9204     7854    -1350     
- Misses       3848     5275    +1427     
+ Partials      939      869      -70     
Flag Coverage Δ
unittest-go-macos-latest 53.06% <100.00%> (+0.08%) ⬆️
unittest-go-postgres- ?
unittest-go-ubuntu-latest 55.34% <100.00%> (+0.09%) ⬆️
unittest-go-windows-2022 52.18% <100.00%> (+0.01%) ⬆️
unittest-js 58.25% <ø> (ø)
Impacted Files Coverage Δ
coderd/coderd.go 97.04% <100.00%> (+0.01%) ⬆️
coderd/httpmw/log.go 100.00% <100.00%> (ø)
coderd/database/queries.sql.go 0.00% <0.00%> (-83.76%) ⬇️
coderd/database/pubsub.go 0.00% <0.00%> (-77.78%) ⬇️
coderd/database/db.go 0.00% <0.00%> (-55.18%) ⬇️
coderd/database/migrate.go 0.00% <0.00%> (-45.00%) ⬇️
cli/config/file.go 69.44% <0.00%> (-8.34%) ⬇️
codersdk/provisionerdaemons.go 58.46% <0.00%> (-6.16%) ⬇️
coderd/workspaceresources.go 54.87% <0.00%> (-6.10%) ⬇️
coderd/coderdtest/coderdtest.go 92.59% <0.00%> (-6.07%) ⬇️
... and 9 more

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 38f0742...d5425ca. Read the comment docs.

func DebugLogRequest(log slog.Logger) func(http.Handler) http.Handler {
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
log.Debug(context.Background(), fmt.Sprintf("%s %s", r.Method, r.URL.Path))
Copy link
Member

Choose a reason for hiding this comment

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

I wish there was a level trace in slog! Seems reasonable regardless, because we'll want customers to have a way to enable this debuggability too.

I really like the simplicity of this.

"cdr.dev/slog"
)

func DebugLogRequest(log slog.Logger) func(http.Handler) http.Handler {
Copy link
Member

Choose a reason for hiding this comment

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

Could it be too jank to put this inline inside coderd.go? I ask because then we'd display code coverage on it (I know we technically already have it), and it'd just add ~4 lines into that file anyhow.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like that.

@johnstcn johnstcn enabled auto-merge (squash) April 8, 2022 14:35
@johnstcn johnstcn merged commit 94ab6f3 into main Apr 8, 2022
@johnstcn johnstcn deleted the cj/httpmw-log branch April 8, 2022 14:35
@f0ssel
Copy link
Contributor

f0ssel commented Apr 8, 2022

Usually http access logs are INFO level because you want them on in production monitoring but don't want all the spam from debug logs. Could we bump it up to INFO?

@johnstcn
Copy link
Member Author

johnstcn commented Apr 8, 2022

Usually http access logs are INFO level because you want them on in production monitoring but don't want all the spam from debug logs. Could we bump it up to INFO?

Logger level could be a configuration knob we could expose in api.Options, what do you think @f0ssel ?

@misskniss misskniss added this to the V2 Beta milestone May 15, 2022
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.

4 participants