Skip to content

ze: make sampling interval configurable #525

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.

Sign up for GitHub

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

Open
matheus-mcosta wants to merge 1 commit into
base: devel
Choose a base branch
from
Open

ze: make sampling interval configurable #525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion backends/ze/ze_sampling_plugin.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -755,7 +755,10 @@ void thapi_initialize_sampling_plugin(void) {

// register L0 sampler exactly once
{
struct timespec interval = {.tv_sec = 0, .tv_nsec = 50000000}; /* 50 ms */
const char *s = getenv("LTTNG_UST_ZE_SAMPLING_ENERGY_PERIOD_MS");
long milliseconds = s ? atol(s) : 50;
struct timespec interval = {.tv_sec = milliseconds / 1000,
.tv_nsec = (milliseconds % 1000) * 1000000L};
plugin_handle = thapi_register_sampling(&thapi_sampling_energy, &interval);
}
return;
Expand Down
44 changes: 44 additions & 0 deletions integration_tests/sampling.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,49 @@
bats_require_minimum_version 1.5.0

@test "sampling_interval_help" {
run iprof --help

[ "$status" -eq 0 ]
Comment on lines +3 to +6

@TApplencourt TApplencourt Aug 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We can use run !

If invoked with one of the following as the first argument, run will perform an implicit check on the exit status of the invoked command:

    -N  expect exit status N (0-255), fail if otherwise
    ! expect nonzero exit status (1-255), fail if command succeeds

(https://bats-core.readthedocs.io/en/stable/writing-tests.html#run-test-other-commands)

Lol I choose the wrong tests to do that. In the test where we do state neq 0 I mean :)

[[ "$output" == *"-i, --sample-interval MS"* ]]
[[ "$output" == *"Default: 50"* ]]
[[ "$output" == *"frequency, energy, engine, fabric-port, and memory"* ]]

@TApplencourt TApplencourt Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we do this kind of test any where else? I think it's bad idea to put the default on the help. If we change the default, we will need to change it here too.

}

@test "sampling_interval_is_passed_to_ze_sampler" {
for option in -i --sample-interval; do
trace="sampling_interval_trace_${option#-}"
rm -rf "$trace"

LTTNG_UST_ZE_LIBZE_LOADER=/dev/null \
iprof --no-analysis --sample --backends ze "$option" 125 \
--trace-output "$trace" -- \
bash -c 'test "$LTTNG_UST_ZE_SAMPLING_ENERGY_PERIOD_MS" = 125'
done
}
Comment on lines +12 to +22

@TApplencourt TApplencourt Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not sur what this is doing, we can remove it I think


@test "sampling_interval_rejects_invalid_values" {
for interval in 0 -1 nope; do
run iprof --sample --sample-interval "$interval" -- true

[ "$status" -ne 0 ]
[[ "$output" == *"ERROR:"* ]]
done
}

@test "sampling_interval_requires_sampling" {
run iprof --sample-interval 125 -- true

[ "$status" -ne 0 ]
[[ "$output" == *"--sample-interval requires --sample"* ]]
}

@test "sampling_interval_requires_ze_backend" {
run iprof --sample --sample-interval 125 --backends cxi -- true

[ "$status" -ne 0 ]
[[ "$output" == *"--sample-interval requires the ze backend"* ]]
}

@test "sampling_heartbeat" {
rm -rf heartbeat_trace

Expand Down
17 changes: 16 additions & 1 deletion xprof/xprof.rb.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -928,6 +928,7 @@ def all_env_tracers(usr_binary)
# is to call zesInit and set ZES_ENABLE_SYSMAN to 0
h['ZES_ENABLE_SYSMAN'] = 0
h['LTTNG_UST_ZE_SAMPLING_ENERGY'] = 1
h['LTTNG_UST_ZE_SAMPLING_ENERGY_PERIOD_MS'] = OPTIONS[:'sample-interval'] if OPTIONS.include?(:'sample-interval')
h['THAPI_SAMPLING_LIBRARIES'] << File.join(PKGLIBDIR, 'ze', 'libZESampling.so')
end
end
Expand Down Expand Up @@ -1067,6 +1068,14 @@ if $thapi_launch || __FILE__ == $PROGRAM_NAME
'Use -1 for no limit.', default: 80)

parser.on('-s', '--sample', 'Enable counters sampling.')
parser.on('-i', '--sample-interval MS', OptionParser::DecimalInteger,
'Set the Level Zero telemetry sampling interval in milliseconds.',
'Controls frequency, energy, engine, fabric-port, and memory samples.',
'Default: 50 ms.') do |interval|
raise(OptionParser::ParseError, 'sample interval must be greater than zero') unless interval.positive?

interval
end

parser.on('--metadata', 'Display trace metadata.')
parser.on('-v', '--version', 'Print the Version String.') do
Expand Down Expand Up @@ -1098,6 +1107,13 @@ if $thapi_launch || __FILE__ == $PROGRAM_NAME
options = {}
begin
parser.parse!(into: options)
options[:'backend-names'] = options[:backends].map { |name_level| name_level.split(':').first }
if options.include?(:'sample-interval')

@TApplencourt TApplencourt Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We sampling other thing like CXI should be do like name_level? sample_interval ze:121012, cxi:22323, heart_bit:1212 and if user just do --sample-interval *:1000 we apply it everywhere?

@matheus-mcosta matheus-mcosta Aug 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Should do multiple mappings in the format of --sample-interval <sample>:<MS>, for example --sample-interval ze:125,cxi:250? and apply to all if --sample-interval '*:100'?
This could lead to better control over the sampling granularity

@TApplencourt TApplencourt Aug 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Exactly. We do that for the backend:level already (like if you want mpi + level zero on the same table you can do --backend mpi:0,ze:0

Oh I guess we can do that for --sampling to a --sampling BACKEND:MS who enable it. So no need for another option, we just extend the current one.

Sorry , I make you do big change :D But it seem far better API wize

(and --sample versus --sampling I have no clue which one is best...)

raise OptionParser::ParseError, '--sample-interval requires --sample' unless options[:sample]
unless options[:'backend-names'].include?('ze')
raise OptionParser::ParseError, '--sample-interval requires the ze backend'
end
end
rescue OptionParser::InvalidOption => e
puts("ERROR: #{e}. Maybe missing --?")
print_help_and_exit(parser)
Expand All @@ -1106,7 +1122,6 @@ if $thapi_launch || __FILE__ == $PROGRAM_NAME
print_help_and_exit(parser)
end

options[:'backend-names'] = options[:backends].map { |name_level| name_level.split(':').first }
OPTIONS = options.freeze

if (launcher = %w[mpirun mpiexec].find { |b| ARGV.include?(b) })
Expand Down
Loading