Set glog flags from configuration #3062
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR fixes dgraph-io#2854 Why glog can't use values set in config file? glog reads values from flags. When a user sets the value (of a glog flag) in a config file, glog doesn't recognize it because the flag value hasn't changed. How does this PR fix it? With this PR, we set the value of glog flags from the values in config file. Note: This is a hack. glog doesn't allow setting configuration programatically and thus we have to use this hack. Signed-off-by: Ibrahim Jarif <[email protected]>
golangcibot
reviewed
Feb 25, 2019
jarifibrahim
commented
Feb 25, 2019
| @@ -31,7 +31,6 @@ import ( | |||
| "github.com/dgraph-io/dgraph/dgraph/cmd/zero" | |||
| "github.com/dgraph-io/dgraph/x" | |||
| "github.com/spf13/cobra" | |||
| flag "github.com/spf13/pflag" | |||
There was a problem hiding this comment.
Aliasing pflag => flag and flag => goflag created unnecessary ambiguity. I've changed this to remove the ambiguity.
Signed-off-by: Ibrahim Jarif <[email protected]>
Signed-off-by: Ibrahim Jarif <[email protected]>
golangcibot
reviewed
Feb 25, 2019
|
We might also want to evaluate https://github.com/Sirupsen/logrus. It would be much cleaner (and easier) to configure the logging with logrus compared to glog. Also, the logs from glog aren't parseable (you can parse them but searching for a specific key might be difficult). Logrus allows adding context information to logs https://github.com/sirupsen/logrus#default-fields which would be very useful when you have a lof of logs. |
manishrjain
approved these changes
Feb 25, 2019
There was a problem hiding this comment.
logrus gets really slow because it uses Go maps.
Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jarifibrahim)
jarifibrahim
added a commit
to jarifibrahim/dgraph
that referenced
this pull request
Feb 28, 2019
dgraph-io#3062 added capabilities to set flag values from config file (we need this for glog). This caused an issue with the log_backtrace_at flag. The flag is defined as `flag.Var(&logging.traceLocation, "log_backtrace_at", "when logging hits line file:N, emit a stack trace")` where tracelocation is ``` // traceLocation represents the setting of the -log_backtrace_at flag. type traceLocation struct { file string line int } ``` If the value isn't specified the nil value for tracelocation type is set in the config, which is tracelocation{"", 0}. But this value can't be set to the flag because of the check on line https://github.com/golang/glog/blob/master/glog.go#L374 This PR adds skips setting the flag to nil value if it nil in the config. Fixes dgraph-io#3076 Signed-off-by: Ibrahim Jarif <[email protected]>
jarifibrahim
added a commit
to jarifibrahim/dgraph
that referenced
this pull request
Feb 28, 2019
dgraph-io#3062 added capabilities to set flag values from config file (we need this for glog). This caused an issue with the log_backtrace_at flag. The flag is defined as `flag.Var(&logging.traceLocation, "log_backtrace_at", "when logging hits line file:N, emit a stack trace")` where tracelocation is ``` // traceLocation represents the setting of the -log_backtrace_at flag. type traceLocation struct { file string line int } ``` If the value isn't specified the nil value for `tracelocation` type is set in the config, which is `tracelocation{"", 0}`. But this value can't be set to the flag because of the check on line https://github.com/golang/glog/blob/master/glog.go#L374 This PR adds skips setting the flag to nil value if it nil in the config. Fixes dgraph-io#3076 Signed-off-by: Ibrahim Jarif <[email protected]>
jarifibrahim
added a commit
to jarifibrahim/dgraph
that referenced
this pull request
Feb 28, 2019
dgraph-io#3062 added capabilities to set flag values from config file (we need this for glog). This caused an issue with the log_backtrace_at flag. The flag is defined as `flag.Var(&logging.traceLocation, "log_backtrace_at", "when logging hits line file:N, emit a stack trace")` where tracelocation is ``` // traceLocation represents the setting of the -log_backtrace_at flag. type traceLocation struct { file string line int } ``` If the value isn't specified the nil value for `tracelocation` type is set in the config, which is `tracelocation{"", 0}`. But this value can't be set to the flag because of the check on line https://github.com/golang/glog/blob/master/glog.go#L374 This PR adds skips setting the flag to nil value if it nil in the config. Fixes dgraph-io#3076 Signed-off-by: Ibrahim Jarif <[email protected]>
manishrjain
pushed a commit
that referenced
this pull request
Mar 1, 2019
#3062 added capabilities to set flag values from config file (we need this for glog). This caused an issue with the log_backtrace_at flag. The flag is defined as `flag.Var(&logging.traceLocation, "log_backtrace_at", "when logging hits line file:N, emit a stack trace")` where tracelocation is ``` // traceLocation represents the setting of the -log_backtrace_at flag. type traceLocation struct { file string line int } ``` If the value isn't specified the nil value for `tracelocation` type is set in the config, which is `tracelocation{"", 0}`. But this value can't be set to the flag because of the check on line https://github.com/golang/glog/blob/master/glog.go#L374 This PR adds skips setting the flag to nil value if it nil in the config. Fixes #3076 Signed-off-by: Ibrahim Jarif <[email protected]>
dna2github
pushed a commit
to dna2fork/dgraph
that referenced
this pull request
Jul 19, 2019
* Set glog flags from configuration This PR fixes dgraph-io#2854 Why glog can't use values set in config file? glog reads values from flags. When a user sets the value (of a glog flag) in a config file, glog doesn't recognize it because the flag value hasn't changed. How does this PR fix it? With this PR, we set the value of glog flags from the values in config file. Note: This is a hack. glog doesn't allow setting configuration programatically and thus we have to use this hack. Signed-off-by: Ibrahim Jarif <[email protected]> * Fix error check Signed-off-by: Ibrahim Jarif <[email protected]> * Fix line length Signed-off-by: Ibrahim Jarif <[email protected]>
dna2github
pushed a commit
to dna2fork/dgraph
that referenced
this pull request
Jul 19, 2019
dgraph-io#3062 added capabilities to set flag values from config file (we need this for glog). This caused an issue with the log_backtrace_at flag. The flag is defined as `flag.Var(&logging.traceLocation, "log_backtrace_at", "when logging hits line file:N, emit a stack trace")` where tracelocation is ``` // traceLocation represents the setting of the -log_backtrace_at flag. type traceLocation struct { file string line int } ``` If the value isn't specified the nil value for `tracelocation` type is set in the config, which is `tracelocation{"", 0}`. But this value can't be set to the flag because of the check on line https://github.com/golang/glog/blob/master/glog.go#L374 This PR adds skips setting the flag to nil value if it nil in the config. Fixes dgraph-io#3076 Signed-off-by: Ibrahim Jarif <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes #2854
Why glog can't use values set in config file?
glog reads values from flags. When a user sets the value (of a glog flag) in a config file, glog doesn't recognize it because the flag value hasn't changed.
How does this PR fix it?
With this PR, we set the value of glog flags from the values in config file.
Note: This is a hack. glog doesn't allow setting configuration programatically and thus we have to use this hack.
Signed-off-by: Ibrahim Jarif [email protected]
This change is