refactor(profiler): centralize compression logic by felixge · Pull Request #3529 · DataDog/dd-trace-go

felixge · 2025-05-18T12:32:52Z

For historical reasons, the profiler has used varying levels of
compression for different profile types (no compression, gzip-1 and
gzip-6). This has been identified as target for cost optimization,
see PROF-11815.

Refactor the compression related code in order to pave the path for
introducing a new compression strategy, which will most likely be based
on zstd.

This patch should not have any observable impact. Profiles are expected
to be uploaded with exactly the same compression settings as before, and
the performance is expected to stay exactly the same as well.

Introduce a dedicated compressor for each profile type that handles
converting the profile data produced by it from the existing compression
(gzip-1 or no compression) to the desired output compression.

Update delta profiles and expGoroutineWaitProfile to perform their
gzip-6 compression in the compressor for their profile type, rather than
doing it themselves.

A few TestRunProfile tests had to be modified because they were not
explicitly declaring the profile types they were testing when
initializing the profiler, which caused panics due to their compressors
being uninitialized.

Reviewer's Checklist

Changed code has unit tests for its functionality at or near 100% coverage.
System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
There is a benchmark for any new code, or changes to existing code.
If this interacts with the agent in a new way, a system test has been added.
New code is free of linting errors. You can check this by running golangci-lint run locally.
Add an appropriate team label so this PR gets put in the right place for the release notes.
Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

pr-commenter · 2025-05-18T13:46:13Z

Benchmarks

Benchmark execution time: 2025-05-19 18:34:33

Comparing candidate commit 442e89c in PR branch push-onwnwvzqkxzn with baseline commit 1153ba4 in branch main.

Found 5 performance improvements and 1 performance regressions! Performance is the same for 49 metrics, 1 unstable metrics.

scenario:BenchmarkInjectW3C-24

🟥 execution_time [+132.654ns; +167.946ns] or [+3.239%; +4.101%]

scenario:BenchmarkSetTagString-24

🟩 execution_time [-8.002ns; -4.138ns] or [-6.629%; -3.428%]

scenario:BenchmarkSetTagStringPtr-24

🟩 execution_time [-7.996ns; -4.764ns] or [-4.363%; -2.599%]

scenario:BenchmarkSingleSpanRetention/with-rules/match-all-24

🟩 execution_time [-10.009µs; -8.896µs] or [-3.622%; -3.219%]

scenario:BenchmarkSingleSpanRetention/with-rules/match-half-24

🟩 execution_time [-10.294µs; -8.956µs] or [-3.722%; -3.238%]

scenario:BenchmarkStartSpan-24

🟩 execution_time [-119.498ns; -94.302ns] or [-4.748%; -3.747%]

kakkoyun

LGTM 🚀

kakkoyun · 2025-05-19T08:50:24Z

+		compressors: make(map[ProfileType]compressor),
+	}
+	types := slices.Collect(maps.Keys(cfg.types))
+	// We need to manually add executionTrace to the list of profile types to be


darccio

It looks great. I'm curious about the performance impact by code layout changes that we seem to have in the benchmarks.

felixge · 2025-05-19T11:55:48Z

It looks great. I'm curious about the performance impact by code layout changes that we seem to have in the benchmarks.

Yeah, so I raised #3530 to check if there is general flakiness here, but I wasn't able to reproduce results like the ones above, so I suspect that we're indeed seeing code layout changes here.

Which ... is pretty annoying 😞. It means we can't really use benchmark platform to spot smaller regression. Note: This isn't the fault of the platform, and it's great that it raises awareness for this. But I'm starting to think we need some code layout randomization for Go if we want to be serious about continuous benchmarking 🤔.

nsrip-dd

LGTM. I tested this out with a toy app and confirmed that the profiles make it through intact and I can view them in the UI.

I'm curious to see how you'll implement recompression. I managed to bash something together (https://go.dev/play/p/YeHeuCgfUjl) that'd fit the current interface. If I recall from past experience, going through io.Pipe can be kinda slow. So definitely factor that in when testing out recompression if you take that approach.

nsrip-dd · 2025-05-19T14:34:10Z


 func TestRunProfile(t *testing.T) {
+	// TODO(felixge): These tests are directly calling the internal runProfile()
+	// function which is brittle. We should refactor them to use the public API.


I'm trying to remember why I didn't already delete this test... I'm happy to look into it

For historical reasons, the profiler has used varying levels of compression for different profile types (no compression, gzip-1 and gzip-6). This has been identified as target for cost optimization, see PROF-11815. Refactor the compression related code in order to pave the path for introducing a new compression strategy, which will most likely be based on zstd. This patch should not have any observable impact. Profiles are expected to be uploaded with exactly the same compression settings as before, and the performance is expected to stay exactly the same as well. Introduce a dedicated compressor for each profile type that handles converting the profile data produced by it from the existing compression (gzip-1 or no compression) to the desired output compression. Update delta profiles and expGoroutineWaitProfile to perform their gzip-6 compression in the compressor for their profile type, rather than doing it themselves. A few TestRunProfile tests had to be modified because they were not explicitly declaring the profile types they were testing when initializing the profiler, which caused panics due to their compressors being uninitialized.

Our non-delta heap, block, and mutex profiles are getting double-compressed. That is, they are gzip-compressed, and if you decompress them then you get another gzip-compressed blob, which is in turn a compressed pprof file. This is due to a bug introduced by #3529. In that PR we reworked our compression logic in order to support re-compression (into zstd in particular). We incorrectly decided whether a given profile type is a delta profile by just checking whether there are delta values for the profile, without checking whether delta profiling is actually enabled. As a result, when delta profiling is disabled we use the delta profling _enabled_ logic, where we assume the input data is uncomprossed and then gzip-compress it.

Our non-delta heap, block, and mutex profiles are getting double-compressed. That is, they are gzip-compressed, and if you decompress them then you get another gzip-compressed blob, which is in turn a compressed pprof file. This is due to a bug introduced by #3529. In that PR we reworked our compression logic in order to support re-compression (into zstd in particular). We incorrectly decided whether a given profile type is a delta profile by just checking whether there are delta values for the profile type, without checking whether delta profiling is actually enabled. As a result, when delta profiling is disabled we use the delta profling enabled logic, where we assume the input data is uncomprossed and then gzip-compress it.

felixge force-pushed the push-onwnwvzqkxzn branch 4 times, most recently from d674907 to 6eed203 Compare May 18, 2025 13:07

felixge force-pushed the push-onwnwvzqkxzn branch from 6eed203 to 65bef4d Compare May 18, 2025 16:43

felixge added the profiler label May 18, 2025

felixge force-pushed the push-onwnwvzqkxzn branch 4 times, most recently from 3c9c178 to fc4682f Compare May 18, 2025 18:48

felixge requested a review from nsrip-dd May 19, 2025 07:57

felixge marked this pull request as ready for review May 19, 2025 07:57

felixge requested review from a team as code owners May 19, 2025 07:57

kakkoyun approved these changes May 19, 2025

View reviewed changes

darccio approved these changes May 19, 2025

View reviewed changes

nsrip-dd approved these changes May 19, 2025

View reviewed changes

felixge force-pushed the push-onwnwvzqkxzn branch from fc4682f to 442e89c Compare May 19, 2025 18:04

felixge enabled auto-merge (squash) May 19, 2025 18:05

felixge merged commit e7a40ff into main May 20, 2025

felixge deleted the push-onwnwvzqkxzn branch May 20, 2025 10:17

nsrip-dd mentioned this pull request Aug 26, 2025

fix(profiler): don't double compress non-delta profiles #3918

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(profiler): centralize compression logic#3529

refactor(profiler): centralize compression logic#3529
felixge merged 1 commit into
mainfrom
push-onwnwvzqkxzn

felixge commented May 18, 2025 •

edited

Loading

Uh oh!

pr-commenter Bot commented May 18, 2025 •

edited

Loading

Uh oh!

kakkoyun left a comment

Uh oh!

kakkoyun May 19, 2025

Uh oh!

darccio left a comment

Uh oh!

felixge commented May 19, 2025

Uh oh!

nsrip-dd left a comment

Uh oh!

nsrip-dd May 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

felixge commented May 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer's Checklist

Uh oh!

pr-commenter Bot commented May 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

scenario:BenchmarkInjectW3C-24

scenario:BenchmarkSetTagString-24

scenario:BenchmarkSetTagStringPtr-24

scenario:BenchmarkSingleSpanRetention/with-rules/match-all-24

scenario:BenchmarkSingleSpanRetention/with-rules/match-half-24

scenario:BenchmarkStartSpan-24

Uh oh!

kakkoyun left a comment

Choose a reason for hiding this comment

Uh oh!

kakkoyun May 19, 2025

Choose a reason for hiding this comment

Uh oh!

darccio left a comment

Choose a reason for hiding this comment

Uh oh!

felixge commented May 19, 2025

Uh oh!

nsrip-dd left a comment

Choose a reason for hiding this comment

Uh oh!

nsrip-dd May 19, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

felixge commented May 18, 2025 •

edited

Loading

pr-commenter Bot commented May 18, 2025 •

edited

Loading