Skip to content

Conversation

@omarabid
Copy link
Contributor

@omarabid omarabid commented Mar 23, 2022

Backends

CLI

The current implementation is "functional", though the merging of configuration is a bit hacky and all over the place now. Configuration is passed down in this order: Defaults -> Env -> Config file -> CLI arguments.

Supported commands: exec / connect / completion
Supported arguments: application-name, log-level, no-logging, rbspy-blocking, pyspy-blocking, pyspy-idle, pyspy-gil, pyspy-native, sample-rate, server-address, tag

If you want to test the CLI, first cd into the pyroscope_cli directory and run

cargo build --release

You can then find the program inside target/release.

CI

Build and Tests are done against two OSes and two CPU architectures: Ubuntu/macOS and x86_64/ARM64. As the packages are separated, each one is built and tested separately. (lib, cli, pprofrs, rbspy, pyspy)

https://github.com/pyroscope-io/pyroscope-rs/actions/runs/2028544090

Remaining Tasks

  • Documentation and README files, particularly for the CLI and separate backends
  • Trying to generate a standalone binary via Github actions for 4 targets (linux/macos - x86_64/ARM64)
  • Better UX for the CLI output
  • Automate package publishing for the CLI and backends similarly the library

omarabid added 13 commits March 21, 2022 12:40
* try --tests --lib flag

* try args

* try default profile

* test building examples

* add target/toolchain

* add libunwind install

* wip(action): add build matrix for cli/backends

* typo

* typo

* use args instead of cwd

* chore(deps): force Cargo.lock for cli

* chore(actions): change matrix order

* chore(actions): add CPU targets

* chore(actions): add target to job name

* chore(actions): add packages to tests

* typo

* add toolchain input
@omarabid omarabid self-assigned this Mar 23, 2022
@omarabid
Copy link
Contributor Author

@drahnr can't seem to add you as reviewer, so pinging you here.

@@ -0,0 +1,5 @@
/target/
Cargo.lock
Copy link
Contributor

Choose a reason for hiding this comment

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

Cargo.lock should become part of the git repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@drahnr Can you explain the reason for that? My understanding is that Cargo.lock should be included only for the binary. (https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries)

sample_rate: u32,
/// Lock Process while sampling
lock_process: py_spy::config::LockingStrategy,
/// Profiling duration. None for infinite.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Some lines have trailing .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@drahnr Would you say an enum (with two variants: Infinity and Duration(core::time::Duration)) is more descriptive?

@@ -0,0 +1,5 @@
/target/
Cargo.lock
Copy link
Contributor

Choose a reason for hiding this comment

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

CC

Copy link
Collaborator

@petethepig petethepig left a comment

Choose a reason for hiding this comment

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

Agreed with @drahnr's comments

I tested it on mac m1, works great!


I have a couple of general comments, not sure if it should go to this release or not (it's up to you @omarabid):

  1. I think we should allow for multiple arguments in exec mode, e.g:
pyroscope-cli exec --spy-name pyspy python3 example.py

Currently it only works like this (and example.py has to be a :

pyroscope-cli exec --spy-name pyspy example.py

^ I think this is common in ruby / python ecosystem (e.g a lot of times these commands are quite complex, for example bundle exec rails server or wsgicli run main.py and so it'd make sense to accept multiple args.

  1. in rbspy case if I add sleep(0.1) to my program, rust implementation shows sleeps in the flamegraph, while our current pyroscope exec implementation does not. That probably has something to do with on_cpu detection. I suspect this is intentional as we're waiting for rbspy team to accept contributions or something like that (@omarabid, correct me if I'm wrong). Here's a script I'm using to test:
#!/usr/bin/env ruby

def work(n)
  i = 0
  while i < n
    i += 1
  end
end

def fast_function
    work(20000)
end

def slow_function
    work(80000)
end

while true
  fast_function
  slow_function
  sleep(0.1)
end

Here's what I'm seeing:
Screen Shot 2022-03-27 at 3 32 37 PM

Here's what I'm expecting:
Screen Shot 2022-03-27 at 3 34 16 PM

@omarabid
Copy link
Contributor Author

@petethepig


  1. Added arguments support in 5f2511a. Arguments for the command could be passed with an additional --.
  2. That's correct. However, I think both outputs are correct, and "on-cpu" should be passed through the CLI as an option too. I'll give the current PR (Add ability to collect traces only when the CPU is active (--on-cpu) rbspy/rbspy#327) a try and report back the results.

@omarabid omarabid merged commit 2691120 into main Mar 29, 2022
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