-
Notifications
You must be signed in to change notification settings - Fork 68
feat: auto listener benchmark #420
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
base: main
Are you sure you want to change the base?
Changes from 6 commits
1385dc2
37e5c1f
89f98a0
61701e1
7c3b6b0
c6b5f71
f1d13de
a829f75
7e088fa
e20ad33
a00b229
2e2c479
950efdc
ec1b4bf
2c72726
871b652
0c4b3cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
## vNext | ||
|
||
|
||
## v0.14.0 | ||
|
||
Released 2025-July-24 | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -5,21 +5,26 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||
Hardware: Apple M4 Pro | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Total Number of Cores: 10 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
(Inside multipass vm running Ubuntu 22.04) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
// When no listener | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
// When no listener (automatic disable via RAII guard) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Test | Average time| | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|-----------------------------|-------------| | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| User_Event_4_Attributes | 8 ns | | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| User_Event_6_Attributes | 8 ns | | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| User_Event_4_Attributes | 19.2 ns | | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| User_Event_6_Attributes | 19.6 ns | | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| User_Event_4_Attributes_EventName_Custom | 20.2 ns | | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| User_Event_4_Attributes_EventName_FromLogRecord | 20.6 ns | | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
// When listener is enabled | ||||||||||||||||||||||||||||||||||||||||||||||||||||
// Run below to enable | ||||||||||||||||||||||||||||||||||||||||||||||||||||
// When listener is enabled (automatic enable via RAII guard) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
// The benchmark now automatically enables/disables the listener | ||||||||||||||||||||||||||||||||||||||||||||||||||||
// using the commands below internally: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
// echo 1 | sudo tee /sys/kernel/debug/tracing/events/user_events/myprovider_L2K1/enable | ||||||||||||||||||||||||||||||||||||||||||||||||||||
// Run below to disable | ||||||||||||||||||||||||||||||||||||||||||||||||||||
// echo 0 | sudo tee /sys/kernel/debug/tracing/events/user_events/myprovider_L2K1/enable | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Test | Average time| | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|-----------------------------|-------------| | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| User_Event_4_Attributes | 530 ns | | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| User_Event_6_Attributes | 586 ns | | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| User_Event_4_Attributes_EventName_Custom | 590 ns | | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| User_Event_4_Attributes_EventName_FromLogRecord | 595 ns | | ||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
// running the following from the current directory | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -35,6 +40,161 @@ use opentelemetry_user_events_logs::Processor; | |||||||||||||||||||||||||||||||||||||||||||||||||||
use tracing::error; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
use tracing_subscriber::prelude::*; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
use tracing_subscriber::Registry; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
use std::process::Command; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
const PROVIDER_NAME: &str = "myprovider"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// Suffix used by the kernel user_events provider naming in these benchmarks. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// Documented to avoid magic strings in path construction. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
const USER_EVENTS_PROVIDER_SUFFIX: &str = "_L2K1"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The hardcoded suffix "_L2K1" is system-specific and may not work across different systems or kernel versions. Consider dynamically discovering the actual suffix by scanning the directory
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
/// RAII guard for enabling/disabling user events listener | ||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The documentation for UserEventsListenerGuard should explain the purpose of the Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||||||||||||||||||||||||||||||||||||
/// | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// This guard automatically enables the user events listener when created and | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// disables it when dropped, ensuring proper cleanup even if the benchmark | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// panics or exits early. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// The guard tracks whether the listener was already enabled before it was | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// created, and only disables it on drop if it wasn't already enabled. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// This prevents interfering with other tests or processes that might have | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// enabled the listener. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
struct UserEventsListenerGuard { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
provider_name: String, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
was_enabled: bool, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
impl UserEventsListenerGuard { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// Enable the user events listener for the given provider | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// This method: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// 1. Checks if the listener is already enabled | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// 2. Enables it if it's not already enabled | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// 3. Returns a guard that will disable the listener on drop (if it wasn't already enabled) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// # Arguments | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// * `provider_name` - The name of the provider to enable/disable | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// # Returns | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// * `Ok(Self)` - A guard that will disable the listener on drop | ||||||||||||||||||||||||||||||||||||||||||||||||||||
/// * `Err(String)` - Error message if enabling failed | ||||||||||||||||||||||||||||||||||||||||||||||||||||
fn enable(provider_name: &str) -> Result<Self, String> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
let enable_path = format!( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
"/sys/kernel/debug/tracing/events/user_events/{}{}//enable", | ||||||||||||||||||||||||||||||||||||||||||||||||||||
provider_name, USER_EVENTS_PROVIDER_SUFFIX | ||||||||||||||||||||||||||||||||||||||||||||||||||||
).replacen("//enable", "/enable", 1); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
// Check if already enabled | ||||||||||||||||||||||||||||||||||||||||||||||||||||
let check_output = Command::new("sudo") | ||||||||||||||||||||||||||||||||||||||||||||||||||||
.arg("cat") | ||||||||||||||||||||||||||||||||||||||||||||||||||||
.arg(&enable_path) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
.output() | ||||||||||||||||||||||||||||||||||||||||||||||||||||
.map_err(|e| format!("Failed to check listener status: {e}"))?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
let was_enabled = check_output.status.success() && | ||||||||||||||||||||||||||||||||||||||||||||||||||||
String::from_utf8_lossy(&check_output.stdout).trim() == "1"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
// Enable the listener using a safe approach (no shell interpretation) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
let mut enable_cmd = Command::new("sudo"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
enable_cmd.arg("tee").arg(&enable_path); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
enable_cmd.stdin(std::process::Stdio::piped()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
let enable_output = enable_cmd | ||||||||||||||||||||||||||||||||||||||||||||||||||||
.spawn() | ||||||||||||||||||||||||||||||||||||||||||||||||||||
.and_then(|mut child| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
if let Some(stdin) = child.stdin.as_mut() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
use std::io::Write; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
stdin.write_all(b"1\n")?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
stdin.write_all(b"1\n")?; | |
stdin.write_all(ENABLE_VALUE)?; |
Copilot uses AI. Check for mistakes.
Outdated
Copilot
AI
Aug 14, 2025
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.
Using sudo
in benchmarks poses security risks and may fail in environments without sudo access. Consider checking if the process already has the necessary permissions or providing a fallback mechanism for non-privileged execution.
)); | |
let was_enabled = match fs::read_to_string(&enable_path) { | |
Ok(contents) => contents.trim() == "1", | |
Err(e) => { | |
if e.kind() == io::ErrorKind::PermissionDenied { | |
return Err(format!( | |
"Insufficient permissions to read '{}'. Please run the benchmark as root or with appropriate capabilities (CAP_SYS_ADMIN). Error: {}", | |
enable_path, e | |
)); | |
} else { | |
return Err(format!("Failed to check listener status: {}", e)); | |
} | |
} | |
}; | |
// Enable the listener by writing "1" to the enable file | |
if let Err(e) = fs::write(&enable_path, b"1\n") { | |
if e.kind() == io::ErrorKind::PermissionDenied { | |
return Err(format!( | |
"Insufficient permissions to write to '{}'. Please run the benchmark as root or with appropriate capabilities (CAP_SYS_ADMIN). Error: {}", | |
enable_path, e | |
)); | |
} else { | |
return Err(format!("Failed to enable listener: {}", e)); | |
} |
Copilot uses AI. Check for mistakes.
Copilot
AI
Aug 14, 2025
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.
The check_user_events_available
function uses sudo
which may not be available in all benchmark environments. Consider checking file permissions first or providing a non-privileged alternative.
} | |
let path = "/sys/kernel/tracing/user_events_status"; | |
fs::read_to_string(path) | |
.map_err(|e| format!("Failed to read {}: {}", path, e)) |
Copilot uses AI. Check for mistakes.
Copilot
AI
Aug 14, 2025
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.
[nitpick] The dummy guard uses was_enabled: true
to prevent cleanup, but this is misleading since it suggests the listener was actually enabled. Consider adding a separate boolean field like is_dummy: bool
to make the intent clearer.
Copilot uses AI. Check for mistakes.
Outdated
Copilot
AI
Aug 14, 2025
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.
Using the magic string "dummy" for the provider name is unclear. Consider using a descriptive constant like DUMMY_PROVIDER_NAME
or making it clear this value is unused in this context.
provider_name: "dummy".to_string(), | |
provider_name: DUMMY_PROVIDER_NAME.to_string(), |
Copilot uses AI. Check for mistakes.
Outdated
Copilot
AI
Aug 14, 2025
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.
The Drop implementation uses sudo
which could fail in restricted environments or CI systems. Consider adding error handling that doesn't panic and provides informative logging when sudo access is unavailable.
Copilot uses AI. Check for mistakes.
Outdated
Copilot
AI
Aug 14, 2025
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.
[nitpick] Similar to the enable value, b"0\n"
should be extracted as a constant const DISABLE_VALUE: &[u8] = b"0\n";
for consistency and maintainability.
stdin.write_all(b"0\n")?; | |
stdin.write_all(DISABLE_VALUE)?; |
Copilot uses AI. Check for mistakes.
Outdated
Copilot
AI
Aug 14, 2025
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.
The error handling pattern with unwrap_or_else
and fallback to dummy_guard()
is repeated in multiple functions. Consider extracting this into a helper function to reduce code duplication and improve maintainability.
}); | |
let _guard = enable_listener_with_fallback(PROVIDER_NAME); |
Copilot uses AI. Check for mistakes.
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.
The magic string "_L2K1" lacks explanation of its meaning or origin. Consider adding documentation explaining what this kernel-generated suffix represents and why it's used in the user events provider naming convention.
Copilot uses AI. Check for mistakes.