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

pkg/logutil: add json log format #626

Merged
merged 3 commits into from
Apr 25, 2017

Conversation

andelf
Copy link
Contributor

@andelf andelf commented Apr 24, 2017

  • log.format: text, json, console
  • log rotate options
  • log entry timestamp options

@andelf andelf changed the title pkg/logutil: add multiple log format pkg/logutil: add json log format Apr 24, 2017
@@ -20,8 +20,21 @@ tso-save-interval = "3s"
[log]
level = "info"

#[log.file]
# log format, one of json, text, console
#format = "text"
Copy link
Contributor

Choose a reason for hiding this comment

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

em, I think most of the time, users don't use console even they run PD in the console. TEXT and JSON may be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

colorful output. can be remove from comment, leave the function in code.

// Log format. one of json, text, or console.
Format string `toml:"format" json:"format"`
// Disable automatic timestamps in output.
DisableTimestamp bool `toml:"disable-timestamp" json:"disable-timestamp"`
Copy link
Contributor

Choose a reason for hiding this comment

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

any benefit to disabling timestamp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ELK system will add a timestamp and it has more accuracy.
Enduser can also translate this timestamp in time field via logstash.

LocalTime: true,
}

if _, err := output.Write([]byte{}); err != nil {
fmt.Fprintf(os.Stderr, "failed to write to log, %v\n", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

seem return error is enough.

@tennix
Copy link
Contributor

tennix commented Apr 25, 2017

Could we put this util to a standalone repo that all our Go projects can use it.
The util library defines basic log fields: timestamp, src-file, line-number, log-level, timestamp, msg. Users can add more fields if they want. With more fields, searching tools like kibana can be more efficient to query logs

Copy link
Contributor

@disksing disksing left a comment

Choose a reason for hiding this comment

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

LGTM.

@disksing
Copy link
Contributor

@tennix I guess it's ok to copy and paste util.go to other projects, it's very simple and other repos may want different default values or config layouts.

@nolouch
Copy link
Contributor

nolouch commented Apr 25, 2017

LGTM

@siddontang
Copy link
Contributor

coverage decreased, please add test.

@andelf
Copy link
Contributor Author

andelf commented Apr 25, 2017

@siddontang
maybe you should say coverage increased
😆
ref https://coveralls.io/builds/11225280

@siddontang siddontang merged commit fe564d4 into tikv:master Apr 25, 2017
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants