Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 91 additions & 2 deletions src/profiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,19 +184,108 @@ pub struct ProfilerBuilder {
}

impl ProfilerBuilder {
/// Sets the reporting interval.
/// Sets the reporting interval (default: 30 seconds).
///
/// This is the interval that samples are *reported* at, and is unrelated
Copy link
Collaborator

Choose a reason for hiding this comment

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

since reported and collected can both mean sampled in this context, I think we should change it to be more explicit:

This is the interval that samples are reported to the backend (e.g. S3, Local file etc.) and is unrelated to the interval in which the application is sampled by async profiler (which defaults to 10hz)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

/// to the interval in which samples are *collected*. There are few
/// needs to change this from the default 30 seconds.
Copy link
Collaborator

Choose a reason for hiding this comment

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

as written sounds like it is about to list reasons. we should either list them or change it to

Most users should not change this setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

///
/// ## Example
///
/// ```no_run
/// # use std::path::PathBuf;
/// # use std::time::Duration;
/// # use async_profiler_agent::profiler::{ProfilerBuilder, SpawnError};
/// # use async_profiler_agent::reporter::local::LocalReporter;
/// # let path = PathBuf::from(".");
/// let agent = ProfilerBuilder::default()
/// .with_reporter(LocalReporter::new(path))
/// .with_reporting_interval(Duration::from_secs(15))
/// .build()
/// .spawn()?;
/// # Ok::<_, SpawnError>(())
/// ```
pub fn with_reporting_interval(mut self, i: Duration) -> ProfilerBuilder {
self.reporting_interval = Some(i);
self
}

/// Sets the reporter.
/// Sets the [`Reporter`], which is used to upload the collected profiling
/// data. Common reporters are [`LocalReporter`], and, with the `s3-no-defaults`
/// feature enabled,
#[cfg_attr(not(feature = "s3-no-defaults"), doc = "`S3Reporter`.")]
#[cfg_attr(feature = "s3-no-defaults", doc = "[`S3Reporter`].")]
Comment on lines +362 to +363
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we assume docs are always built with --all-features?

[package.metadata.docs.rs]
all-features = true

Copy link
Contributor Author

@arielb1 arielb1 Sep 24, 2025

Choose a reason for hiding this comment

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

No, because people run cargo doc locally, and I don't want them to get spammy warnings.

Copy link
Contributor Author

@arielb1 arielb1 Sep 24, 2025

Choose a reason for hiding this comment

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

also the tests run cargo test locally and can fail if there are problems in examples.

/// It is also possible to write your own [`Reporter`].
///
/// If you want to output to multiple reporters, you can use
/// [`MultiReporter`].
///
/// [`LocalReporter`]: crate::reporter::local::LocalReporter
/// [`MultiReporter`]: crate::reporter::multi::MultiReporter
#[cfg_attr(
feature = "s3-no-defaults",
doc = "[`S3Reporter`]: crate::reporter::s3::S3Reporter"
)]
///
/// ## Example
///
/// ```no_run
/// # use std::path::PathBuf;
/// # use std::time::Duration;
/// # use async_profiler_agent::profiler::{ProfilerBuilder, SpawnError};
/// # use async_profiler_agent::reporter::local::LocalReporter;
/// # let path = PathBuf::from(".");
/// let agent = ProfilerBuilder::default()
/// .with_reporter(LocalReporter::new(path))
/// .build()
/// .spawn()?;
/// # Ok::<_, SpawnError>(())
/// ```
pub fn with_reporter(mut self, r: impl Reporter + Send + Sync + 'static) -> ProfilerBuilder {
self.reporter = Some(Box::new(r));
self
}

/// Provide custom agent metadata.
///
/// The async-profiler Rust agent sends metadata to the [Reporter] with
/// the identity of the current host and process, which is normally
/// transmitted as `metadata.json` within the generated `.zip` file,
/// using the schema format [`reporter::s3::MetadataJson`].
///
/// That metadata can later be used by tooling to be able to sort
/// profiling reports by host.
///
/// async-profiler Rust agent will by default try to fetch the metadata
/// using [IMDS] when running on [Amazon EC2] or [Amazon Fargate], and
/// will error if it's unable to find it. If you are running the
/// async-profiler agent on any other form of compute,
/// you will need to create and attach your own metadata
/// by calling this function.
///
/// ## Example
///
/// This will create a reporter with empty ([AgentMetadata::Other])
/// metadata.
///
/// ```no_run
/// # use std::path::PathBuf;
/// # use async_profiler_agent::profiler::{ProfilerBuilder, SpawnError};
/// # use async_profiler_agent::reporter::local::LocalReporter;
/// # use async_profiler_agent::metadata::AgentMetadata;
/// # let path = PathBuf::from(".");
/// let agent = ProfilerBuilder::default()
/// .with_reporter(LocalReporter::new(path))
/// .with_custom_agent_metadata(AgentMetadata::Other)
Copy link
Collaborator

Choose a reason for hiding this comment

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

in the local reporter the metadata is totally ignored right? so this example might be slightly misleading

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But AgentMetadata::Other prevents async-profiler from trying to auto-detect metadata, which is why it's important.

/// .build()
/// .spawn()?;
/// # Ok::<_, SpawnError>(())
/// ```
///
/// [`reporter::s3::MetadataJson`]: crate::reporter::s3::MetadataJson
/// [Amazon EC2]: https://aws.amazon.com/ec2
/// [Amazon Fargate]: https://aws.amazon.com/fargate
/// [IMDS]: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-metadata.html
pub fn with_custom_agent_metadata(mut self, j: AgentMetadata) -> ProfilerBuilder {
self.agent_metadata = Some(j);
self
Expand Down
26 changes: 26 additions & 0 deletions src/reporter/local.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,32 @@ enum LocalReporterError {
/// A reporter that reports into a directory.
///
/// The files are reported with the filename `yyyy-mm-ddTHH-MM-SSZ.jfr`
///
/// 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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think part of my confusion is from calling it Other. Should we call it NoMetadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

///
/// [AgentMetadata::Other]: crate::metadata::AgentMetadata::Other
/// [MultiReporter]: crate::reporter::multi::MultiReporter
///
/// ### Example
///
/// ```
/// # use async_profiler_agent::metadata::AgentMetadata;
/// # use async_profiler_agent::profiler::{ProfilerBuilder, SpawnError};
/// # use async_profiler_agent::reporter::local::LocalReporter;
/// # #[tokio::main]
/// # async fn main() -> Result<(), SpawnError> {
/// let profiler = ProfilerBuilder::default()
/// .with_reporter(LocalReporter::new("/tmp/profiles"))
/// .with_custom_agent_metadata(AgentMetadata::Other)
Copy link
Collaborator

Choose a reason for hiding this comment

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

with_custom_agent_metdata isn't required is it? So you shouldn't need to set it to use the LocalReporter?

Copy link
Collaborator

Choose a reason for hiding this comment

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

or is that just required to suppress the error about missing metadata? Maybe we could add a uses_metadata() method to the reporter trait or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, it will prevent an error due to missing metadata

I don't like the unorthogonality of not having uses_metadata

/// .build();
/// # if false { // don't spawn the profiler in doctests
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we just not run the doctests instead? They will still be compiled. (that also makes the build significantly faster)

Copy link
Contributor Author

@arielb1 arielb1 Sep 25, 2025

Choose a reason for hiding this comment

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

I wanted to run as much of the doctests as I can, to make sure we don't have weird panic problems

/// profiler.spawn()?;
/// # }
/// # Ok(())
/// # }
/// ```
#[derive(Debug)]
pub struct LocalReporter {
directory: PathBuf,
Expand Down
41 changes: 41 additions & 0 deletions src/reporter/multi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,47 @@ impl fmt::Display for MultiError {
/// A reporter that reports profiling results to several destinations.
///
/// If one of the destinations errors, it will continue reporting to the other ones.
///
/// ## Example
///
/// Output to both S3 and a local directory:
///
#[cfg_attr(feature = "s3-no-defaults", doc = "```no_run")]
#[cfg_attr(not(feature = "s3-no-defaults"), doc = "```compile_fail")]
/// # use async_profiler_agent::profiler::{ProfilerBuilder, SpawnError};
/// # use async_profiler_agent::reporter::Reporter;
/// # use async_profiler_agent::reporter::local::LocalReporter;
/// # use async_profiler_agent::reporter::multi::MultiReporter;
/// # use async_profiler_agent::reporter::s3::{S3Reporter, S3ReporterConfig};
/// # use aws_config::BehaviorVersion;
/// # use std::path::PathBuf;
/// #
/// # #[tokio::main]
/// # async fn main() -> Result<(), SpawnError> {
/// let bucket_owner = "<your account id>";
/// let bucket_name = "<your bucket name>";
/// let profiling_group = "a-name-to-give-the-uploaded-data";
/// let path = PathBuf::from("path/to/write/jfrs");
///
/// let sdk_config = aws_config::defaults(BehaviorVersion::latest()).load().await;
///
/// let reporter = MultiReporter::new(vec![
/// Box::new(LocalReporter::new(path)),
/// Box::new(S3Reporter::new(S3ReporterConfig {
/// sdk_config: &sdk_config,
/// bucket_owner: bucket_owner.into(),
/// bucket_name: bucket_name.into(),
/// profiling_group_name: profiling_group.into(),
/// })),
/// ]);
Comment on lines +62 to +69
Copy link
Collaborator

Choose a reason for hiding this comment

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

would probably be nice to add a builder API at some point to avoid callers needing to box themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feels like an insufficiently used API for that.

/// let profiler = ProfilerBuilder::default()
/// .with_reporter(reporter)
/// .build();
///
/// profiler.spawn()?;
/// # Ok(())
/// # }
/// ```
pub struct MultiReporter {
reporters: Vec<Box<dyn Reporter + Send + Sync>>,
}
Expand Down