Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

add pyroscope #4871

Merged
merged 14 commits into from
Feb 22, 2022
2 changes: 1 addition & 1 deletion .gitlab-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ test-build-linux-stable:
script:
- ./scripts/gitlab/test_linux_stable.sh
# we're using the bin built here, instead of having a parallel `build-linux-release`
- time cargo build --profile testnet --verbose --bin polkadot
- time cargo build --profile testnet --features pyroscope --verbose --bin polkadot
drahnr marked this conversation as resolved.
Show resolved Hide resolved
drahnr marked this conversation as resolved.
Show resolved Hide resolved
- sccache -s
# pack artifacts
- mkdir -p ./artifacts
Expand Down
84 changes: 84 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ runtime-benchmarks= [ "polkadot-cli/runtime-benchmarks" ]
try-runtime = [ "polkadot-cli/try-runtime" ]
fast-runtime = [ "polkadot-cli/fast-runtime" ]
runtime-metrics = [ "polkadot-cli/runtime-metrics" ]
pyroscope = ["polkadot-cli/pyroscope"]

# Configuration for building a .deb package - for use with `cargo-deb`
[package.metadata.deb]
Expand Down
2 changes: 2 additions & 0 deletions cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ clap = { version = "3.0", features = ["derive"], optional = true }
log = "0.4.13"
thiserror = "1.0.30"
futures = "0.3.21"
pyro = { package = "pyroscope", version = "0.3.1", optional = true }

service = { package = "polkadot-service", path = "../node/service", default-features = false, optional = true }
polkadot-node-core-pvf = { path = "../node/core/pvf", optional = true }
Expand Down Expand Up @@ -57,6 +58,7 @@ trie-memory-tracker = ["sp-trie/memory-tracker"]
full-node = ["service/full-node"]
try-runtime = ["service/try-runtime"]
fast-runtime = ["service/fast-runtime"]
pyroscope = ["pyro"]

# Configure the native runtimes to use. Polkadot is enabled by default.
#
Expand Down
9 changes: 8 additions & 1 deletion cli/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,14 @@ pub struct RunCmd {
/// Must be valid socket address, of format `IP:Port`
/// commonly `127.0.0.1:6831`.
#[clap(long)]
pub jaeger_agent: Option<std::net::SocketAddr>,
pub jaeger_agent: Option<String>,

/// Add the destination address to the `pyroscope` agent.
///
/// Must be valid socket address, of format `IP:Port`
/// commonly `127.0.0.1:4040`.
#[clap(long)]
pub pyroscope_server: Option<String>,
}

#[allow(missing_docs)]
Expand Down
43 changes: 42 additions & 1 deletion cli/src/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use log::info;
use sc_cli::{Role, RuntimeVersion, SubstrateCli};
use service::{self, IdentifyVariant};
use sp_core::crypto::Ss58AddressFormatRegistry;
use std::net::ToSocketAddrs;

pub use crate::error::Error;
pub use polkadot_performance_test::PerfCheckError;
Expand Down Expand Up @@ -266,7 +267,17 @@ where
info!("----------------------------");
}

let jaeger_agent = cli.run.jaeger_agent;
let jaeger_agent = if let Some(ref jaeger_agent) = cli.run.jaeger_agent {
Some(
jaeger_agent
.to_socket_addrs()
.map_err(Error::AddressResolutionFailure)?
.next()
.ok_or_else(|| Error::AddressResolutionMissing)?,
)
} else {
None
};

runner.run_node_until_exit(move |config| async move {
let role = config.role.clone();
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

.to_socket_addrs()
.map_err(Error::AddressResolutionFailure)?
.next()
.ok_or_else(|| Error::AddressResolutionMissing)?;
// The pyroscope agent requires a `http://` prefix, so we just do that.
let mut agent = pyro::PyroscopeAgent::builder(
"http://".to_owned() + address.to_string().as_str(),
"polkadot".to_owned(),
)
.sample_rate(113)
.build()?;
agent.start();
Some(agent)
} else {
None
};

#[cfg(not(feature = "pyroscope"))]
if cli.run.pyroscope_server.is_some() {
return Err(Error::PyroscopeNotCompiledIn)
}

match &cli.subcommand {
None => run_node_inner(cli, service::RealOverseerGen, polkadot_node_metrics::logger_hook()),
Some(Subcommand::BuildSpec(cmd)) => {
Expand Down Expand Up @@ -509,5 +545,10 @@ pub fn run() -> Result<()> {
)
.into()),
}?;

#[cfg(feature = "pyroscope")]
if let Some(mut pyroscope_agent) = pyroscope_agent_maybe.take() {
pyroscope_agent.stop();
}
Ok(())
}
14 changes: 14 additions & 0 deletions cli/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,20 @@ pub enum Error {
#[error(transparent)]
PerfCheck(#[from] polkadot_performance_test::PerfCheckError),

#[cfg(not(feature = "pyroscope"))]
#[error("Binary was not compiled with `--feature=pyroscope`")]
PyroscopeNotCompiledIn,

#[cfg(feature = "pyroscope")]
#[error("Failed to connect to pyroscope agent")]
PyroscopeError(#[from] pyro::error::PyroscopeError),

#[error("Failed to resolve provided URL")]
AddressResolutionFailure(#[from] std::io::Error),

#[error("URL did not resolve to anything")]
AddressResolutionMissing,

#[error("Other: {0}")]
Other(String),
}