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

Support pprof profiling in BadWolf #155

Merged
merged 5 commits into from
Nov 4, 2020
Merged

Conversation

rogerlucena
Copy link
Contributor

@rogerlucena rogerlucena commented Oct 23, 2020

This PR comes to allow the user to have more visibility on latency and memory metrics when using BadWolf, adding support for pprof profiling through BadWolf's CLI.

When in the terminal, after already entering the BadWolf's CLI with the command bw bql, the user can now write:

start profiling;

To start pprof profiling. The user can also specify the CPU profiling rate with the -cpurate flag below:

start profiling -cpurate <samples_per_second>

Similarly, to stop it they can just write:

stop profiling;

With profiling enabled, the metrics for latency and memory consumption for the BQLs executed will be printed into two files, cpuprofile and memprofile, that can then be consumed and visualized through the go pprof CLI tool.

Commands such as go tool pprof -http=":8081" bw cpuprofile open in the browser an interface to visualize the CPU metrics: the user can see the graph of function calls made and the latencies involved, useful to identify bottlenecks; they can visualize the correspondent Flame Graph as well, and also an ordered list of the heaviest function calls made. Another useful command is go tool pprof -pdf bw cpuprofile > cpuprofile.pdf to generate pdfs from the profiling files. With memprofile the previous pprof commands work similarly.

@rogerlucena rogerlucena marked this pull request as draft October 23, 2020 00:40
@rogerlucena rogerlucena marked this pull request as ready for review October 29, 2020 14:54
@thiagovas thiagovas self-requested a review November 3, 2020 12:47
Copy link

@Tati1701 Tati1701 left a comment

Choose a reason for hiding this comment

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

A couple of small comments on indenting the error flow so it is more clear to the reader.

I'll go ahead and approve but PTAL at them before submitting.

tools/vcli/bw/repl/repl.go Outdated Show resolved Hide resolved
tools/vcli/bw/repl/repl.go Outdated Show resolved Hide resolved
tools/vcli/bw/repl/repl.go Show resolved Hide resolved
@@ -175,6 +220,59 @@ func REPL(od storage.Store, input *os.File, rl ReadLiner, chanSize, bulkSize, bu
done <- false
continue
}
if strings.HasPrefix(l, "start profiling") {
Copy link

Choose a reason for hiding this comment

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

For a separate PR/Unralated to your changes:
Consider updating the loop to have a switch instead of several if strings.HasPrefix checks.
Remember: in Go, switch statements are more general than in other languages, so you can have cases checking for HasPrefix.
https://golang.org/doc/effective_go.html#switch
Besides being more readable, it can probably simplify the done <- false/continue repetitions. You might want to look at labels to control the flow here, rather than using the channel :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you Tati! It is cool that switch can be this general in Go, we will keep this in mind for a future PR as well. =]

tools/vcli/bw/repl/repl.go Outdated Show resolved Hide resolved
tools/vcli/bw/repl/repl.go Outdated Show resolved Hide resolved
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.

4 participants