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

Switch logging to use structured logging everywhere #7671

Merged
merged 1 commit into from
Dec 14, 2016

Conversation

jsternberg
Copy link
Contributor

The logging library has been switched to use uber-go/zap. While the
logging has been changed to use structured logging, this commit does not
change any of the logging statements to take advantage of the new
structured log or new log levels. Those changes will come in future
commits.

Fixes #7036.

@jsternberg jsternberg force-pushed the js-7036-structured-logging branch 6 times, most recently from 336e3ab to 844e612 Compare December 6, 2016 21:25
@joelegasse joelegasse self-requested a review December 13, 2016 17:07
Copy link
Contributor

@joelegasse joelegasse left a comment

Choose a reason for hiding this comment

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

This is a step in the right direction. Without updating the log messages, I would go through this and remove the non-service context values in the filestore, cacheloader, and shard services. They duplicate information and really stretch out the log lines (especially with the file paths)

func (s *Shard) SetLogOutput(w io.Writer) {
s.logger.SetOutput(w)
func (s *Shard) WithLogger(log zap.Logger) {
s.baseLogger = log.With(
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like only half of the change to switch to a full structured log. Without changing the current message being printed later on, it results in long messages with duplicated information:

[I] 2016-12-14T16:25:12Z /home/jlegasse/.influxdb/data/_internal/monitor/9 database index loaded in 8.860841ms shard=9 path=/home/jlegasse/.influxdb/data/_internal/monitor/9 service=shard

I think for this change, the extra context should be left out. We can revisit later and switch from formatted strings to zap context values all at once, then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this, but kept the scaffolding code so it's easy to readd.

The logging library has been switched to use uber-go/zap. While the
logging has been changed to use structured logging, this commit does not
change any of the logging statements to take advantage of the new
structured log or new log levels. Those changes will come in future
commits.
@jsternberg jsternberg merged commit 76df362 into master Dec 14, 2016
@jsternberg jsternberg deleted the js-7036-structured-logging branch December 14, 2016 17:59
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.

None yet

2 participants