Skip to content

chore(profiling): add lz4 to profile test suite#13499

Merged
taegyunkim merged 2 commits into
mainfrom
taegyunkim/prof-11194-lz4
May 23, 2025
Merged

chore(profiling): add lz4 to profile test suite#13499
taegyunkim merged 2 commits into
mainfrom
taegyunkim/prof-11194-lz4

Conversation

@taegyunkim

@taegyunkim taegyunkim commented May 23, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

To be used in #13464 to decompress pprof files, split into a separate PR to reduce the diffs

Checklist

  • PR author has checked that all the criteria below are met
  • The PR description includes an overview of the change
  • The PR description articulates the motivation for the change
  • The change includes tests OR the PR description describes a testing strategy
  • The PR description notes risks associated with the change, if any
  • Newly-added code is easy to change
  • The change follows the library release note guidelines
  • The change includes or references documentation updates if necessary
  • Backport labels are set (if applicable)

Reviewer Checklist

  • Reviewer has checked that all the criteria below are met
  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Newly-added code is easy to change
  • Release note makes sense to a user of the library
  • If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
  • Backport labels are set in a manner that is consistent with the release branch maintenance policy

@github-actions

github-actions Bot commented May 23, 2025

Copy link
Copy Markdown
Contributor

CODEOWNERS have been resolved as:

.riot/requirements/383825b.txt                                          @DataDog/apm-python
.riot/requirements/5e497cb.txt                                          @DataDog/apm-python
.riot/requirements/d15c0f8.txt                                          @DataDog/apm-python
.riot/requirements/dc3342c.txt                                          @DataDog/apm-python
.riot/requirements/f31bf6b.txt                                          @DataDog/apm-python
riotfile.py                                                             @DataDog/apm-python
.riot/requirements/11c2039.txt                                          @DataDog/apm-python
.riot/requirements/1671e93.txt                                          @DataDog/apm-python
.riot/requirements/1907fd8.txt                                          @DataDog/apm-python
.riot/requirements/1959911.txt                                          @DataDog/apm-python
.riot/requirements/1a8fa20.txt                                          @DataDog/apm-python
.riot/requirements/1b558c8.txt                                          @DataDog/apm-python
.riot/requirements/1ee2a7f.txt                                          @DataDog/apm-python
.riot/requirements/3fab567.txt                                          @DataDog/apm-python
.riot/requirements/43f8115.txt                                          @DataDog/apm-python
.riot/requirements/47bee20.txt                                          @DataDog/apm-python
.riot/requirements/5a33d0b.txt                                          @DataDog/apm-python
.riot/requirements/8d9a826.txt                                          @DataDog/apm-python
.riot/requirements/9a319c8.txt                                          @DataDog/apm-python
.riot/requirements/b64548a.txt                                          @DataDog/apm-python

@taegyunkim taegyunkim changed the title Taegyunkim/prof 11194 lz4 chore(profiling): add lz4 to profile test suite May 23, 2025
@taegyunkim taegyunkim added Profiling Continous Profling changelog/no-changelog A changelog entry is not required for this PR. labels May 23, 2025
@taegyunkim
taegyunkim marked this pull request as ready for review May 23, 2025 19:38
@taegyunkim
taegyunkim requested a review from a team as a code owner May 23, 2025 19:38
@taegyunkim
taegyunkim requested review from P403n1x87 and nsrip-dd May 23, 2025 19:38
@github-actions

github-actions Bot commented May 23, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Bootstrap import analysis

Comparison of import times between this PR and base.

Summary

The average import time from this PR is: 231 ± 3 ms.

The average import time from base is: 235 ± 3 ms.

The import time difference between this PR and base is: -4.0 ± 0.1 ms.

Import time breakdown

The following import paths have shrunk:

ddtrace.auto 2.191 ms (0.95%)
ddtrace.bootstrap.sitecustomize 1.537 ms (0.66%)
ddtrace.bootstrap.preload 1.537 ms (0.66%)
ddtrace.internal.remoteconfig.client 0.721 ms (0.31%)
ddtrace 0.654 ms (0.28%)

nsrip-dd
nsrip-dd approved these changes May 23, 2025
View reviewed changes

@nsrip-dd nsrip-dd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Though I'm curious where it's actually used? I didn't see lz4 mentioned in the PR you linked, but maybe I didn't look hard enough

@taegyunkim

taegyunkim commented May 23, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@nsrip-dd it's used to parse pprof files in this function

dd-trace-py/tests/profiling/collector/pprof_utils.py

Lines 115 to 124 in 35ecfee

def parse_profile(filename_prefix: str):
files = glob.glob(filename_prefix + ".*")
files.sort()
filename = files[-1]
with lz4.frame.open(filename) as fp:
serialized_data = fp.read()
profile = pprof_pb2.Profile()
profile.ParseFromString(serialized_data)
assert len(profile.sample) > 0, "No samples found in profile"
return profile

And only profile-v2 had been using this function to check the pprof file

@taegyunkim
taegyunkim enabled auto-merge (squash) May 23, 2025 20:17
@pr-commenter

pr-commenter Bot commented May 23, 2025

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2025-05-23 20:22:33

Comparing candidate commit 5d9e20b in PR branch taegyunkim/prof-11194-lz4 with baseline commit 4e8a947 in branch main.

Found 0 performance improvements and 6 performance regressions! Performance is the same for 506 metrics, 8 unstable metrics.

scenario:iast_aspects-format_map_aspect

  • 🟥 execution_time [+404.768ns; +518.970ns] or [+12.499%; +16.025%]

scenario:iast_aspects-lstrip_aspect

  • 🟥 execution_time [+1.626µs; +1.724µs] or [+12.577%; +13.338%]

scenario:iast_aspects-rstrip_aspect

  • 🟥 execution_time [+1.394µs; +1.656µs] or [+10.875%; +12.923%]

scenario:iast_aspects-split_aspect

  • 🟥 execution_time [+171.249ns; +203.138ns] or [+11.834%; +14.037%]

scenario:iast_aspects-swapcase_aspect

  • 🟥 execution_time [+337.497ns; +374.521ns] or [+13.827%; +15.343%]

scenario:iast_aspects-title_aspect

  • 🟥 execution_time [+266.572ns; +314.323ns] or [+11.149%; +13.146%]

@taegyunkim
taegyunkim merged commit 9f6c5ee into main May 23, 2025
@taegyunkim
taegyunkim deleted the taegyunkim/prof-11194-lz4 branch May 23, 2025 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@P403n1x87 P403n1x87 Awaiting requested review from P403n1x87 P403n1x87 is a code owner automatically assigned from DataDog/apm-python
1 more reviewer
@nsrip-dd nsrip-dd nsrip-dd approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

changelog/no-changelog A changelog entry is not required for this PR. Profiling Continous Profling

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@taegyunkim @nsrip-dd