add pyroscope #4871
add pyroscope #4871
Conversation
drahnr
force-pushed
the
bernhard-pyroscope
branch
from
99ec3c5
to
bed7568
Compare
drahnr
added
B0-silent
|
I think to be complete, this PR should also have some docs on how to use Pyroscope with a local or remote server, similar to what we have for the Grafana readme. |
|
At a second thought, I think we don't need this kind of readme for pyroscope. We don't have this for jaeger/tempo either. grafana is a bit of special case since we have the dashboard exports in there and to make changes, using them is a prerequisite and the whole usage pattern is significantly more complex. |
drahnr
force-pushed
the
bernhard-pyroscope
branch
from
7314d62
to
1770541
Compare
Co-authored-by: Andronik <write@reusable.software>
Until the panics are fixed in upstream, this is sane.
drahnr
force-pushed
the
bernhard-pyroscope
branch
from
b10f902
to
57dbdc7
Compare
Fixes an underflow panic.
| @@ -293,6 +304,31 @@ where | |||
| pub fn run() -> Result<()> { | |||
| let cli: Cli = Cli::from_args(); | |||
|
|
|||
| #[cfg(feature = "pyroscope")] | |||
| let mut pyroscope_agent_maybe = if let Some(ref agent_addr) = cli.run.pyroscope_server { | |||
| let address = agent_addr | |||
There was a problem hiding this comment.
Do we need an address resolution here in the first place, counting that we reconstruct an URL afterwards? This URL can support more than plain http, for example https. I have no strong opinion here, but maybe it is better to name option like pyroscope_url and treat it as an URL from the beginning?
There was a problem hiding this comment.
Two competing things: We want compatibility, but we also want URLs. url::Url does not accept an IP without a prefixed protocol such as tcp:// or http:// - but we must maintain compatibility. The above is an escape hatch to reduce either to an IP address and avoid the ordeal.
This reverts commit 57dbdc7.
cli/src/command.rs
Outdated
| "http://".to_owned() + address.to_string().as_str(), | ||
| "polkadot".to_owned(), | ||
| ) | ||
| .sample_rate(100) |
There was a problem hiding this comment.
Made the sample rate a prime number above 100, to retain accuracy and not introduce too much overhead. This is just an initial number, we'll have to adjust this based on practical experience, hence I'd like to keep this on the lower, less invasive side for the time being.
There was a problem hiding this comment.
Yeah, I think it's a good idea to pick a "less round" sampling rate.
For not-perf there wasn't really any deep meaning behind why I picked 900, besides the fact that I wanted it high to make the results more detailed/reliable.
|
BTW, do we already have some example flamegraphs or any other data that this generates? I'm curious how it'll handle our codebase compared to my profiler, especially whether it'll be able to gather full stack traces and whether it can properly demangle everything and decode inline frames along with proper line numbers. |
(required vpn): https://github.com/paritytech/devops/issues/1221#issuecomment-1034150974 |
|
bot merge |
This is a test to see if this yields better results compared to just ebpf for the binary