fix(profiling): default to libdatadog if autoinject/SSI is enabled #8822

sanchda · 2024-04-01T18:14:45Z

In injected configurations, the Profiler can cause deployed (bundled? stashed?) dependencies to over-ride the dependencies of the underlying application. This is especially problematic for the protobuf library.

This PR changes several things

Introduces a new "required" option to the libdatadog (libdd) collector/exporter. Normally, if the user merely enables libdatadog, the profiler will make a best effort at using it, but fallback to the old collector/exporter if needed. When libdatadog is required, profiling will be disabled if libdatadog cannot be used.
Has lib-inection set libdatadog to "required"

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.
If change touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.

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

pr-commenter · 2024-04-01T19:24:08Z

Benchmarks

Benchmark execution time: 2024-04-02 18:23:54

Comparing candidate commit adba36d in PR branch sanchda/permissive_protobuf with baseline commit 405625e in branch main.

Found 8 performance improvements and 12 performance regressions! Performance is the same for 181 metrics, 9 unstable metrics.

scenario:coreapiscenario-core_dispatch_with_results_no_listeners

🟥 max_rss_usage [+683.708KB; +782.660KB] or [+3.226%; +3.693%]

scenario:httppropagationextract-all_styles_all_headers

🟩 max_rss_usage [-769.269KB; -700.785KB] or [-3.513%; -3.200%]

scenario:httppropagationextract-datadog_tracecontext_tracestate_propagated_on_trace_id_match

🟥 max_rss_usage [+668.666KB; +741.177KB] or [+3.159%; +3.502%]

scenario:httppropagationextract-invalid_trace_id_header

🟥 max_rss_usage [+627.563KB; +797.435KB] or [+2.967%; +3.770%]

scenario:httppropagationextract-none_propagation_style

🟥 max_rss_usage [+571.507KB; +937.460KB] or [+2.722%; +4.465%]

scenario:httppropagationextract-valid_headers_basic

🟩 max_rss_usage [-790.678KB; -718.698KB] or [-3.613%; -3.284%]

scenario:httppropagationextract-wsgi_valid_headers_basic

🟩 max_rss_usage [-748.318KB; -448.124KB] or [-3.423%; -2.050%]

scenario:httppropagationinject-with_tags_max_size

🟥 max_rss_usage [+735.826KB; +782.151KB] or [+3.488%; +3.708%]

scenario:otelspan-start-finish

🟩 max_rss_usage [-723.482KB; -641.305KB] or [-3.129%; -2.774%]

scenario:otelspan-start-finish-telemetry

🟩 max_rss_usage [-734.587KB; -671.569KB] or [-3.170%; -2.898%]

scenario:samplingrules-low_match

🟩 max_rss_usage [-1170.288KB; -822.416KB] or [-4.157%; -2.921%]

scenario:sethttpmeta-all-disabled

🟥 max_rss_usage [+618.126KB; +785.164KB] or [+2.879%; +3.657%]

scenario:sethttpmeta-all-enabled

🟥 max_rss_usage [+705.438KB; +782.639KB] or [+3.290%; +3.650%]

scenario:sethttpmeta-collectipvariant_exists

🟥 max_rss_usage [+580.435KB; +787.220KB] or [+2.697%; +3.658%]

scenario:sethttpmeta-no-useragentvariant

🟥 max_rss_usage [+506.975KB; +782.855KB] or [+2.349%; +3.627%]

scenario:sethttpmeta-obfuscation-worst-case-explicit-query

🟩 max_rss_usage [-758.812KB; -682.570KB] or [-3.384%; -3.044%]

scenario:span-add-metrics

🟥 execution_time [+2.642ms; +4.822ms] or [+3.118%; +5.690%]

scenario:span-add-tags

🟥 max_rss_usage [+1.872MB; +1.964MB] or [+6.198%; +6.501%]

scenario:span-start

🟥 max_rss_usage [+1.013MB; +1.167MB] or [+2.467%; +2.841%]

scenario:span-start-finish

🟩 max_rss_usage [-606.384KB; -531.894KB] or [-2.763%; -2.424%]

…race-py into sanchda/permissive_protobuf

ddtrace/profiling/scheduler.py

lib-injection/sitecustomize.py

releasenotes/notes/profiling-fix-protobuf-injection-d94f7c152681795c.yaml

…81795c.yaml Co-authored-by: Gabriele N. Tornetta <P403n1x87@users.noreply.github.com>

P403n1x87

LGTM!

ddtrace/settings/injection.py

Co-authored-by: Emmett Butler <723615+emmettbutler@users.noreply.github.com>

Kyle-Verhoog

Overall looks great. I think I would defer the detection of injection for now since we need to establish this consistently across all the libraries and instead rely on the control flow we have already to enable the functionality.

ddtrace/settings/injection.py

lib-injection/sitecustomize.py

Kyle-Verhoog

🚢 great stuff @sanchda!

sanchda · 2024-04-02T17:22:08Z

@Kyle-Verhoog and I had an offline discussion, where we agreed that protobuf actually needs to be stripped out of the inject Docker image and deb/rpm releases. I made a best effort to do this, although I wanted to err on the side of caution and only remove the protobuf-related components rather than everything under google/*. This leaves some empty directories right now.

/datadog-init $ find . -name '*protobuf*'
/datadog-init $ find . -name '*_upb*'
/datadog-init $ find . -name '*google*'
./ddtrace_pkgs/site-packages-ddtrace-py3.10-manylinux2014/google
./ddtrace_pkgs/site-packages-ddtrace-py3.8-manylinux2014/google
./ddtrace_pkgs/site-packages-ddtrace-py3.7-musllinux_1_1/google
./ddtrace_pkgs/site-packages-ddtrace-py3.9-musllinux_1_1/google
./ddtrace_pkgs/site-packages-ddtrace-py3.12-manylinux2014/google
./ddtrace_pkgs/site-packages-ddtrace-py3.9-manylinux2014/google
./ddtrace_pkgs/site-packages-ddtrace-py3.11-manylinux2014/google
./ddtrace_pkgs/site-packages-ddtrace-py3.10-musllinux_1_1/google
./ddtrace_pkgs/site-packages-ddtrace-py3.7-manylinux2014/google
./ddtrace_pkgs/site-packages-ddtrace-py3.12-musllinux_1_1/google
./ddtrace_pkgs/site-packages-ddtrace-py3.11-musllinux_1_1/google
./ddtrace_pkgs/site-packages-ddtrace-py3.8-musllinux_1_1/google

datadog-dd-trace-py-rkomorn · 2024-04-02T18:57:15Z

Datadog Report

Branch report: sanchda/permissive_protobuf
Commit report: c6425e5
Test service: dd-trace-py

✅ 0 Failed, 369 Passed, 681 Skipped, 19m 23.49s Total duration (1h 4m 53.78s time saved)
❄️ 1 New Flaky

New Flaky Tests (1)

test_iter_events_multi_thread - test_memalloc.py - Last Failure
Expand for error
```
 assert 923 >= 1000
```

…8822) In injected configurations, the Profiler can cause deployed (bundled? stashed?) dependencies to over-ride the dependencies of the underlying application. This is especially problematic for the `protobuf` library. This PR changes several things * Introduces a new "required" option to the libdatadog (libdd) collector/exporter. Normally, if the user merely enables libdatadog, the profiler will make a best effort at using it, but fallback to the old collector/exporter if needed. When libdatadog is required, profiling will be disabled if libdatadog cannot be used. * Has lib-inection set libdatadog to "required" ## Checklist - [X] Change(s) are motivated and described in the PR description - [X] Testing strategy is described if automated tests are not included in the PR - [X] Risks are described (performance impact, potential for breakage, maintainability) - [X] Change is maintainable (easy to change, telemetry, documentation) - [X] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed or label `changelog/no-changelog` is set - [X] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)) - [X] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) - [x] If this PR changes the public interface, I've notified `@DataDog/apm-tees`. - [X] If change touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from `@DataDog/security-design-and-guidance`. ## Reviewer Checklist - [x] Title is accurate - [x] All changes are related to the pull request's stated goal - [x] Description motivates each change - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [x] Testing strategy adequately addresses listed risks - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] Release note makes sense to a user of the library - [x] Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - [x] 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) --------- Co-authored-by: sanchda <sanchda@users.noreply.github.com> Co-authored-by: Gabriele N. Tornetta <P403n1x87@users.noreply.github.com> Co-authored-by: Emmett Butler <723615+emmettbutler@users.noreply.github.com> (cherry picked from commit 1c1f54a)

…ackport 2.7] (#8834) Backport 1c1f54a from #8822 to 2.7. In injected configurations, the Profiler can cause deployed (bundled? stashed?) dependencies to over-ride the dependencies of the underlying application. This is especially problematic for the `protobuf` library. This PR changes several things * Introduces a new "required" option to the libdatadog (libdd) collector/exporter. Normally, if the user merely enables libdatadog, the profiler will make a best effort at using it, but fallback to the old collector/exporter if needed. When libdatadog is required, profiling will be disabled if libdatadog cannot be used. * Has lib-inection set libdatadog to "required" ## Checklist - [X] Change(s) are motivated and described in the PR description - [X] Testing strategy is described if automated tests are not included in the PR - [X] Risks are described (performance impact, potential for breakage, maintainability) - [X] Change is maintainable (easy to change, telemetry, documentation) - [X] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed or label `changelog/no-changelog` is set - [X] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)) - [X] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) - [x] If this PR changes the public interface, I've notified `@DataDog/apm-tees`. - [X] If change touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from `@DataDog/security-design-and-guidance`. ## Reviewer Checklist - [x] Title is accurate - [x] All changes are related to the pull request's stated goal - [x] Description motivates each change - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [x] Testing strategy adequately addresses listed risks - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] Release note makes sense to a user of the library - [x] Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - [x] 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) Co-authored-by: David Sanchez <838104+sanchda@users.noreply.github.com>

CI: Changes some test assert that started failing since #8822 ## Checklist - [x] Change(s) are motivated and described in the PR description - [x] Testing strategy is described if automated tests are not included in the PR - [x] Risks are described (performance impact, potential for breakage, maintainability) - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed or label `changelog/no-changelog` is set - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)) - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) - [x] If this PR changes the public interface, I've notified `@DataDog/apm-tees`. - [x] If change touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from `@DataDog/security-design-and-guidance`. ## Reviewer Checklist - [x] Title is accurate - [x] All changes are related to the pull request's stated goal - [x] Description motivates each change - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [x] Testing strategy adequately addresses listed risks - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] Release note makes sense to a user of the library - [x] Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - [x] 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)

CI: Changes some test assert that started failing since #8822 ## Checklist - [x] Change(s) are motivated and described in the PR description - [x] Testing strategy is described if automated tests are not included in the PR - [x] Risks are described (performance impact, potential for breakage, maintainability) - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed or label `changelog/no-changelog` is set - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)) - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) - [x] If this PR changes the public interface, I've notified `@DataDog/apm-tees`. - [x] If change touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from `@DataDog/security-design-and-guidance`. ## Reviewer Checklist - [x] Title is accurate - [x] All changes are related to the pull request's stated goal - [x] Description motivates each change - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [x] Testing strategy adequately addresses listed risks - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] Release note makes sense to a user of the library - [x] Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - [x] 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) (cherry picked from commit b085205)

Backport b085205 from #8843 to 2.7. CI: Changes some test assert that started failing since #8822 ## Checklist - [x] Change(s) are motivated and described in the PR description - [x] Testing strategy is described if automated tests are not included in the PR - [x] Risks are described (performance impact, potential for breakage, maintainability) - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed or label `changelog/no-changelog` is set - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)) - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) - [x] If this PR changes the public interface, I've notified `@DataDog/apm-tees`. - [x] If change touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from `@DataDog/security-design-and-guidance`. ## Reviewer Checklist - [x] Title is accurate - [x] All changes are related to the pull request's stated goal - [x] Description motivates each change - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [x] Testing strategy adequately addresses listed risks - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] Release note makes sense to a user of the library - [x] Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - [x] 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) Co-authored-by: Federico Mon <federico.mon@datadoghq.com>

sanchda and others added 3 commits April 1, 2024 17:22

Add preliminary support for auto-instrument in profiling

c82c549

Suppress more imports

1ccdca8

Merge branch 'main' into sanchda/permissive_protobuf

e56b475

sanchda and others added 7 commits April 1, 2024 15:08

Merge branch 'main' into sanchda/permissive_protobuf

d08aa1e

Style

b4b20ab

Fix typo

5faf29f

More fixups

9cfb4cc

Merge branch 'main' into sanchda/permissive_protobuf

5eaf6cb

Fix wrong config

31860fc

Merge branch 'sanchda/permissive_protobuf' of github.com:DataDog/dd-t…

38214b8

…race-py into sanchda/permissive_protobuf

P403n1x87 reviewed Apr 2, 2024

View reviewed changes

ddtrace/profiling/scheduler.py Outdated Show resolved Hide resolved

sanchda added 3 commits April 2, 2024 13:01

Make types safe again

f671a35

Fix config whoopsie

c6b69ad

Fix import

970e6f9

sanchda marked this pull request as ready for review April 2, 2024 13:26

sanchda requested review from a team as code owners April 2, 2024 13:26

sanchda requested review from emmettbutler and erikayasuda April 2, 2024 13:26

sanchda commented Apr 2, 2024

View reviewed changes

lib-injection/sitecustomize.py Outdated Show resolved Hide resolved

Merge branch 'main' into sanchda/permissive_protobuf

2baa9d4

P403n1x87 reviewed Apr 2, 2024

View reviewed changes

releasenotes/notes/profiling-fix-protobuf-injection-d94f7c152681795c.yaml Outdated Show resolved Hide resolved

Update releasenotes/notes/profiling-fix-protobuf-injection-d94f7c1526…

3670665

…81795c.yaml Co-authored-by: Gabriele N. Tornetta <P403n1x87@users.noreply.github.com>

P403n1x87 approved these changes Apr 2, 2024

View reviewed changes

sanchda enabled auto-merge (squash) April 2, 2024 14:48

sanchda disabled auto-merge April 2, 2024 14:51

sanchda enabled auto-merge (squash) April 2, 2024 14:52

emmettbutler approved these changes Apr 2, 2024

View reviewed changes

ddtrace/settings/injection.py Outdated Show resolved Hide resolved

emmettbutler changed the title ~~chore(profiling): default to libdatadog if autoinject/SSI is enabled~~ fix(profiling): default to libdatadog if autoinject/SSI is enabled Apr 2, 2024

emmettbutler added the backport 2.7 label Apr 2, 2024

sanchda and others added 2 commits April 2, 2024 10:01

Update ddtrace/settings/injection.py

d2acd08

Co-authored-by: Emmett Butler <723615+emmettbutler@users.noreply.github.com>

Merge branch 'main' into sanchda/permissive_protobuf

0ea2159

sanchda disabled auto-merge April 2, 2024 15:02

sanchda enabled auto-merge (squash) April 2, 2024 15:02

Kyle-Verhoog reviewed Apr 2, 2024

View reviewed changes

ddtrace/settings/injection.py Outdated Show resolved Hide resolved

lib-injection/sitecustomize.py Outdated Show resolved Hide resolved

Replace DD_INJECTION_ENABLED with a direct requirement on profiling

4ef8cbf

sanchda disabled auto-merge April 2, 2024 15:51

sanchda enabled auto-merge (squash) April 2, 2024 15:51

Kyle-Verhoog approved these changes Apr 2, 2024

View reviewed changes

Best effort to cleanup protobuf stuff

adba36d

emmettbutler and others added 4 commits April 2, 2024 10:59

lint

7dc51f6

Merge branch 'main' into sanchda/permissive_protobuf

69ca350

Merge branch 'main' into sanchda/permissive_protobuf

dcb8025

lint

c6425e5

sanchda merged commit 1c1f54a into main Apr 2, 2024
83 of 86 checks passed

sanchda deleted the sanchda/permissive_protobuf branch April 2, 2024 19:17

github-actions bot mentioned this pull request Apr 2, 2024

fix(profiling): default to libdatadog if autoinject/SSI is enabled [backport 2.7] #8834

Merged

18 tasks

gnufede mentioned this pull request Apr 3, 2024

ci: fix telemetry test #8843

Merged

18 tasks

github-actions bot mentioned this pull request Apr 3, 2024

ci: fix telemetry test [backport 2.7] #8847

Merged

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(profiling): default to libdatadog if autoinject/SSI is enabled #8822

fix(profiling): default to libdatadog if autoinject/SSI is enabled #8822

sanchda commented Apr 1, 2024 •

edited

Loading

pr-commenter bot commented Apr 1, 2024 •

edited

Loading

P403n1x87 left a comment

Kyle-Verhoog left a comment

Kyle-Verhoog left a comment

sanchda commented Apr 2, 2024

datadog-dd-trace-py-rkomorn bot commented Apr 2, 2024

fix(profiling): default to libdatadog if autoinject/SSI is enabled #8822

fix(profiling): default to libdatadog if autoinject/SSI is enabled #8822

Conversation

sanchda commented Apr 1, 2024 • edited Loading

Checklist

Reviewer Checklist

pr-commenter bot commented Apr 1, 2024 • edited Loading

Benchmarks

scenario:coreapiscenario-core_dispatch_with_results_no_listeners

scenario:httppropagationextract-all_styles_all_headers

scenario:httppropagationextract-datadog_tracecontext_tracestate_propagated_on_trace_id_match

scenario:httppropagationextract-invalid_trace_id_header

scenario:httppropagationextract-none_propagation_style

scenario:httppropagationextract-valid_headers_basic

scenario:httppropagationextract-wsgi_valid_headers_basic

scenario:httppropagationinject-with_tags_max_size

scenario:otelspan-start-finish

scenario:otelspan-start-finish-telemetry

scenario:samplingrules-low_match

scenario:sethttpmeta-all-disabled

scenario:sethttpmeta-all-enabled

scenario:sethttpmeta-collectipvariant_exists

scenario:sethttpmeta-no-useragentvariant

scenario:sethttpmeta-obfuscation-worst-case-explicit-query

scenario:span-add-metrics

scenario:span-add-tags

scenario:span-start

scenario:span-start-finish

P403n1x87 left a comment

Choose a reason for hiding this comment

Kyle-Verhoog left a comment

Choose a reason for hiding this comment

Kyle-Verhoog left a comment

Choose a reason for hiding this comment

sanchda commented Apr 2, 2024

datadog-dd-trace-py-rkomorn bot commented Apr 2, 2024

Datadog Report

New Flaky Tests (1)

sanchda commented Apr 1, 2024 •

edited

Loading

pr-commenter bot commented Apr 1, 2024 •

edited

Loading