From 70354dfbc0c3cd180d0b7b7ca1c42bd8115a6a45 Mon Sep 17 00:00:00 2001 From: Nick Ripley Date: Mon, 28 Feb 2022 12:17:59 -0500 Subject: [PATCH 1/2] profiler: don't upload full profiles if delta profiling is enabled The full heap, mutex, and block profiles are not needed when delta profiling is enabled. Removing them (especially the full heap profile) results in a significant bandwidth and storage cost reduction. --- profiler/profile.go | 19 +++++++++++-------- profiler/profile_test.go | 16 ++++++---------- profiler/profiler_test.go | 9 ++++----- 3 files changed, 21 insertions(+), 23 deletions(-) diff --git a/profiler/profile.go b/profiler/profile.go index 9fc3b108ad..a3d82e07a1 100644 --- a/profiler/profile.go +++ b/profiler/profile.go @@ -214,22 +214,25 @@ func (p *profiler) runProfile(pt ProfileType) ([]*profile, error) { if err != nil { return nil, err } - profs := []*profile{{ - name: t.Filename, - data: data, - }} // Compute the deltaProf (will be nil if not enabled for this profile type). deltaStart := time.Now() deltaProf, err := p.deltaProfile(t, data) if err != nil { return nil, fmt.Errorf("delta profile error: %s", err) } - // Report metrics and append deltaProf if not nil. end := now() tags := append(p.cfg.tags, pt.Tag()) - // TODO(fg) stop uploading non-delta profiles in the next version of - // dd-trace-go after delta profiles are released. - if deltaProf != nil { + var profs []*profile + if !p.cfg.deltaProfiles || deltaProf == nil { + profs = append(profs, &profile{ + name: t.Filename, + data: data, + }) + } else { + // If the user has enabled delta profiles, then the delta + // profile is all we need for the backend and we can save + // significant storage and bandwidth by not including the full, + // non-delta profile. profs = append(profs, deltaProf) p.cfg.statsd.Timing("datadog.profiler.go.delta_time", end.Sub(deltaStart), tags, 1) } diff --git a/profiler/profile_test.go b/profiler/profile_test.go index c8978f7975..7c14422c92 100644 --- a/profiler/profile_test.go +++ b/profiler/profile_test.go @@ -139,23 +139,19 @@ main;bar 0 0 8 16 // additional profile type to ease the transition) profs, err := p.runProfile(profType) require.NoError(t, err) - require.Equal(t, 2, len(profs)) - require.Equal(t, profType.Filename(), profs[0].name) + require.Equal(t, 1, len(profs)) + require.Equal(t, "delta-"+profType.Filename(), profs[0].name) require.Equal(t, prof1, profs[0].data) - require.Equal(t, "delta-"+profType.Filename(), profs[1].name) - require.Equal(t, prof1, profs[1].data) // second run, should produce p1 profile and delta profile profs, err = p.runProfile(profType) require.NoError(t, err) - require.Equal(t, 2, len(profs)) - require.Equal(t, profType.Filename(), profs[0].name) - require.Equal(t, prof2, profs[0].data) - require.Equal(t, "delta-"+profType.Filename(), profs[1].name) - require.Equal(t, test.WantDelta.String(), protobufToText(profs[1].data)) + require.Equal(t, 1, len(profs)) + require.Equal(t, "delta-"+profType.Filename(), profs[0].name) + require.Equal(t, test.WantDelta.String(), protobufToText(profs[0].data)) // check delta prof details like timestamps and duration - deltaProf, err := pprofile.ParseData(profs[1].data) + deltaProf, err := pprofile.ParseData(profs[0].data) require.NoError(t, err) require.Equal(t, test.Prof2.Time.UnixNano(), deltaProf.TimeNanos) require.Equal(t, deltaPeriod.Nanoseconds(), deltaProf.DurationNanos) diff --git a/profiler/profiler_test.go b/profiler/profiler_test.go index 1974784e27..2f4535edca 100644 --- a/profiler/profiler_test.go +++ b/profiler/profiler_test.go @@ -265,8 +265,8 @@ func TestProfilerInternal(t *testing.T) { assert.EqualValues(1, startCPU) assert.EqualValues(1, stopCPU) - // should contain cpu.pprof, metrics.json, heap.pprof, delta-heap.pprof - assert.Equal(4, len(bat.profiles)) + // should contain cpu.pprof, metrics.json, delta-heap.pprof + assert.Equal(3, len(bat.profiles)) p.exit <- struct{}{} <-wait @@ -319,11 +319,10 @@ func TestProfilerPassthrough(t *testing.T) { } assert := assert.New(t) - // should contain cpu.pprof, heap.pprof, delta-heap.pprof - assert.Equal(3, len(bat.profiles)) + // should contain cpu.pprof, delta-heap.pprof + assert.Equal(2, len(bat.profiles)) assert.NotEmpty(bat.profiles[0].data) assert.NotEmpty(bat.profiles[1].data) - assert.NotEmpty(bat.profiles[2].data) } func unstartedProfiler(opts ...Option) (*profiler, error) { From fd3d644efdbdc484ce036d94e476ef0116f6a337 Mon Sep 17 00:00:00 2001 From: Nick Ripley Date: Tue, 1 Mar 2022 08:08:30 -0500 Subject: [PATCH 2/2] profiler: make delta profile check logic more clean Removed a redundant condition (deltaProfile is documented to return nil if delta profiles are disabled) and changed the order of conditions to match the default configuration (detla profiles first). --- profiler/profile.go | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/profiler/profile.go b/profiler/profile.go index a3d82e07a1..5fc140fadb 100644 --- a/profiler/profile.go +++ b/profiler/profile.go @@ -223,18 +223,17 @@ func (p *profiler) runProfile(pt ProfileType) ([]*profile, error) { end := now() tags := append(p.cfg.tags, pt.Tag()) var profs []*profile - if !p.cfg.deltaProfiles || deltaProf == nil { + if deltaProf != nil { + profs = append(profs, deltaProf) + p.cfg.statsd.Timing("datadog.profiler.go.delta_time", end.Sub(deltaStart), tags, 1) + } else { + // If the user has disabled delta profiles, or the profile type + // doesn't support delta profiles (like the CPU profile) then + // send the original profile unchanged. profs = append(profs, &profile{ name: t.Filename, data: data, }) - } else { - // If the user has enabled delta profiles, then the delta - // profile is all we need for the backend and we can save - // significant storage and bandwidth by not including the full, - // non-delta profile. - profs = append(profs, deltaProf) - p.cfg.statsd.Timing("datadog.profiler.go.delta_time", end.Sub(deltaStart), tags, 1) } p.cfg.statsd.Timing("datadog.profiler.go.collect_time", end.Sub(start), tags, 1) return profs, nil