Skip to content

Conversation

@arielb1
Copy link
Collaborator

@arielb1 arielb1 commented Apr 16, 2025

📬 Issue #, if available:

✍️ Description of changes:

🔏 By submitting this pull request

  • I confirm that I've made a best effort attempt to update all relevant documentation.
  • I confirm that my contribution is made under the terms of the Apache 2.0 license.

@arielb1
Copy link
Collaborator Author

arielb1 commented Apr 16, 2025

r? @rcoh

Comment on lines +81 to +83
{
rt.on_before_task_poll(|_| async_profiler_agent::pollcatch::before_poll_hook())
.on_after_task_poll(|_| async_profiler_agent::pollcatch::after_poll_hook());
Copy link
Collaborator

Choose a reason for hiding this comment

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

this has to be set in the builder right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yea

}
}

impl super::profiler::ProfilerEngine for AsProf {
Copy link
Collaborator

Choose a reason for hiding this comment

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

AsProf — I assume this means AsyncProfiler but since As is an english word common in Rust this is a bit confusing. Consider renaming to AsyncProfiler

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this was copied from the previous agent. I can rename.

Comment on lines +25 to +32
#[cfg(not(any(
all(target_arch = "x86_64", target_feature = "sse2"),
target_arch = "aarch64",
)))]
#[inline]
fn _now() -> u64 {
0
}
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 emit a deprecation in this case? Or some sort of warning?

Copy link
Collaborator Author

@arielb1 arielb1 Apr 16, 2025

Choose a reason for hiding this comment

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

the general sense is that async-profiler only support x86 and ARM anyway

Comment on lines +40 to +42
pub fn main() -> anyhow::Result<()> {
let mut rt: tokio::runtime::Builder = tokio::runtime::Builder::new_multi_thread();
rt.enable_all();
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you file an issue for turning an example into an integration test that we can run to validate future changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

opened #19

@arielb1 arielb1 merged commit f532aae into main Apr 16, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants