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

chore(profiling): native extensions are no longer optional on supported platforms #9169

Merged
merged 10 commits into from
Jun 20, 2024

Conversation

sanchda
Copy link
Contributor

@sanchda sanchda commented May 6, 2024

This patch changes the affinity for certain profiling-related extensions. They were marked as optional during our vetting process, but now should be built everywhere. This isn't a problem for CI. Serverless currently strips these components out post-build, and that's still OK.

Things this PR changes

  • Whether or not builds for native profiling components can fail setup.py. These builds are already attempted every time (and succeed, as far as I know, whenever they're specified in our CI)

Things this PR does not change

  • Shipped artifacts (these artifacts already ship in our CI)
  • Loaded modules (module load is still deferred until the user requests an optional feature)
  • Defaults (users still have to request that they want to use this stuff, explicitly)

Risks

  • May break local builds in environments where a C++ compiler is not available. We now require a Rust toolchain to build core components--is it too much to also expect a C++ (compiler plus cmake) toolchain?

Checklist

  • Change(s) are motivated and described in the PR description
  • Testing strategy is described if automated tests are not included in the PR
  • Risks are described (performance impact, potential for breakage, maintainability)
  • Change is maintainable (easy to change, telemetry, documentation)
  • Library release note guidelines are followed or label changelog/no-changelog is set
  • Documentation is included (in-code, generated user docs, public corp docs)
  • Backport labels are set (if applicable)
  • If this PR changes the public interface, I've notified @DataDog/apm-tees.

Reviewer Checklist

  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Description motivates each change
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Change is maintainable (easy to change, telemetry, documentation)
  • Release note makes sense to a user of the library
  • 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

@datadog-dd-trace-py-rkomorn
Copy link

Datadog Report

Branch report: sanchda/less_permissive_builds
Commit report: 4dcd235
Test service: dd-trace-py

✅ 0 Failed, 173837 Passed, 1149 Skipped, 11h 25m 1.6s Total duration (8m 4s time saved)
⌛ 2 Performance Regressions

⌛ Performance Regressions vs Default Branch (2)

  • test_file_delete[False] - test_openai_v1.py 3.07s (+2.56s, +510%) - Details
  • test_export_server_down - test_http.py 5.82s (+4.79s, +464%) - Details

@codecov-commenter
Copy link

codecov-commenter commented May 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 10.27%. Comparing base (cd92beb) to head (7c6fde2).
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #9169       +/-   ##
===========================================
- Coverage   75.89%   10.27%   -65.62%     
===========================================
  Files        1315     1285       -30     
  Lines      124966   123127     -1839     
===========================================
- Hits        94837    12650    -82187     
- Misses      30129   110477    +80348     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@datadog-dd-trace-py-rkomorn
Copy link

datadog-dd-trace-py-rkomorn bot commented Jun 5, 2024

Datadog Report

Branch report: sanchda/less_permissive_builds
Commit report: 77ec84e
Test service: dd-trace-py

✅ 0 Failed, 176637 Passed, 1270 Skipped, 11h 45m 40.03s Total duration (19m 45.74s time saved)

@pr-commenter
Copy link

pr-commenter bot commented Jun 5, 2024

Benchmarks

Benchmark execution time: 2024-06-10 22:46:28

Comparing candidate commit 7c6fde2 in PR branch sanchda/less_permissive_builds with baseline commit 360b469 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 221 metrics, 9 unstable metrics.

@sanchda sanchda added the changelog/no-changelog A changelog entry is not required for this PR. label Jun 5, 2024
@sanchda sanchda marked this pull request as ready for review June 5, 2024 19:21
@sanchda sanchda requested a review from a team as a code owner June 5, 2024 19:21
Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

lgtm

we already require a C++ compiler anyways, afaik?

@sanchda
Copy link
Contributor Author

sanchda commented Jun 7, 2024

we already require a C++ compiler anyways, afaik?

Yeah, and we pull in cmake through our deps, so AFAICT the net difference to the required build platform for the overwhelming majority of users is zero.

@sanchda sanchda enabled auto-merge (squash) June 8, 2024 03:54
@sanchda sanchda merged commit 0b1e1ce into main Jun 20, 2024
162 checks passed
@sanchda sanchda deleted the sanchda/less_permissive_builds branch June 20, 2024 16:33
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants