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

Suppress logging before flag.Parse from glog #2970

Merged
merged 4 commits into from
Feb 4, 2019

Conversation

jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Feb 2, 2019

Replace goflag.Parse() call with goflag.CommandLine.Parse([]string{})
This serves multiple purposes

  1. We don't actually parse the os.Args (which Parse() method internally does). This ensures that the command line arguments are parsed only by RootCmd.Execute() call (which is the parent command)
  2. We suppress the Error: Logging before flag.Parse... warning messages. https://github.com/golang/glog/blob/master/glog.go#L679 causes the warning message and we suppress this warning by calling the Parse method on the underlying flagset.

Signed-off-by: Ibrahim Jarif [email protected]

Before this PR

➜  dgraph git:(master) ./dgraph -h                                        
Usage of ./dgraph:
  -alsologtostderr
    	log to standard error as well as files
  -log_backtrace_at value
    	when logging hits line file:N, emit a stack trace
  -log_dir string
    	If non-empty, write log files in this directory
  -logtostderr
    	log to standard error instead of files
  -stderrthreshold value
    	logs at or above this threshold go to stderr
  -v value
    	log level for V logs
  -vmodule value
    	comma-separated list of pattern=N settings for file-filtered logging

with this PR

➜  dgraph git:(goflag-parse) ✗ ./dgraph -h

Dgraph is a horizontally scalable and distributed graph database,
providing ACID transactions, consistent replication and linearizable reads.
...
cluster.

Dgraph version   : v1.0.12-rc3-58-g3401ace6
...
Go version       : go1.11.4

....
Usage:
  dgraph [command]

Available Commands:
  acl         Run the Dgraph acl tool
  ...
  zero        Run Dgraph Zero

Flags:
      --alsologtostderr                  log to standard error as well as files
      --bindall                          Use 0.0.0.0 instead of localhost to bind to all addresses on local machine. (default true)
      --block_rate int                   Block profiling rate. Must be used along with block profile_mode
      --config string                    Configuration file. Takes precedence over default values, but is overridden to values set with environment variables and flags.
      --enterprise_features              Enable Dgraph enterprise features. If you set this to true, you agree to the Dgraph Community License.
      --expose_trace                     Allow trace endpoint to be accessible from remote
  -h, --help                             help for dgraph
      --log_backtrace_at traceLocation   when logging hits line file:N, emit a stack trace (default :0)
      --log_dir string                   If non-empty, write log files in this directory
      --logtostderr                      log to standard error instead of files
      --profile_mode string              Enable profiling mode, one of [cpu, mem, mutex, block]
  -v, --v Level                          log level for V logs
      --vmodule moduleSpec               comma-separated list of pattern=N settings for file-filtered logging

Use "dgraph [command] --help" for more information about a command.
➜  dgraph git:(goflag-parse) ✗ 

Related to #2890


This change is Reviewable

Replace `goflag.Parse()` call with `goflag.CommandLine.Parse([]string{})`
This serves multiple purposes
1. We don't actually parse the os.Args (which Parse() method internally does). This ensures that the command line arguments are parsed only by RootCmd.Execute() call (which is the parent command)
2. We suppress the `Error: Logging before flag.Parse...` warning messages. https://github.com/golang/glog/blob/master/glog.go#L679 causes the warning message and we suppress this warning by calling the Parse method on the underlying flagset.

Signed-off-by: Ibrahim Jarif <[email protected]>
@CLAassistant
Copy link

CLAassistant commented Feb 2, 2019

CLA assistant check
All committers have signed the CLA.

dgraph/cmd/root.go Outdated Show resolved Hide resolved
@jarifibrahim
Copy link
Contributor Author

Can someone please restart the CI (dgraph) build? The previous build failed due to timeout.

@codexnull codexnull self-assigned this Feb 4, 2019
Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @codexnull, @golangcibot, and @jarifibrahim)


dgraph/cmd/root.go, line 60 at r2 (raw file):

	initCmds()

	// Convinces goflags that we have called Parse() to avoid noisy logs.

rewrite the comment in the passive voice.

"Convinces goflags that Parse() has been called ..."


dgraph/cmd/root.go, line 62 at r2 (raw file):

	// Convinces goflags that we have called Parse() to avoid noisy logs.
	// https://github.com/kubernetes/kubernetes/issues/17162#issuecomment-225596212
	// We don't need to check the error here.

Check the error, even if it's pointless, to clear the lint warning.

dgraph/cmd/root.go Outdated Show resolved Hide resolved
@codexnull codexnull removed their assignment Feb 4, 2019
Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

Good to merge, once Martin approves this. Thanks for the change, @jarifibrahim .

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @golangcibot and @jarifibrahim)

Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @golangcibot and @jarifibrahim)

@martinmr martinmr merged commit 78aa45d into dgraph-io:master Feb 4, 2019
@jarifibrahim jarifibrahim deleted the goflag-parse branch February 10, 2019 12:14
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
Replace `goflag.Parse()` call with `goflag.CommandLine.Parse([]string{})`
This serves multiple purposes
1. We don't actually parse the os.Args (which Parse() method internally does). This ensures that the command line arguments are parsed only by RootCmd.Execute() call (which is the parent command)
2. We suppress the `Error: Logging before flag.Parse...` warning messages. https://github.com/golang/glog/blob/master/glog.go#L679 causes the warning message and we suppress this warning by calling the Parse method on the underlying flagset.

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
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants