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

Introduce logging level configuration #514

Merged
merged 5 commits into from
Nov 14, 2017

Conversation

pavolloffay
Copy link
Member

Logging struct was defined in flag package but not used. I have also removed unused code from there.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4a64414 on pavolloffay:logging-flag into 677251a on jaegertracing:master.

@@ -126,6 +134,7 @@ func main() {
case <-signalsChannel:
logger.Info("Jaeger Collector is finishing")
}
select {}
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed?

@@ -33,7 +33,7 @@ const (
// ESStorageType is the storage type flag denoting an ElasticSearch backing store
ESStorageType = "elasticsearch"
spanStorageType = "span-storage.type"
logLevel = "log-level"
logLevel = "log-Level"
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't it be lower case?

@@ -118,6 +126,7 @@ func main() {
case <-serverChannel:
logger.Info("Jaeger Query is finishing")
}
select {}
Copy link
Member

Choose a reason for hiding this comment

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

?

return flags
}

// AddFlags adds flags for SharedFlags
func AddFlags(flagSet *flag.FlagSet) {
flagSet.String(spanStorageType, CassandraStorageType, fmt.Sprintf("The type of span storage backend to use, options are currently [%v,%v,%v]", CassandraStorageType, ESStorageType, MemoryStorageType))
flagSet.String(logLevel, "info", "Minimal allowed log level")
flagSet.String(logLevel, "info", "Minimal allowed log Level. For more levels see zap logger documentation")
Copy link
Member

Choose a reason for hiding this comment

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

instead of zap logger documentation I'd print the url

Copy link
Member Author

Choose a reason for hiding this comment

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

added but it looks a bit weird.

Message for conf file:

Configuration file in JSON, TOML, YAML, HCL, or Java properties formats (default none). See spf13/viper for precedence.

}

// NewLogger pareses log Level
func (flags *SharedFlags) NewLogger(conf zap.Config, options ...zap.Option) (*zap.Logger, error) {
Copy link
Member

Choose a reason for hiding this comment

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

it looks like in all cases you're passing zap.Production() as the config. Is there a use case you're thinking of when it's not going to be production? Also, do we need a way to affect that config from cmd line / our config?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a use case you're thinking of when it's not going to be production? Also, do we need a way to affect that config from cmd line / our config?

Not right now. Maybe a debug flag or something like that in the future. I made this as flexible as possible.

So do you want to change it to the following?

func (flags *SharedFlags) NewLogger() (*zap.Logger, error) 

return nil
}

// NewLogger pareses log Level
Copy link
Member

Choose a reason for hiding this comment

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

typo "pareses"


func (co cassandraOptions) ServerList() []string {
return strings.Split(co.Servers, ",")
}
Copy link
Member

Choose a reason for hiding this comment

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

nice catch

@@ -132,7 +141,8 @@ func main() {
)

if error := command.Execute(); error != nil {
logger.Fatal(error.Error())
fmt.Println(error.Error())
Copy link
Member

Choose a reason for hiding this comment

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

this is unfortunate, because a logger may print the error quite differently, e.g. with a stack trace, write to file, report to Sentry, etc. If we guaranteed that the command only returned errors when it can't init the logger, then it wouldn't be too bad, but I don't think this is generally the case.

Copy link
Member Author

Choose a reason for hiding this comment

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

This statement is invoked when:

  • an error is propagated from runE -> in all cases except logger creation and config file parsing zap.Fatal is called which directly calls os.exit.
  • cobra fails to parse flags e.g. a wrong/undefined flag

Other solution might be to use a default logger here.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling e7488af on pavolloffay:logging-flag into 677251a on jaegertracing:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling e960dc4 on pavolloffay:logging-flag into 2b73fe9 on jaegertracing:master.

if file := v.GetString(configFile); file != "" {
v.SetConfigFile(file)
err := v.ReadInConfig()
if err != nil {
logger.Fatal("Error loading config file", zap.Error(err), zap.String(configFile, file))
errors.Wrap(err, fmt.Sprintf("Error loading config file %s", file))
Copy link
Member

Choose a reason for hiding this comment

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

missing return? And you can use Wrapf for extra formatting.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 ouch, the change from log Fatal :)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 1c96e3f on pavolloffay:logging-flag into 2b73fe9 on jaegertracing:master.

@coveralls
Copy link

coveralls commented Nov 9, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 3e0b69e on pavolloffay:logging-flag into a2ed9b8 on jaegertracing:master.

@pavolloffay
Copy link
Member Author

@yurishkuro could you please re-review?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 100.0% when pulling e2295a9 on pavolloffay:logging-flag into 32e2cd3 on jaegertracing:master.

@yurishkuro
Copy link
Member

@pavolloffay could you please rebase from master to make sure all is clean? Then ok to merge.

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@pavolloffay
Copy link
Member Author

@yurishkuro thanks, rebased, I will merge on green

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling aa7915b on pavolloffay:logging-flag into e0a74f2 on jaegertracing:master.

@pavolloffay pavolloffay merged commit 10d660f into jaegertracing:master Nov 14, 2017
@ghost ghost removed the review label Nov 14, 2017
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