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

[PROF-11405] Graduate heap profiling from alpha to preview, second try #4460

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Mar 5, 2025

What does this PR do?

A few days ago, we merged #4331 and #4401 to master.

Starting with #4331, we used a new libdatadog feature for implementing the Ruby heap profiler. While validating these changes, we discovered this libdatadog feature had a bug (DataDog/libdatadog#896) and thus reverted all PRs related to this heap profiler change until we could get the fix in #4409.

Because the bug was only in the new libdatadog feature, it was still OK to use the old approach with the buggy libdatadog (since it used an alternative, non-buggy, code path).

Now that we have the fix on the libdatadog side almost ready, I'm opening this PR to re-apply the changes from #4331 and #4401 to master.

This PR is thus almost the reverse of the revert PR in #4409. The only difference is that it does not yet include the changes from #4376 for the same reason that PR was needed originally.

Motivation:

Release heap profiling feature.

Change log entry
Yes. Please use the one in #4401 .

Additional Notes:

There's quite a diff on this PR, but there's no differences to those earlier PRs other than adding a regression test for this issue (the regression test is a separate commit that can be reviewed by itself).

In fact, you may notice that CI is red for this PR because I'm opening it before the libdatadog fix is released. My intention is to exactly come back once the new libdatadog is released, and see this PR turn green with a simple merge from master.

How to test the change?

The earlier PRs already included tests; and I added a regression test for the libdatadog issue in this PR.

ivoanjo added 3 commits March 4, 2025 16:54
…aged-string-storage-try2"

This reverts commit ded9fcb,
thus re-applying the changes from #4331.
…ap-profiler-preview"

This reverts commit 32ded2e, thus
re-applying the changes from #4401.
This test fails with libdatadog 16.0.1 and passes with
DataDog/libdatadog#896
@ivoanjo ivoanjo requested review from a team as code owners March 5, 2025 15:24
@github-actions github-actions bot added core Involves Datadog core libraries profiling Involves Datadog profiling labels Mar 5, 2025
ivoanjo added a commit that referenced this pull request Mar 5, 2025
…econd try

**What does this PR do?**

This reverts commit 569bf56.

We had previously reverted that commit in
#4376 but due to a bug
had to undo all the heap profiling-related changes in
#4409 .

After #4460 gets merged,
we can finally enable this benchmark again.

**Motivation:**

The benchmark was disabled for the reason mentioned in the comment.

**Additional Notes:**

I'm going to open this PR stacked on top of
#4460, but this PR should
only be merged after that other PR is merged to master.

**How to test the change?**

Validate this benchmark is now running in the benchmarking platform
results page.
@ivoanjo ivoanjo removed the core Involves Datadog core libraries label Mar 5, 2025
@datadog-datadog-prod-us1
Copy link
Contributor

Datadog Report

Branch report: ivoanjo/prof-11394-heap-profiling-graduate-try2
Commit report: f850521
Test service: dd-trace-rb

❌ 1 Failed (0 Known Flaky), 19818 Passed, 1366 Skipped, 3m 16.53s Total Time

❌ Failed Tests (1)

  • Datadog::Profiling::StackRecorder libdatadog managed string storage regression test when reusing managed string ids across multiple profiles produces correct profiles - rspec - Details

    Expand for error
     expected ["", "local root span id", "trace endpoint", "end_timestamp_ns", "cpu-time", "nanoseconds", "cpu-samp...lloc-samples", "alloc-samples-unscaled", "heap-live-samples", "heap-live-size", "bytes", "timeline"] to include "key", "hello", and "world"
     
     Failure/Error: expect(decoded_profile2.string_table).to include("key", "hello", "world")
       expected ["", "local root span id", "trace endpoint", "end_timestamp_ns", "cpu-time", "nanoseconds", "cpu-samp...lloc-samples", "alloc-samples-unscaled", "heap-live-samples", "heap-live-size", "bytes", "timeline"] to include "key", "hello", and "world"
     ./spec/datadog/profiling/stack_recorder_spec.rb:1095:in 'block (4 levels) in <top (required)>'
     ./spec/spec_helper.rb:238:in 'block (2 levels) in <top (required)>'
     ./spec/spec_helper.rb:123:in 'block (2 levels) in <top (required)>'
     /usr/local/bundle/gems/webmock-3.25.0/lib/webmock/rspec.rb:39:in 'block (2 levels) in <top (required)>'
     /usr/local/bundle/gems/rspec-wait-0.0.10/lib/rspec/wait.rb:47:in 'block (2 levels) in <top (required)>'
    

Copy link
Member

@Strech Strech left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
profiling Involves Datadog profiling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants