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

fix duration in (fast)delta profiles #1612

Merged
merged 2 commits into from Dec 8, 2022

Conversation

pmbauer
Copy link
Member

@pmbauer pmbauer commented Dec 8, 2022

What does this PR do?

Fixes a bug in the duration computation for fast delta

The current version produces delta pprofs with a DurationNanos ~equal to the life of the process.

Note: this was a regression from a previously tested version of fastdelta. Unfortunately, there wasn't sufficient test coverage for this case. This pull addresses that too.

Describe how to test/QA your changes

follow test procedure executed for fast delta

  • test in reliability env
  • test on production shadow instance

Reviewer's Checklist

  • If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
  • Changed code has unit tests for its functionality.
  • If this interacts with the agent in a new way, a system test has been added.

@pmbauer pmbauer requested a review from a team as a code owner December 8, 2022 00:58
Copy link
Contributor

@nsrip-dd nsrip-dd left a comment

Choose a reason for hiding this comment

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

Thanks Paul!

The TestRepeatedHeapProfile test (which you have to enable with an env var since it's too expensive to run in our CI) also catches this bug if you add checking for the timestamps, which I neglected to do before.Since it's not run during our CI it's slightly less useful, though. Here's a patch if you think it's worth making that change as well:

diff --git a/profiler/internal/fastdelta/fd_test.go b/profiler/internal/fastdelta/fd_test.go
index 17404b80..1ac882dc 100644
--- a/profiler/internal/fastdelta/fd_test.go
+++ b/profiler/internal/fastdelta/fd_test.go
@@ -340,6 +340,9 @@ func makeGolden(t testing.TB, before, after []byte, fields ...pprofutils.ValueTy
        if err != nil {
                t.Fatal(err)
        }
+       c.TimeNanos = a.TimeNanos
+       // DurationNanos is the time period covered by the profile.
+       c.DurationNanos = a.TimeNanos - b.TimeNanos
        return c
 }
 
@@ -721,6 +724,10 @@ func TestRepeatedHeapProfile(t *testing.T) {
 
                golden := makeGolden(t, before, after, fields...)
 
+               if golden.DurationNanos != delta.DurationNanos {
+                       t.Fatalf("iter %d: want duration %v, got %v", i, golden.DurationNanos, delta.DurationNanos)
+               }
+
                golden.Scale(-1)
                diff, err := profile.Merge([]*profile.Profile{delta, golden})
                if err != nil {

@nsrip-dd nsrip-dd added this to the v1.45.0 milestone Dec 8, 2022
@pmbauer
Copy link
Member Author

pmbauer commented Dec 8, 2022

Yep. I considered adding the time check to TestRepeatedHeapProfile, but as it doesn't run by default or in CI, I thought it worth breaking out. Too, that's a fairly heavyweight test.

@pr-commenter
Copy link

pr-commenter bot commented Dec 8, 2022

Benchmarks

Comparing candidate commit 477791a in PR branch pmbauer/fix-fastdelta-profile-duration with baseline commit ce33558 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 6 cases.

@nsrip-dd nsrip-dd merged commit 2f1555e into main Dec 8, 2022
@nsrip-dd nsrip-dd deleted the pmbauer/fix-fastdelta-profile-duration branch December 8, 2022 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants