Profiling: fix SystemSettings initialization #3579
Conversation
This also serves to document some AtomicPtr operations and orderings.
|
|
I've played with this PR and it works 🎉 Also adding a heads up: this PR might conflict with #3550 |
|
I've added the |
morrisonlevi
force-pushed
the
levi/minit-system-settings
branch
from
d75b2d0 to
f57cdf7
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3579 +/- ##
==========================================
+ Coverage 61.93% 62.01% +0.07%
==========================================
Files 140 140
Lines 13309 13309
Branches 1762 1762
==========================================
+ Hits 8243 8253 +10
+ Misses 4275 4268 -7
+ Partials 791 788 -3 see 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Benchmarks [ profiler ]Benchmark execution time: 2026-01-16 21:43:30 Comparing candidate commit c9706f3 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 29 metrics, 7 unstable metrics. |
morrisonlevi
changed the title
| datadog.profiling.allocation_enabled=1 | ||
| datadog.profiling.allocation_sampling_distance=1 | ||
| datadog.profiling.log_level=trace | ||
| datadog.profiling.output_pprof=/tmp/profiling-data/pprof |
There was a problem hiding this comment.
I think this should not be set in the test. CI sets this as an ENV variable to make sure tests run faster and not upload.
| datadog.profiling.output_pprof=/tmp/profiling-data/pprof |
realFlowControl
left a comment
realFlowControl
left a comment
There was a problem hiding this comment.
Looks good, I left a comment 🎉
| // ... except that the engine does the rounding in the wrong place for | ||
| // str_repeat, so it with our inputs it ends up doing this, with `f` being the | ||
| // function which determines the rounded amount: | ||
| // 1 * 87 + f(24 + 0 + 1) | ||
| // 87 + f(25) | ||
| // 87 + 32 = 119 | ||
| // So it over-allocates by 7 bytes, and the log will have 119 bytes. | ||
| // Our regex allows for 112 (correct) and 119 (observed). |
There was a problem hiding this comment.
@danielsn This little detail cost 1.5 hours of my day. Not now, but it's something we can fix in php-src some day.
Description
This PR:
SystemSettingsto minit.SystemSettingson first request to pick up env vars sent by php-fpm/apache (legacy behavior, IMO, kept to preserve behavior).NonZeroU32/NonZeroU64so that it's easier to see that the sampling distance cannot be <= 0..phpttest to prevent future regressions.Builds on #3578.
Motivation
This fixes a recent regression where
datadog.profiling.allocation_sampling_distancewasn't being applied. This occurred because we moved the initialization of the profiling stats toginit, and at that point,SystemSettingsare not initialized, so it picked up the default value.I hit this regression in my work on PR #3559.
Reviewer checklist