Skip to content
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

feat(prof) I/O profiling preview #3083

Merged
merged 17 commits into from
Feb 20, 2025
Merged

Conversation

realFlowControl
Copy link
Member

@realFlowControl realFlowControl commented Feb 12, 2025

Description

This adds I/O profiling preview to the PHP profiler on Linux machines (it is only compiled for Linux, but technically this is a ELF64 feature and could work on other OS that are using ELF64 too):

  • adds DD_PROFILING_EXPERIMENTAL_IO_ENABLED / datadog.profiling.experimental_io_enabled configuration (default false)
  • adds new sample types:
    • socket_read_time
    • socket_write_time
    • file_read_time
    • file_write_time
    • socket_read_size
    • socket_write_size
    • file_read_size
    • file_write_size

Known limitations:

  • tests, including correctness are missing
  • benchmarks are missing
  • GOT hooking code and I/O profiling code is mixed
  • mmap() is not observed
  • SQLite is not observed (probably due to missing mmap() support)

Those things will be added/handled in future PRs, also because this feature is still experimental. The sampling distances are best guess and will likely be adjusted later.

https://datadoghq.atlassian.net/browse/PROF-11288

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.85%. Comparing base (4da93b3) to head (c406db0).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3083      +/-   ##
============================================
- Coverage     74.86%   74.85%   -0.02%     
  Complexity     2792     2792              
============================================
  Files           112      112              
  Lines         11046    11046              
============================================
- Hits           8270     8268       -2     
- Misses         2776     2778       +2     
Flag Coverage Δ
tracer-php 74.85% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4da93b3...c406db0. Read the comment docs.

@github-actions github-actions bot added the profiling Relates to the Continuous Profiler label Feb 12, 2025
@pr-commenter
Copy link

pr-commenter bot commented Feb 12, 2025

Benchmarks [ profiler ]

Benchmark execution time: 2025-02-20 17:04:02

Comparing candidate commit c406db0 in PR branch florian/poc-got-io-profiling with baseline commit 4da93b3 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 29 metrics, 7 unstable metrics.

@realFlowControl realFlowControl marked this pull request as ready for review February 12, 2025 16:34
@realFlowControl realFlowControl requested a review from a team as a code owner February 12, 2025 16:34
@realFlowControl realFlowControl force-pushed the florian/poc-got-io-profiling branch from 6244902 to 826265c Compare February 14, 2025 10:36
@realFlowControl realFlowControl force-pushed the florian/poc-got-io-profiling branch from 016d5c8 to 2b73c8e Compare February 20, 2025 10:14
ValueType::new("socket-read-size", "bytes"),
ValueType::new("socket-write-size", "bytes"),
ValueType::new("file-io-read-size", "bytes"),
ValueType::new("file-io-write-size", "bytes"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

At this point, with that many sample types, I think this should look like:

let cpu_time = system_settings.profiling_experimental_cpu_time_enabled;
let alloc = system_settings.profiling_allocation_enabled;
#[cfg(feature = "timeline")]
let timeline = system_settings.profiling_timeline_enabled;
#[cfg(not(feature = "timeline"))]
let timeline = false;
#[cfg(feature = "exception_profiling")]
let exception = system_settings.profiling_allocation_enabled;
#[cfg(not(feature = "exception_profiling"))]
let exception = false;
#[cfg(feature = "io_profiling")]
let io = system_settings.profiling_io_enabled;
#[cfg(not(feature = "io_profiling"))]
let io = false;
let sample_types = vec![
    (ValueType::new("sample", "count"), true, mem::offset_of!(SampleValues, interrupt_count)),
    (ValueType::new("wall-time", "nanoseconds"), true, mem::offset_of!(SampleValues, wall_time)),
    (ValueType::new("cpu-time", "nanoseconds"), cpu_time, mem::offset_of!(SampleValues, cpu_time)),
    (ValueType::new("alloc-samples", "count"), allocation, mem::offset_of!(SampleValues, alloc_samples)),
    (ValueType::new("alloc-size", "bytes"), allocation, mem::offset_of!(SampleValues, alloc_size)),
    (ValueType::new("timeline", "nanoseconds"), timeline, mem::offset_of!(SampleValues, timeline)),
    (ValueType::new("exception-samples", "count"), exception, mem::offset_of!(SampleValues, exception)),
    (ValueType::new("socket-read-time", "nanoseconds"), io, mem::offset_of!(SampleValues, socket_read_time)),
    (ValueType::new("socket-write-time", "nanoseconds"), io, mem::offset_of!(SampleValues, socket_write_time)),
    (ValueType::new("file-io-read-time", "nanoseconds"), io, mem::offset_of!(SampleValues, file_read_time)),
    (ValueType::new("file-io-write-time", "nanoseconds"), io, mem::offset_of!(SampleValues, file_write_time)),
    (ValueType::new("socket-read-size", "bytes"), io, mem::offset_of!(SampleValues, socket_read_size)),
    (ValueType::new("socket-write-size", "bytes"), io, mem::offset_of!(SampleValues, socket_write_size)),
    (ValueType::new("file-io-read-size", "bytes"), io, mem::offset_of!(SampleValues, file_read_size)),
    (ValueType::new("file-io-write-size", "bytes"), io, mem::offset_of!(SampleValues, file_write_size)),
];
// and create sample_types(_mask) with a small loop from the tuples as well as offsets for filter() function

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Does not need to be done now, just a suggestion for next time at least)

Copy link
Member Author

Choose a reason for hiding this comment

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

Doing this in a follow-up PR

@realFlowControl realFlowControl force-pushed the florian/poc-got-io-profiling branch from 853e15b to 79a3394 Compare February 20, 2025 14:31
@realFlowControl realFlowControl force-pushed the florian/poc-got-io-profiling branch from 1447603 to 64d29b9 Compare February 20, 2025 15:26
Copy link
Collaborator

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

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

Looks great to me now, thanks!

@realFlowControl realFlowControl merged commit c5148ba into master Feb 20, 2025
728 of 740 checks passed
@realFlowControl realFlowControl deleted the florian/poc-got-io-profiling branch February 20, 2025 17:11
@github-actions github-actions bot added this to the 1.7.0 milestone Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
profiling Relates to the Continuous Profiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants