-
Notifications
You must be signed in to change notification settings - Fork 3
Better docs, also allow spawning its own runtime #97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
8a5cdb4
to
59b42a7
Compare
59b42a7
to
4dc8825
Compare
This makes ProfilerOptions non-exhaustive, and is therefore technically a breaking-change, but that was a bug
src/profiler.rs
Outdated
/// Sets the reporting interval. | ||
/// Sets the reporting interval (default: 30 seconds). | ||
/// | ||
/// This is the interval that samples are *reported* at, and is unrelated |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/profiler.rs
Outdated
/// to the interval in which samples are *collected*. There are few | ||
/// needs to change this from the default 30 seconds. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
#[cfg_attr(not(feature = "s3-no-defaults"), doc = "`S3Reporter`.")] | ||
#[cfg_attr(feature = "s3-no-defaults", doc = "[`S3Reporter`].")] |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/profiler.rs
Outdated
/// .with_reporter(LocalReporter::new(path)) | ||
/// .with_custom_agent_metadata(AgentMetadata::Other) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
src/reporter/local.rs
Outdated
/// # async fn main() -> Result<(), SpawnError> { | ||
/// let profiler = ProfilerBuilder::default() | ||
/// .with_reporter(LocalReporter::new("/tmp/profiles")) | ||
/// .with_custom_agent_metadata(AgentMetadata::Other) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
self.detach_inner(); | ||
} | ||
|
||
fn spawn_attached( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
somewhat hard to follow the purpose of these different spawn code paths. some comments may help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added comment
src/profiler.rs
Outdated
/// [Profiler::spawn_controllable_thread_to_runtime]. | ||
/// | ||
/// `spawn_fn` should be [`std::thread::spawn`], or some function that behaves like it (to | ||
/// allow for configuring thread properties). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an example that sets the thread name would probably be very helpful
src/profiler.rs
Outdated
/// | ||
/// This function must be run within a Tokio runtime. If your application does | ||
/// not have a `main` Tokio runtime, see | ||
/// [Profiler::spawn_controllable_thread_to_runtime]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this link to spawn_thread
instead?
pub fn with_native_mem(mut self, native_mem_interval: String) -> Self { | ||
self.native_mem = Some(native_mem_interval); | ||
pub fn with_native_mem_bytes(mut self, native_mem_interval: usize) -> Self { | ||
self.native_mem = Some(native_mem_interval.to_string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this works without an string suffix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea it works (bytes precision).
src/profiler.rs
Outdated
self | ||
} | ||
|
||
/// Sets the interval, in nanoseconds, in which the profiler will collect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use a Duration
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
972c30f
to
95eab39
Compare
0f12ba2
to
a7bb694
Compare
a7bb694
to
d5e60a8
Compare
shell: bash | ||
working-directory: tests | ||
run: chmod +x simple pollcatch-decoder && LD_LIBRARY_PATH=$PWD ./integration.sh | ||
run: chmod +x simple pollcatch-decoder && LD_LIBRARY_PATH=$PWD ./integration.sh && LD_LIBRARY_PATH=$PWD ./separate_runtime_integration.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for ease of debugging we might want two separate targets / steps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels overcomplicated to me. If we have a problem with a big step, we can always split it.
src/reporter/local.rs
Outdated
/// | ||
/// 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. |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
/// let profiler = ProfilerBuilder::default() | ||
/// .with_local_reporter("/tmp/profiles") | ||
/// .build(); | ||
/// # if false { // don't spawn the profiler in doctests |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
/// CPU-time samples, via the [async-profiler `interval` option]. | ||
/// | ||
/// CPU-time samples (JFR `jdk.ExecutionSample`) sample only threads that | ||
/// are currently running on a CPU, not threads that are sleeping. | ||
/// | ||
/// It can use a higher frequency than wall-clock sampling since the | ||
/// number of the threads that are running on a CPU at a given time is | ||
/// naturally limited by the number of CPUs, while the number of sleeping | ||
/// threads can be much larger. | ||
/// | ||
/// The default is to do a CPU-time sample every 100 milliseconds. | ||
/// | ||
/// The async-profiler agent collects both CPU time and wall-clock time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is really important info about the profiler in general. I wonder if we should hoist it into the readme. (the wall vs. cpu discussion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably also need to touch on the fact that tasks are one level higher still so you can't really see "wall clock" profiling for async programs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so you can't really see "wall clock" profiling for async programs
It is plenty useful since it is what catches problems with synchronous locks. It just tells you the answer to a different question.
src/profiler.rs
Outdated
doc = "[`S3Reporter`]: crate::reporter::s3::S3Reporter" | ||
)] | ||
/// | ||
#[cfg_attr(feature = "s3-no-defaults", doc = "## Example")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if worth it but you could probably make this #[cfg_attr(feature = "s3-no-defaults", doc = include_str!("s3-example.md")]
which would it easier to edit and easier to read this code and the similar code below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth it, the cargo fmt way is ugly
src/profiler.rs
Outdated
|
||
/// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this link doesn't render in any of the three places it is used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renders for me
|
||
/// Spawns this [RunningProfiler] into a separate thread within a new Tokio runtime, | ||
/// and returns a [RunningProfilerThread] attached to it. | ||
fn spawn_attached( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a table or other doc at the struct level with spawn
spawn_controllable
, spawn_controllable_to_thread
etc. in struct docs would be very helpful in picking which one to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
f6bfa90
to
1d050fd
Compare
1d050fd
to
c2ef426
Compare
📬 Issue #, if available:
✍️ Description of changes:
🔏 By submitting this pull request