Skip to content

Conversation

@taegyunkim
Copy link
Contributor

@taegyunkim taegyunkim commented May 20, 2025

A necessary step to remove

  1. Legacy pprof/http exporters
  2. Stack v1

These test files were removed as they were mostly there to test Legacy pprof exporter code, and we have the same tests in profile-v2 suite, and they don't trigger stack collector at all.

  • tests/profiling/collector/test_asyncio.py: AsyncioLockCollector tests
  • tests/profiling/collector/test_threading.py: ThreadingLockCollector tests
  • tests/profiling/collector/test_threading_asyncio.py: ThreadingLockCollector tests with asyncio

Rest of the test files modified

  1. to test with libdatadog profiler exporter if possible
  2. to remove references to Recorder class.

I'll follow up with another PR removing legacy pprof/http exporters

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
Copy link
Contributor

github-actions bot commented May 20, 2025

CODEOWNERS have been resolved as:

riotfile.py                                                             @DataDog/apm-python
tests/profiling/_test_multiprocessing.py                                @DataDog/profiling-python
tests/profiling/collector/test_memalloc.py                              @DataDog/profiling-python
tests/profiling/collector/test_stack.py                                 @DataDog/profiling-python
tests/profiling/collector/test_stack_asyncio.py                         @DataDog/profiling-python
tests/profiling/simple_program.py                                       @DataDog/profiling-python
tests/profiling/simple_program_fork.py                                  @DataDog/profiling-python
tests/profiling/simple_program_gevent.py                                @DataDog/profiling-python
tests/profiling/test_accuracy.py                                        @DataDog/profiling-python
tests/profiling/test_gunicorn.py                                        @DataDog/profiling-python
tests/profiling/test_main.py                                            @DataDog/profiling-python
tests/profiling/test_profiler.py                                        @DataDog/profiling-python
tests/profiling/test_uwsgi.py                                           @DataDog/profiling-python
tests/profiling/utils.py                                                @DataDog/profiling-python
tests/profiling_v2/test_main.py                                         @DataDog/profiling-python

@github-actions
Copy link
Contributor

github-actions bot commented May 20, 2025

Bootstrap import analysis

Comparison of import times between this PR and base.

Summary

The average import time from this PR is: 237 ± 2 ms.

The average import time from base is: 240 ± 2 ms.

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

Import time breakdown

The following import paths have shrunk:

ddtrace.auto 2.078 ms (0.88%)
ddtrace.bootstrap.sitecustomize 1.405 ms (0.59%)
ddtrace.bootstrap.preload 1.405 ms (0.59%)
ddtrace.internal.remoteconfig.client 0.662 ms (0.28%)
ddtrace 0.673 ms (0.28%)
ddtrace.internal._unpatched 0.024 ms (0.01%)

@pr-commenter
Copy link

pr-commenter bot commented May 20, 2025

Benchmarks

Benchmark execution time: 2025-06-04 01:58:45

Comparing candidate commit 3aac25c in PR branch taegyunkim/prof-11194-v1-libdd with baseline commit 12791bc in branch main.

Found 0 performance improvements and 1 performance regressions! Performance is the same for 504 metrics, 3 unstable metrics.

scenario:iastaspects-rstrip_aspect

  • 🟥 execution_time [+1.383µs; +1.577µs] or [+10.871%; +12.390%]

taegyunkim added a commit that referenced this pull request May 23, 2025
To be used in #13464 to decompress pprof files, split into a separate PR
to reduce the diffs

## Checklist
- [x] 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](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## 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](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
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](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
@taegyunkim taegyunkim marked this pull request as ready for review May 23, 2025 20:52
@taegyunkim taegyunkim requested review from a team as code owners May 23, 2025 20:52
@taegyunkim taegyunkim requested review from ZStriker19 and erikayasuda and removed request for a team May 23, 2025 20:52
@taegyunkim taegyunkim added Profiling Continous Profling changelog/no-changelog A changelog entry is not required for this PR. labels May 23, 2025
@taegyunkim taegyunkim changed the base branch from taegyunkim/prof-10736-v2-test to main June 3, 2025 21:56
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.

In general, seems reasonable. To be transparent, I haven't closely inspected everything since this is a pretty big PR. I basically checked that we have similar coverage either with your modified tests or in the v2 tests for stuff you deleted. Let me know if there's anything you think needs more scrutiny.

@taegyunkim
Copy link
Contributor Author

taegyunkim commented Jun 4, 2025

@nsrip-dd @P403n1x87
Again, this is an interim step before we completely remove stack v1, and v2 is enabled default for a while. So I'll merge this and follow up with removing legacy exporters and v1.

Thanks again for reviews!

@taegyunkim taegyunkim merged commit 3b47ae7 into main Jun 4, 2025
740 of 742 checks passed
@taegyunkim taegyunkim deleted the taegyunkim/prof-11194-v1-libdd branch June 4, 2025 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants