-
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 8 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -32,10 +37,178 @@ use opentelemetry_sdk::Resource; | |||||||||||||||||||||||||||||||||||||||||||||||||||
#[cfg(feature = "experimental_eventname_callback")] | ||||||||||||||||||||||||||||||||||||||||||||||||||||
use opentelemetry_user_events_logs::EventNameCallback; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
use opentelemetry_user_events_logs::Processor; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
use std::process::Command; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
use tracing::error; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
use tracing_subscriber::prelude::*; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
use tracing_subscriber::Registry; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
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); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
"/sys/kernel/debug/tracing/events/user_events/{}{}/enable", | |
provider_name, USER_EVENTS_PROVIDER_SUFFIX | |
); |
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] The hardcoded byte string b"1\n"
is repeated throughout the code. Consider extracting this as a constant like const ENABLE_VALUE: &[u8] = b"1\n";
to improve maintainability.
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.
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 success message should also be conditional on whether the listener was actually enabled by this call. When was_enabled
is true, the message "User events listener enabled" is misleading since no action was taken.
); | |
if was_enabled { | |
println!( | |
"User events listener was already enabled for provider: {}", | |
provider_name | |
); | |
} else { | |
println!( | |
"User events listener enabled for provider: {}", | |
provider_name | |
); | |
} |
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.
Same issue as in the enable method - the string formatting creates a double slash that needs to be replaced. Use the correct format string directly to avoid this workaround.
.replacen("//enable", "/enable", 1); | |
"/sys/kernel/debug/tracing/events/user_events/{}{}/enable", | |
self.provider_name, USER_EVENTS_PROVIDER_SUFFIX | |
); |
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 disable_path
variable is constructed using self.provider_name
which could be the dummy provider name if this is a dummy guard. However, the is_dummy
check occurs after the path construction. This could lead to attempting to write to an invalid path. Move the is_dummy
check before any path operations.
// Skip cleanup for dummy guards | |
if self.is_dummy { | |
return; | |
} | |
let disable_path = format!( | |
"/sys/kernel/debug/tracing/events/user_events/{}{}/enable", | |
self.provider_name, USER_EVENTS_PROVIDER_SUFFIX | |
); |
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.
[nitpick] The success case in Drop::drop() uses println!
while error cases use eprintln!
. Consider using consistent output streams or documenting the rationale for different output destinations.
println!( | |
eprintln!( |
Copilot uses AI. Check for mistakes.
Outdated
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 means the listener is always enabled when the benchmarks is run. What we really want is for the benchmark to run this with and without enabling the listener. A criterion bench group - like group.bench_function(enabled) and group.bench_function(disabled).
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.
Oh yes, good find - seems I missed while reviewing.
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.
Thank you for the excellent suggestion! You're absolutely right - the current approach always enables the listener, which doesn't provide meaningful performance comparison data.
I've restructured all benchmark functions to use criterion bench groups that run both with and without the listener enabled:
Each benchmark now creates a benchmark_group with "disabled" and "enabled" variants
The "disabled" case runs without any listener management (baseline performance)
The "enabled" case uses the RAII guard to enable/disable the listener
This provides proper comparison data showing the actual overhead of enabling user events
The changes maintain all the safety benefits (RAII guards, proper cleanup) while giving us the performance comparison we need. Users can now see the baseline performance vs. the overhead of having the listener active.
This addresses the core issue you identified - we now have proper benchmark data for both states rather than always running with the listener enabled.
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.