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

Add profiling capabilities to consumer and producer #109

Merged
merged 1 commit into from
Jul 20, 2017

Conversation

bzz
Copy link
Contributor

@bzz bzz commented Jul 12, 2017

  • producer: add --cpuprofile and --memprofile CLI flags to save profiles to FS
  • consumer: add --profiler flag to expose profiler data over HTTP

Update

  • investigate migration to https://github.com/pkg/profile (suitable only for processes with short lifetime, to write profiles to a file on exit)
  • change borges producer to HTTP as it's a long-running service exposing --profiler and --profiler-port

@bzz
Copy link
Contributor Author

bzz commented Jul 12, 2017

\cc @smola @ajnavarro for review

@codecov
Copy link

codecov bot commented Jul 12, 2017

Codecov Report

Merging #109 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #109   +/-   ##
=======================================
  Coverage   66.26%   66.26%           
=======================================
  Files          11       11           
  Lines         821      821           
=======================================
  Hits          544      544           
  Misses        213      213           
  Partials       64       64

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e66d1bc...e4bddba. Read the comment docs.

Copy link
Contributor

@smola smola left a comment

Choose a reason for hiding this comment

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

Awesome!

  • I would use github.com/pkg/profile, which has a simpler API and deals with some of the tricky details
  • Maybe we should add a configurable profiler (with environment variables) in src-d/core-retrieval, which we can add to a single line to any command (e.g. defer core. Profiler().Start().Stop())
  • It's a bit unexpected that in one command there is a flag that enables HTTP profiler with a hardcoded address and port, and the other command has 2 flags to enable memory and cpu profilers dumping to a file. It would be nice to be able to access both in any case.
  • Add access to the block profile too!

@bzz
Copy link
Contributor Author

bzz commented Jul 13, 2017

I would use github.com/pkg/profile, which has a simpler API and deals with some of the tricky details

Did not know about this one 😳 thank for pointing out, will look into that and switch.

Maybe we should add a configurable profiler (with environment variables) in src-d/core-retrieval

this sounds good, but please keep in mind that its just an initial work. I would add such improvement to backlog and deal in separate PR after we done some profiling sessions and understand better what we actually need.

It's a bit unexpected that in one command there is a flag that enables HTTP profiler with a hardcoded address and port, and the other command has 2 flags to enable memory and cpu profilers dumping to a file. It would be nice to be able to access both in any case.

I agree, and ofcourse it's not set in stone, but it has a rationale - profiler API for "saving to files" works very well in case of short-live processes, which os.Exit. HTTP API was designed for long-running servers and by default works with samples in 30sec interval, but is available all the time.

So it was uses the same way here, and in Berserkers (CLI vs Server) and Bblfsh server (Client vs Server).

Right now, I would keep it like that as it's already useful (low hanging fruit), not to spend to much time at this first step, but it can be further improved in subsequent PRs of course.

Let me know what you think!

@smola
Copy link
Contributor

smola commented Jul 13, 2017

@bzz Note that borges producer in production is a service that runs permanently getting mentions from rovers, and accessing though HTTP is easier when deployed in Kubernetes.

@smola smola requested a review from ajnavarro July 13, 2017 08:42
Copy link
Contributor

@ajnavarro ajnavarro left a comment

Choose a reason for hiding this comment

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

Can we implement it using http as santiago said?

@bzz
Copy link
Contributor Author

bzz commented Jul 13, 2017

Sure, I will take care of it tomorrow, scope updated in PR description.

@bzz bzz force-pushed the add-profiler branch 2 times, most recently from 37cce01 to 1ad9f52 Compare July 19, 2017 09:40
@bzz
Copy link
Contributor Author

bzz commented Jul 19, 2017

History re-written, now both consumer and producer expose CPU, memory and block profilers over HTTP

@bzz bzz merged commit 933a58d into src-d:master Jul 20, 2017
@bzz bzz deleted the add-profiler branch July 20, 2017 00:17
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