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 flag to log requests at info level #236

Conversation

colin-stuart
Copy link
Contributor

Adds an optional flag to log requests at the info level, still defaulting to logging requests at the debug level.

Copy link
Contributor

@cinaglia cinaglia left a comment

Choose a reason for hiding this comment

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

Wanted to leave some preliminary feedback until maintainers chime in.

server/server.go Outdated
@@ -79,6 +79,7 @@ type Config struct {
HTTPMiddleware []middleware.Interface `yaml:"-"`
Router *mux.Router `yaml:"-"`
DoNotAddDefaultHTTPMiddleware bool `yaml:"-"`
LogRequestsAtInfoLevel bool `yaml:"-"`
Copy link
Contributor

Choose a reason for hiding this comment

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

My hunch is that it would preferable to place this under LogSourceIPsRegex alongside the the log-related flags.

Also, we should:

  • Add a yaml tag so it can be configured via file
  • Update (cfg *Config).RegisterFlags to accept a server.log-requests-at-info-level flag.

middleware/logging.go Outdated Show resolved Hide resolved
@bboreham
Copy link
Collaborator

bboreham commented Feb 9, 2022

Thanks for the PR.

It’s now calling logWithRequest about six times; can you factor that out?

@colin-stuart colin-stuart force-pushed the add-flag-to-log-requests-at-info-level branch from 987bba4 to 2a9f7dd Compare February 9, 2022 17:33
@colin-stuart
Copy link
Contributor Author

Thanks for the the review @bboreham, let me know what you think about the refactor. Another option would be to add another global convenience function that takes a level string and a calls the appropriate Debugf()/Infof()/etc. function.

@bboreham
Copy link
Collaborator

What I meant was: call logWithRequest once, then hold the resulting logging.Interface in a variable for the duration of the Wrap() inner function.
It's a relatively expensive operation:

func (l Log) logWithRequest(r *http.Request) logging.Interface {
localLog := l.Log
traceID, ok := tracing.ExtractTraceID(r.Context())
if ok {
localLog = localLog.WithField("traceID", traceID)
}
if l.SourceIPs != nil {
ips := l.SourceIPs.Get(r)
if ips != "" {
localLog = localLog.WithField("sourceIPs", ips)
}
}
return user.LogWith(r.Context(), localLog)
}

@colin-stuart
Copy link
Contributor Author

Thanks @bboreham, I see what you meant now!

@bboreham
Copy link
Collaborator

bboreham commented Mar 2, 2022

Looks better now, thanks. Have you tried it?

@colin-stuart
Copy link
Contributor Author

Yep, enabled it for a service running locally and the requests logs are showing at the info level.

@bboreham bboreham merged commit 00e2e23 into weaveworks:master Mar 2, 2022
@colin-stuart colin-stuart deleted the add-flag-to-log-requests-at-info-level branch March 2, 2022 16:09
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.

3 participants