address review comments

Ariel Ben-Yehuda · Ariel Ben-Yehuda · commit c2ef4264d619 · 2025-09-25T19:37:30.000Z
diff --git a/README.md b/README.md
@@ -76,6 +76,35 @@ The metadata is not used by the agent directly, and only provided to the reporte
 [Fargate]: https://aws.amazon.com/fargate
 [IMDS]: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-metadata.html
 
+### What information the profiler gathers
+
+Memory samples (JFR `profiler.Malloc`) sample allocated memory every
+so many bytes of allocated memory, and are matched by `profiler.Free`
+to allow detecting if that memory is not free'd.
+
+CPU-time samples (JFR `jdk.ExecutionSample`) sample only threads that
+are currently running on a CPU, not threads that are sleeping.
+
+Wall-clock samples (JFR `profiler.WallClockSample`) sample threads
+whether they are sleeping or running, and can therefore be
+very useful for finding threads that are blocked, for example
+on a synchronous lock or a slow system call.
+
+When using Tokio, since tasks are not threads, tasks that are not
+currently running will not be sampled by a wall clock sample. However,
+a wall clock sample is still very useful in Tokio, since it is what
+you want to catch tasks that are blocking a thread by waiting on
+synchronous operations.
+
+The default is to do a wall-clock sample every second, and a CPU-time
+sample every 100 CPU milliseconds. This can be configured via
+[`ProfilerOptionsBuilder`].
+
+Memory samples are not enabled by default, but can be enabled by [`with_native_mem_bytes`].
+
+[`ProfilerOptionsBuilder`]: https://docs.rs/async-profiler-agent/0.1/async_profiler_agent/profiler/struct.ProfilerOptionsBuilder.html
+[`with_native_mem_bytes`]: https://docs.rs/async-profiler-agent/0.1/async_profiler_agent/profiler/struct.ProfilerOptionsBuilder.html#method.with_native_mem_bytes
+
 ### PollCatch
 
 If you want to find long poll times, and you have `RUSTFLAGS="--cfg tokio_unstable"`, you can
diff --git a/examples/simple/main.rs b/examples/simple/main.rs
@@ -119,7 +119,7 @@ async fn main_internal(args: Args) -> Result<(), anyhow::Error> {
     let profiler = match (&args.local, args.s3_bucket_args()) {
         (Some(local), S3BucketArgs { .. }) => profiler
             .with_reporter(LocalReporter::new(local))
-            .with_custom_agent_metadata(AgentMetadata::Other),
+            .with_custom_agent_metadata(AgentMetadata::NoMetadata),
         #[cfg(feature = "s3-no-defaults")]
         (
             _,
diff --git a/src/metadata/mod.rs b/src/metadata/mod.rs
@@ -6,7 +6,7 @@
 pub use std::time::Duration;
 
 /// Host Metadata, which describes a host that runs a profiling agent. The current set of supported agent metadata is
-/// AWS-specific. If you are not running on AWS, you can use [AgentMetadata::Other].
+/// AWS-specific. If you are not running on AWS, you can use [AgentMetadata::NoMetadata].
 #[derive(Debug, Clone, PartialEq, Eq)]
 #[non_exhaustive]
 pub enum AgentMetadata {
@@ -43,7 +43,11 @@ pub enum AgentMetadata {
         ecs_cluster_arn: String,
     },
     /// Metadata for a host that is neither an EC2 nor a Fargate
+    #[deprecated = "Use AgentMetadata::NoMetadata"]
     Other,
+    /// A placeholder when a host has no metadata, or when a reporter does not
+    /// use metadata.
+    NoMetadata,
 }
 
 /// Metadata associated with a specific individual profiling report
@@ -66,7 +70,7 @@ pub mod aws;
 /// [private] dummy metadata to make testing easier
 #[cfg(test)]
 pub(crate) const DUMMY_METADATA: ReportMetadata<'static> = ReportMetadata {
-    instance: &AgentMetadata::Other,
+    instance: &AgentMetadata::NoMetadata,
     start: Duration::from_secs(1),
     end: Duration::from_secs(2),
     reporting_interval: Duration::from_secs(1),
diff --git a/src/profiler.rs b/src/profiler.rs
@@ -207,7 +207,7 @@ impl ProfilerOptionsBuilder {
         self
     }
 
-    /// Sets the interva in which the profiler will collect
+    /// Sets the interval in which the profiler will collect
     /// CPU-time samples, via the [async-profiler `interval` option].
     ///
     /// CPU-time samples (JFR `jdk.ExecutionSample`) sample only threads that
@@ -261,7 +261,13 @@ impl ProfilerOptionsBuilder {
     /// Wall-clock samples (JFR `profiler.WallClockSample`) sample threads
     /// whether they are sleeping or running, and can therefore be
     /// very useful for finding threads that are blocked, for example
-    /// on a synchronous lock.
+    /// on a synchronous lock or a slow system call.
+    ///
+    /// When using Tokio, since tasks are not threads, tasks that are not
+    /// currently running will not be sampled by a wall clock sample. However,
+    /// a wall clock sample is still very useful in Tokio, since it is what
+    /// you want to catch tasks that are blocking a thread by waiting on
+    /// synchronous operations.
     ///
     /// The default is to do a wall-clock sample every second.
     ///
@@ -370,81 +376,7 @@ impl ProfilerBuilder {
         doc = "[`S3Reporter`]: crate::reporter::s3::S3Reporter"
     )]
     ///
-    #[cfg_attr(feature = "s3-no-defaults", doc = "## Example")]
-    #[cfg_attr(feature = "s3-no-defaults", doc = "")]
-    #[cfg_attr(feature = "s3-no-defaults", doc = r#"```no_run"#)]
-    #[cfg_attr(feature = "s3-no-defaults", doc = r#"# use std::path::PathBuf;"#)]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"# use async_profiler_agent::profiler::{ProfilerBuilder, SpawnError};"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"# use async_profiler_agent::reporter::s3::{S3Reporter, S3ReporterConfig};"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"# use async_profiler_agent::metadata::AgentMetadata;"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"# use aws_config::BehaviorVersion;"#
-    )]
-    #[cfg_attr(feature = "s3-no-defaults", doc = r#"# #[tokio::main]"#)]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"# async fn main() -> Result<(), SpawnError> {"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"# let path = PathBuf::from(".");"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"let bucket_owner = "<your account id>";"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"let bucket_name = "<your bucket name>";"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"let profiling_group = "a-name-to-give-the-uploaded-data";"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"let sdk_config = aws_config::defaults(BehaviorVersion::latest()).load().await;"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"let agent = ProfilerBuilder::default()"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"    .with_reporter(S3Reporter::new(S3ReporterConfig {"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"        sdk_config: &sdk_config,"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"        bucket_owner: bucket_owner.into(),"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"        bucket_name: bucket_name.into(),"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"        profiling_group_name: profiling_group.into(),"#
-    )]
-    #[cfg_attr(feature = "s3-no-defaults", doc = r#"    }))"#)]
-    #[cfg_attr(feature = "s3-no-defaults", doc = r#"    .build()"#)]
-    #[cfg_attr(feature = "s3-no-defaults", doc = r#"    .spawn()?;"#)]
-    #[cfg_attr(feature = "s3-no-defaults", doc = r#"# Ok::<_, SpawnError>(())"#)]
-    #[cfg_attr(feature = "s3-no-defaults", doc = r#"# }"#)]
-    #[cfg_attr(feature = "s3-no-defaults", doc = r#"```"#)]
+    #[cfg_attr(feature = "s3-no-defaults", doc = include_str!("s3-example.md"))]
     pub fn with_reporter(mut self, r: impl Reporter + Send + Sync + 'static) -> ProfilerBuilder {
         self.reporter = Some(Box::new(r));
         self
@@ -482,7 +414,7 @@ impl ProfilerBuilder {
     /// ```
     pub fn with_local_reporter(mut self, path: impl Into<PathBuf>) -> ProfilerBuilder {
         self.reporter = Some(Box::new(LocalReporter::new(path.into())));
-        self.with_custom_agent_metadata(AgentMetadata::Other)
+        self.with_custom_agent_metadata(AgentMetadata::NoMetadata)
     }
 
     /// Provide custom agent metadata.
@@ -502,94 +434,7 @@ impl ProfilerBuilder {
     /// you will need to create and attach your own metadata
     /// by calling this function.
     ///
-    #[cfg_attr(feature = "s3-no-defaults", doc = "## Example")]
-    #[cfg_attr(feature = "s3-no-defaults", doc = "")]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = "This will create a profiler that will upload to S3 with"
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = "empty ([AgentMetadata::Other]) metadata."
-    )]
-    #[cfg_attr(feature = "s3-no-defaults", doc = r#""#)]
-    #[cfg_attr(feature = "s3-no-defaults", doc = r#"```no_run"#)]
-    #[cfg_attr(feature = "s3-no-defaults", doc = r#"# use std::path::PathBuf;"#)]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"# use async_profiler_agent::profiler::{ProfilerBuilder, SpawnError};"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"# use async_profiler_agent::reporter::s3::{S3Reporter, S3ReporterConfig};"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"# use async_profiler_agent::metadata::AgentMetadata;"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"# use aws_config::BehaviorVersion;"#
-    )]
-    #[cfg_attr(feature = "s3-no-defaults", doc = r#"# #[tokio::main]"#)]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"# async fn main() -> Result<(), SpawnError> {"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"# let path = PathBuf::from(".");"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"let bucket_owner = "<your account id>";"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"let bucket_name = "<your bucket name>";"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"let profiling_group = "a-name-to-give-the-uploaded-data";"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"let sdk_config = aws_config::defaults(BehaviorVersion::latest()).load().await;"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"let agent = ProfilerBuilder::default()"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"    .with_reporter(S3Reporter::new(S3ReporterConfig {"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"        sdk_config: &sdk_config,"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"        bucket_owner: bucket_owner.into(),"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"        bucket_name: bucket_name.into(),"#
-    )]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"        profiling_group_name: profiling_group.into(),"#
-    )]
-    #[cfg_attr(feature = "s3-no-defaults", doc = r#"    }))"#)]
-    #[cfg_attr(
-        feature = "s3-no-defaults",
-        doc = r#"    .with_custom_agent_metadata(AgentMetadata::Other)"#
-    )]
-    #[cfg_attr(feature = "s3-no-defaults", doc = r#"    .build()"#)]
-    #[cfg_attr(feature = "s3-no-defaults", doc = r#"    .spawn()?;"#)]
-    #[cfg_attr(feature = "s3-no-defaults", doc = r#"# Ok::<_, SpawnError>(())"#)]
-    #[cfg_attr(feature = "s3-no-defaults", doc = r#"# }"#)]
-    #[cfg_attr(feature = "s3-no-defaults", doc = r#"```"#)]
+    #[cfg_attr(feature = "s3-no-defaults", doc = include_str!("s3-example-custom-metadata.md"))]
     /// [`reporter::s3::MetadataJson`]: crate::reporter::s3::MetadataJson
     /// [Amazon EC2]: https://aws.amazon.com/ec2
     /// [Amazon Fargate]: https://aws.amazon.com/fargate
@@ -871,6 +716,33 @@ impl RunningProfilerThread {
 
 /// Rust profiler based on [async-profiler].
 ///
+/// Spawning a profiler can be done either in an attached (controllable)
+/// mode, which allows for stopping the profiler (and, in fact, stops
+/// it when the relevant handle is dropped), or in detached mode,
+/// in which the profiler keeps running forever. Applications that can
+/// shut down the profiler at run-time, for example applications that
+/// support reconfiguration of a running profiler, generally want to use
+/// controllable mode. Other applications (most of them) should use
+/// detached mode.
+///
+/// In addition, the profiler can either be spawned into the current Tokio
+/// runtime, or into a new one. Normally, applications should spawn
+/// the profiler into their own Tokio runtime, but applications that
+/// don't have a default Tokio runtime should spawn it into a
+/// different one
+///
+/// This leaves 4 functions:
+/// 1. [Self::spawn] - detached, same runtime
+/// 2. [Self::spawn_thread_to_runtime] - detached, different runtime
+/// 3. [Self::spawn_controllable] - controllable, same runtime
+/// 4. [Self::spawn_controllable_thread_to_runtime] - controllable, different runtime
+///
+/// In addition, there's a helper function that just spawns the profiler
+/// to a new runtime in a new thread, for applications that don't have
+/// a Tokio runtime and don't need complex control:
+///
+/// 5. [Self::spawn_thread] - detached, new runtime in a new thread
+///
 /// [async-profiler]: https://github.com/async-profiler/async-profiler
 pub struct Profiler {
     reporting_interval: Duration,
@@ -923,7 +795,7 @@ impl Profiler {
 
     /// Like [Self::spawn], but instead of spawning within the current Tokio
     /// runtime, spawns within a set Tokio runtime and then runs a thread that calls
-    /// [tokio::runtime::Runtime::block_on] on that runtime.
+    /// [block_on](tokio::runtime::Runtime::block_on) on that runtime.
     ///
     /// If your configuration is standard, use [Profiler::spawn_thread].
     ///
@@ -970,7 +842,7 @@ impl Profiler {
 
     /// Like [Self::spawn], but instead of spawning within the current Tokio
     /// runtime, spawns within a new Tokio runtime and then runs a thread that calls
-    /// [tokio::runtime::Runtime::block_on] on that runtime, setting up the runtime
+    /// [block_on](tokio::runtime::Runtime::block_on) on that runtime, setting up the runtime
     /// by itself.
     ///
     /// If your configuration is less standard, use [Profiler::spawn_thread_to_runtime]. Calling
@@ -1080,7 +952,7 @@ impl Profiler {
 
     /// Like [Self::spawn_controllable], but instead of spawning within the current Tokio
     /// runtime, spawns within a set Tokio runtime and then runs a thread that calls
-    /// [tokio::runtime::Runtime::block_on] on that runtime.
+    /// [block_on](tokio::runtime::Runtime::block_on) on that runtime.
     ///
     /// `spawn_fn` should be [`std::thread::spawn`], or some function that behaves like it (to
     /// allow for configuring thread properties, for example thread names).
@@ -1243,7 +1115,7 @@ async fn profiler_tick<E: ProfilerEngine>(
         #[cfg(feature = "aws-metadata-no-defaults")]
         let md = crate::metadata::aws::load_agent_metadata().await?;
         #[cfg(not(feature = "aws-metadata-no-defaults"))]
-        let md = crate::metadata::AgentMetadata::Other;
+        let md = crate::metadata::AgentMetadata::NoMetadata;
         tracing::debug!("loaded metadata");
         agent_metadata.replace(md);
     }
@@ -1597,7 +1469,7 @@ mod tests {
             r#"Some(LocalReporter { directory: "." })"#
         );
         match reporter.agent_metadata {
-            Some(AgentMetadata::Other) => {}
+            Some(AgentMetadata::NoMetadata) => {}
             bad => panic!("{bad:?}"),
         };
     }
diff --git a/src/reporter/local.rs b/src/reporter/local.rs
@@ -25,9 +25,9 @@ enum LocalReporterError {
 ///
 /// It does not currently use the metadata, so if you are using
 /// [LocalReporter] alone, rather than inside a [MultiReporter], you
-/// can just use [AgentMetadata::Other] as metadata.
+/// can just use [AgentMetadata::NoMetadata] as metadata.
 ///
-/// [AgentMetadata::Other]: crate::metadata::AgentMetadata::Other
+/// [AgentMetadata::NoMetadata]: crate::metadata::AgentMetadata::NoMetadata
 /// [MultiReporter]: crate::reporter::multi::MultiReporter
 ///
 /// ### Example
diff --git a/src/reporter/s3.rs b/src/reporter/s3.rs
@@ -145,7 +145,9 @@ fn make_s3_file_name(
             let task_arn = ecs_task_arn.replace("/", "-").replace("_", "-");
             format!("ecs_{task_arn}_")
         }
+        #[allow(deprecated)]
         AgentMetadata::Other => "onprem__".to_string(),
+        AgentMetadata::NoMetadata => "unknown__".to_string(),
     };
     let time: chrono::DateTime<chrono::Utc> = time.into();
     let time = time
@@ -252,7 +254,8 @@ mod test {
         );
     }
 
-    #[test_case(AgentMetadata::Other, "profile_pg_onprem___<pid>_<time>.zip"; "other")]
+    #[test_case(#[allow(deprecated)] { AgentMetadata::Other }, "profile_pg_onprem___<pid>_<time>.zip"; "other")]
+    #[test_case(AgentMetadata::NoMetadata, "profile_pg_unknown___<pid>_<time>.zip"; "no-metadata")]
     #[test_case(AgentMetadata::Ec2AgentMetadata {
         aws_account_id: "1".into(),
         aws_region_id: "us-east-1".into(),
diff --git a/src/s3-example-custom-metadata.md b/src/s3-example-custom-metadata.md
diff --git a/src/s3-example.md b/src/s3-example.md

Original file line number	Diff line number	Diff line change
`@@ -119,7 +119,7 @@ async fn main_internal(args: Args) -> Result<(), anyhow::Error> {`
`119`	`119`	`let profiler = match (&args.local, args.s3_bucket_args()) {`
`120`	`120`	`(Some(local), S3BucketArgs { .. }) => profiler`
`121`	`121`	`.with_reporter(LocalReporter::new(local))`
`122`		`- .with_custom_agent_metadata(AgentMetadata::Other),`
	`122`	`+ .with_custom_agent_metadata(AgentMetadata::NoMetadata),`
`123`	`123`	`#[cfg(feature = "s3-no-defaults")]`
`124`	`124`	`(`
`125`	`125`	`_,`