-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
tools/benchmark: add flags for pprof to storage put #4201
Conversation
@xiang90 sorry for that, I changed the place of the vars. PTAL. If a child command has at least one PersistentFlag, ancestors' PersistentFlag aren't inherited. The PR spf13/cobra#220 is adding a new type of flag that is inherited to children unconditionally. |
4078689
to
581c8c3
Compare
@@ -46,6 +50,11 @@ func init() { | |||
storagePutCmd.Flags().IntVar(&storageKeySize, "key-size", 64, "a size of key (Byte)") | |||
storagePutCmd.Flags().IntVar(&valueSize, "value-size", 64, "a size of value (Byte)") | |||
storagePutCmd.Flags().BoolVar(&txn, "txn", false, "put a key in transaction or not") | |||
|
|||
// TODO: after the PR https://github.com/spf13/cobra/pull/220 is merged, the below pprof related flags should be moved to RootCmd | |||
storagePutCmd.Flags().StringVar(&cpuProfPath, "cpuprof-path", "", "a path of file for storing cpu profile result") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we change the flag to cpuprofile
and memprofile
. go blog post suggests the flag in that way (http://blog.golang.org/profiling-go-programs)
OK. LGTM. |
581c8c3
to
690d86d
Compare
@xiang90 updated the names of flags, PTAL. |
@@ -46,6 +47,11 @@ func init() { | |||
storagePutCmd.Flags().IntVar(&storageKeySize, "key-size", 64, "a size of key (Byte)") | |||
storagePutCmd.Flags().IntVar(&valueSize, "value-size", 64, "a size of value (Byte)") | |||
storagePutCmd.Flags().BoolVar(&txn, "txn", false, "put a key in transaction or not") | |||
|
|||
// TODO: after the PR https://github.com/spf13/cobra/pull/220 is merged, the below pprof related flags should be moved to RootCmd | |||
storagePutCmd.Flags().StringVar(&cpuProfPath, "cpuprofile", "", "a path of file for storing cpu profile result") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the path of file for storing cpu profile resut
@mitake Can you address last few nits? |
This commit adds flags for profiling with runtime/pprof to storage put: - --cpuprofile: specify a path of CPU profiling result, if it is not empty, profiling is activated - --memprofile: specify a path of heap profiling result, if it is not empty, profiling is activated Of course, the flags should be added to RootCmd ideally. However, adding common flags that shared by children command requires the ongoing PR: spf13/cobra#220 . Therefore this commit adds the flags to storage put only.
690d86d
to
1c802e9
Compare
@xiang90 solved the nits, could you take a look? |
tools/benchmark: add flags for pprof to storage put
Thanks! |
This commit adds flags for profiling with runtime/pprof to storage
put:
empty, profiling is activated
empty, profiling is activated
Of course, the flags should be added to RootCmd ideally. However,
adding common flags that shared by children command requires the
ongoing PR: spf13/cobra#220 . Therefore this
commit adds the flags to storage put only.