Skip to content

feat(prof): configurable allocation sampling distance#3227

Merged
realFlowControl merged 5 commits intomasterfrom
florian/configurable-allocation-sampling-frequence
May 8, 2025
Merged

feat(prof): configurable allocation sampling distance#3227
realFlowControl merged 5 commits intomasterfrom
florian/configurable-allocation-sampling-frequence

Conversation

@realFlowControl
Copy link
Copy Markdown
Member

@realFlowControl realFlowControl commented May 2, 2025 •
edited
Loading

Description

This makes the allocation profiling sampling distance (formerly fixed 4MB) configurable through the DD_PROFILING_ALLOCATION_SAMPLING_DISTANCE environment variable or datadog.profiling.allocation_sampling_distance PHP INI setting.

4MB should be enough for anyone - 😉

In most cases this is true, but if your application is heavily allocating lots of memory you'd see elevated overhead from the allocation profiler. This feature gives you a level to adjust in case you do want allocation profiling, but see elevated overhead because the application you are running is allocating a lot of memory.

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

PROF-11753

@github-actions github-actions Bot added the profiling Relates to the Continuous Profiler label May 2, 2025
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 2, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.41%. Comparing base (0697a7f) to head (c002610).
Report is 2 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3227   +/-   ##
=========================================
  Coverage     71.41%   71.41%           
  Complexity     2948     2948           
=========================================
  Files           118      118           
  Lines         11633    11633           
=========================================
  Hits           8308     8308           
  Misses         3325     3325
Flag Coverage Δ
tracer-php 71.41% <ø> (ø)

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

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 0697a7f...c002610. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@realFlowControl realFlowControl force-pushed the florian/configurable-allocation-sampling-frequence branch from 0802dd0 to 0c5e8bd Compare May 2, 2025 09:32
@pr-commenter
Copy link
Copy Markdown

pr-commenter Bot commented May 2, 2025 •
edited
Loading

Benchmarks [ profiler ]

Benchmark execution time: 2025-05-08 06:58:03

Comparing candidate commit c002610 in PR branch florian/configurable-allocation-sampling-frequence with baseline commit 0697a7f in branch master.

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

@realFlowControl realFlowControl force-pushed the florian/configurable-allocation-sampling-frequence branch 2 times, most recently from d16b77a to dbf9537 Compare May 2, 2025 10:31
@realFlowControl realFlowControl force-pushed the florian/configurable-allocation-sampling-frequence branch from dbf9537 to 97b9d21 Compare May 2, 2025 14:54
@realFlowControl realFlowControl marked this pull request as ready for review May 2, 2025 19:28
@realFlowControl realFlowControl requested review from a team as code owners May 2, 2025 19:28
@morrisonlevi morrisonlevi changed the title feat(profiling) make allocation profiling sampling distance configurable feat(prof): configurable allocation sampling distance May 6, 2025
morrisonlevi
morrisonlevi approved these changes May 6, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@morrisonlevi morrisonlevi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have some nits with the PR, but I do have a bigger concern: if we add dynamic sampling, how are these supposed to interact? I wonder how Python has done things recently, we should maybe align, or if they need to change something, better to do it now before it gets widely adopted. Maybe DD_PROFILING_ALLOCATION_SAMPLING_DISTANCE=dynamic?

Comment thread .github/workflows/prof_correctness.yml Outdated
Comment thread profiling/src/allocation/mod.rs
Comment thread profiling/src/config.rs
Comment thread profiling/src/config.rs Outdated
@realFlowControl
Copy link
Copy Markdown
Member Author

realFlowControl commented May 6, 2025

Thats exactly the idea, DD_PROFILING_ALLOCATION_SAMPLING_DISTANCE for now is the distance in byte, in the future this will be unsigned int (distance in byte) or the string dynamic where dynamic would be the new default, once we have dynamic sampling implemented.

morrisonlevi
morrisonlevi reviewed May 6, 2025
View reviewed changes
Comment thread profiling/src/profiling/mod.rs Outdated
morrisonlevi
morrisonlevi reviewed May 6, 2025
View reviewed changes
Comment thread profiling/tests/phpt/allocation_bind_static_01.phpt
@realFlowControl realFlowControl merged commit a1c3353 into master May 8, 2025
823 of 829 checks passed
@realFlowControl realFlowControl deleted the florian/configurable-allocation-sampling-frequence branch May 8, 2025 11:03
@github-actions github-actions Bot added this to the 1.9.0 milestone May 8, 2025
@realFlowControl realFlowControl mentioned this pull request May 9, 2025
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@morrisonlevi morrisonlevi morrisonlevi approved these changes

Assignees

No one assigned

Labels

profiling Relates to the Continuous Profiler

Projects

None yet

Milestone

1.9.0

Development

Successfully merging this pull request may close these issues.

3 participants

@realFlowControl @codecov-commenter @morrisonlevi