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

Update logging formatting/encoder #547

Merged
merged 2 commits into from
Oct 28, 2021
Merged

Update logging formatting/encoder #547

merged 2 commits into from
Oct 28, 2021

Conversation

mxyng
Copy link
Collaborator

@mxyng mxyng commented Oct 28, 2021

  • Use zap logging for (almost) everything
    • Only thing that isn't using zap.logger is login statements since it won't look as good
  • Use combination of gorilla/handlers CustomLoggingHandler and zap.Logger for HTTP request logging
    • We lose request duration but should realistically be part of metrics, not logs
  • Use different logging configuration for terminal and non-terminal

Console example. Note DEBUG is actually in green:

$ infra logout -l debug
2021-10-28T12:55:48.116-0700    DEBUG   logging out localhost
2021-10-28T12:55:48.273-0700    DEBUG   cleaning up kubeconfig
2021-10-28T12:55:48.279-0700    DEBUG   cleaning up cache

Kubernetes logs example.

$ stern engine
+ infra-engine-75c5c6657f-2c4db › engine
infra-engine-75c5c6657f-2c4db engine {"level":"info","ts":1635451021.1426108,"msg":"serving on port 443"}
infra-engine-75c5c6657f-2c4db engine {"level":"debug","ts":1635451021.1448,"msg":"endpoint is: localhost:8443"}
infra-engine-75c5c6657f-2c4db engine {"level":"debug","ts":1635451021.1448908,"msg":"not testing DNS lookup for localhost"}
infra-engine-75c5c6657f-2c4db engine {"level":"debug","ts":1635451021.1609979,"msg":"handled request","method":"GET","path":"/healthz","status":200,"size":2}
infra-engine-75c5c6657f-2c4db engine {"level":"debug","ts":1635451021.4673965,"msg":"handled request","method":"GET","path":"/healthz","status":200,"size":2}
infra-engine-75c5c6657f-2c4db engine {"level":"warn","ts":1635451023.4528298,"msg":"could not verify Infra TLS certificates: x509: certificate signed by unknown authority"}
infra-engine-75c5c6657f-2c4db engine {"level":"debug","ts":1635451023.5346036,"msg":"deduplicating users"}
infra-engine-75c5c6657f-2c4db engine {"level":"debug","ts":1635451023.5346286,"msg":"syncing local roles from infra configuration"}
infra-engine-75c5c6657f-2c4db engine {"level":"debug","ts":1635451024.442757,"msg":"handled request","method":"GET","path":"/healthz","status":200,"size":2}

Closes #539

}

func ZapLogFormatter(_ io.Writer, params handlers.LogFormatterParams) {
L.Debug("handled request",
Copy link
Collaborator

Choose a reason for hiding this comment

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

good change moving this to debug

LevelKey: "level",
EncodeLevel: zapcore.CapitalColorLevelEncoder,

TimeKey: "time",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the time necessary for terminal errors?

Copy link
Collaborator Author

@mxyng mxyng Oct 28, 2021

Choose a reason for hiding this comment

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

It's probably not but I find the log line without timestamp looks awkward:

$ infra version
WARN    Infra CLI (v0.3.6-next) is out of date. Please update to 0.3.6.

vs.

$ infra version
2021-10-28T13:58:08.750-0700    WARN    Infra CLI (v0.3.6-next) is out of date. Please update to 0.3.6.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another thing is while we don't have long lived commands in Infra CLI right now, it may be the case in the future where timestamps will be useful, e.g. running admin API commands.

@mxyng
Copy link
Collaborator Author

mxyng commented Oct 28, 2021

Follow up in #549

@mxyng mxyng merged commit 4660aba into main Oct 28, 2021
@mxyng mxyng deleted the cli-logging branch October 28, 2021 21:06
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.

Use internal/logging for everything
2 participants