[PROF-3760] Revert "Merge pull request #3760 from DataDog/ivoanjo/prof-10123-placeholder-missing-allocations" #3775
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…f-10123-placeholder-missing-allocations" **What does this PR do?** This PR reverts the change from #3760. While we still want to add this feature, the implementation in #3760 can cause heap profiler to fail with an exception. This failure also uncovered a few other potential bugs in our error handling code. Thus, to be on the safe side, and because we want to get a new release out, I'm reverting the whole PR, until I can re-open a fixed version. **Motivation:** Remove blockers from the 2.2.0 release. **Additional Notes:** This reverts commit 7732142, reversing changes made to ca006e9. **How to test the change?** Validate that CI is still green.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3775 +/- ##
==========================================
- Coverage 97.92% 97.91% -0.01%
==========================================
Files 1243 1243
Lines 74964 74904 -60
Branches 3613 3611 -2
==========================================
- Hits 73406 73344 -62
- Misses 1558 1560 +2 ☔ View full report in Codecov by Sentry. |
TonyCTHsu
approved these changes
Jul 10, 2024
ivoanjo
added a commit
that referenced
this pull request
Jul 18, 2024
…eap profiler, try 2 Adding the "skipped samples" placeholder breaks the heap profiler, as it starts failing with: > WARN -- datadog: [datadog] CpuAndWallTimeWorker thread error. Cause: > RuntimeError Ended a heap recording that was not started > WARN -- datadog: [datadog] Detected issue with profiler (worker > component), stopping profiling. This is because the stack recorder up until now used the check for `alloc_samples > 0` as a proxy for "this is an allocation sample, and someone already called `track_object(...)` with it". The addition of the skipped samples placeholder broke this assumption, as it uses `alloc_samples > 0` but explicitly there's no object being tracked. To fix this issue, I've added a new argument to the `sample_values` that explicitly marks a sample as also being heap (or not). Thus, we can now explicitly tell the stack recorder which situation we are in using that flag, instead of trying to derive it from `alloc_samples > 0`. This is actually the second attempt at fixing this. The first attempt got reverted along with #3775 because it tried to detect placeholders (and thus used a placeholder as a proxy for "someone called `track_object(...)`". This was incorrect because there's other placeholders than just the "skipped samples" (case in point: the "In native code" placeholder), which can happen together with an object allocation.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This PR reverts the change from #3760.
While we still want to add this feature, the implementation in #3760 can cause heap profiler to fail with an exception.
This failure also uncovered a few other potential bugs in our error handling code.
Thus, to be on the safe side, and because we want to get a new release out, I'm reverting the whole PR, until I can re-open a fixed version.
Motivation:
Remove blockers from the 2.2.0 release.
Additional Notes:
This reverts commit 7732142, reversing changes made to ca006e9.
How to test the change?
Validate that CI is still green.