dd-trace-py icon indicating copy to clipboard operation
dd-trace-py copied to clipboard

chore(wrapt): remove all references to pkg_resources

Open mabdinur opened this issue 1 year ago • 5 comments

Resolves: https://github.com/DataDog/dd-trace-py/issues/7918

Checklist

  • [ ] Change(s) are motivated and described in the PR description.
  • [ ] Testing strategy is described if automated tests are not included in the PR.
  • [ ] Risk is outlined (performance impact, potential for breakage, maintainability, etc).
  • [ ] Change is maintainable (easy to change, telemetry, documentation).
  • [ ] Library release note guidelines are followed. If no release note is required, add label changelog/no-changelog.
  • [ ] Documentation is included (in-code, generated user docs, public corp docs).
  • [ ] Backport labels are set (if applicable)

Reviewer Checklist

  • [ ] Title is accurate.
  • [ ] No unnecessary changes are introduced.
  • [ ] Description motivates each change.
  • [ ] Avoids breaking API changes unless absolutely necessary.
  • [ ] Testing strategy adequately addresses listed risk(s).
  • [ ] Change is maintainable (easy to change, telemetry, documentation).
  • [ ] Release note makes sense to a user of the library.
  • [ ] Reviewer has explicitly 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
  • [ ] If this PR 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.
  • [ ] This PR doesn't touch any of that.

mabdinur avatar Dec 18 '23 20:12 mabdinur

Benchmarks

Benchmark execution time: 2024-01-05 16:41:31

Comparing candidate commit dfb5b604a3a8632c6ec4d8f690a8d703cc27109c in PR branch munir/fix-pkg-resources-deprecations with baseline commit 7862889fb4db1841df0ab25f89b71ef387f35a53 in branch main.

Found 13 performance improvements and 25 performance regressions! Performance is the same for 157 metrics, 9 unstable metrics.

scenario:coreapiscenario-context_with_data_listeners

  • 🟩 max_rss_usage [-825.409KB; -673.318KB] or [-2.808%; -2.290%]

scenario:coreapiscenario-context_with_data_listeners_and_all_listeners

  • 🟥 max_rss_usage [+645.169KB; +807.682KB] or [+2.251%; +2.818%]

scenario:coreapiscenario-context_with_data_no_listeners

  • 🟥 max_rss_usage [+675.695KB; +849.246KB] or [+2.360%; +2.966%]

scenario:coreapiscenario-core_dispatch_no_listeners

  • 🟩 max_rss_usage [-794.350KB; -648.671KB] or [-2.705%; -2.209%]

scenario:coreapiscenario-core_dispatch_only_all_listeners

  • 🟥 max_rss_usage [+595.746KB; +754.705KB] or [+2.079%; +2.634%]

scenario:coreapiscenario-core_dispatch_with_results_listeners_and_all_listeners

  • 🟥 max_rss_usage [+592.239KB; +769.272KB] or [+2.064%; +2.682%]

scenario:coreapiscenario-core_dispatch_with_results_only_all_listeners

  • 🟥 max_rss_usage [+680.811KB; +836.757KB] or [+2.377%; +2.921%]

scenario:coreapiscenario-get_item_exists

  • 🟥 max_rss_usage [+646.424KB; +791.272KB] or [+2.258%; +2.764%]

scenario:coreapiscenario-get_item_missing

  • 🟥 max_rss_usage [+631.986KB; +772.942KB] or [+2.204%; +2.696%]

scenario:httppropagationextract-all_styles_all_headers

  • 🟥 max_rss_usage [+647.959KB; +849.129KB] or [+2.270%; +2.975%]

scenario:httppropagationextract-empty_headers

  • 🟥 max_rss_usage [+697.958KB; +868.352KB] or [+2.443%; +3.039%]

scenario:httppropagationextract-full_t_id_datadog_headers

  • 🟩 max_rss_usage [-859.453KB; -641.731KB] or [-2.931%; -2.188%]

scenario:httppropagationextract-invalid_span_id_header

  • 🟥 max_rss_usage [+626.193KB; +837.718KB] or [+2.191%; +2.931%]

scenario:httppropagationextract-invalid_trace_id_header

  • 🟩 max_rss_usage [-792.415KB; -632.583KB] or [-2.708%; -2.162%]

scenario:httppropagationextract-large_valid_headers_all

  • 🟥 max_rss_usage [+590.328KB; +813.372KB] or [+2.062%; +2.841%]

scenario:httppropagationextract-medium_valid_headers_all

  • 🟥 max_rss_usage [+698.793KB; +880.624KB] or [+2.445%; +3.081%]

scenario:httppropagationextract-none_propagation_style

  • 🟥 max_rss_usage [+618.360KB; +815.240KB] or [+2.164%; +2.854%]

scenario:httppropagationextract-valid_headers_all

  • 🟩 max_rss_usage [-945.132KB; -733.409KB] or [-3.228%; -2.505%]

scenario:httppropagationextract-wsgi_invalid_span_id_header

  • 🟥 max_rss_usage [+817.851KB; +1025.349KB] or [+2.883%; +3.614%]

scenario:httppropagationextract-wsgi_invalid_trace_id_header

  • 🟩 max_rss_usage [-935.147KB; -751.177KB] or [-3.187%; -2.560%]

scenario:httppropagationextract-wsgi_valid_headers_all

  • 🟥 max_rss_usage [+693.160KB; +892.402KB] or [+2.428%; +3.126%]

scenario:httppropagationinject-ids_only

  • 🟥 max_rss_usage [+720.824KB; +847.124KB] or [+2.528%; +2.971%]

scenario:httppropagationinject-with_all

  • 🟩 max_rss_usage [-776.584KB; -626.706KB] or [-2.652%; -2.140%]

scenario:httppropagationinject-with_dd_origin

  • 🟩 max_rss_usage [-926.915KB; -798.730KB] or [-3.158%; -2.721%]

scenario:httppropagationinject-with_priority_and_origin

  • 🟩 max_rss_usage [-739.138KB; -610.903KB] or [-2.523%; -2.085%]

scenario:httppropagationinject-with_tags

  • 🟩 max_rss_usage [-775.307KB; -645.595KB] or [-2.645%; -2.202%]

scenario:httppropagationinject-with_tags_max_size

  • 🟩 max_rss_usage [-789.955KB; -659.210KB] or [-2.696%; -2.250%]

scenario:otelspan-start-finish

  • 🟥 max_rss_usage [+656.055KB; +833.660KB] or [+2.202%; +2.798%]

scenario:sethttpmeta-all-disabled

  • 🟥 max_rss_usage [+807.754KB; +928.131KB] or [+2.501%; +2.873%]

scenario:sethttpmeta-all-enabled

  • 🟥 max_rss_usage [+746.640KB; +852.848KB] or [+2.311%; +2.640%]

scenario:sethttpmeta-obfuscation-send-querystring-disabled

  • 🟩 max_rss_usage [-909.404KB; -808.048KB] or [-2.758%; -2.450%]

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

  • 🟩 max_rss_usage [-948.219KB; -838.047KB] or [-2.873%; -2.539%]

scenario:sethttpmeta-useragentvariant_exists_2

  • 🟥 max_rss_usage [+677.581KB; +767.898KB] or [+2.092%; +2.370%]

scenario:sethttpmeta-useragentvariant_exists_3

  • 🟥 max_rss_usage [+696.096KB; +820.653KB] or [+2.148%; +2.532%]

scenario:sethttpmeta-useragentvariant_not_exists_2

  • 🟥 max_rss_usage [+790.271KB; +913.665KB] or [+2.449%; +2.831%]

scenario:span-add-metrics

  • 🟥 max_rss_usage [+22.000MB; +22.141MB] or [+54.312%; +54.660%]

scenario:span-start-finish-telemetry

  • 🟥 max_rss_usage [+641.762KB; +760.709KB] or [+2.246%; +2.662%]

scenario:span-start-finish-traceid128

  • 🟥 max_rss_usage [+607.526KB; +705.651KB] or [+2.122%; +2.465%]

pr-commenter[bot] avatar Dec 19 '23 02:12 pr-commenter[bot]

Has this been resolved in wrapt itself? Can we resolve it by vendoring a new version, or even adding wrapt as a dependency?

majorgreys avatar Dec 20 '23 16:12 majorgreys

Has this been resolved in wrapt itself? Can we resolve it by vendoring a new version, or even adding wrapt as a dependency?

Nope: https://github.com/GrahamDumpleton/wrapt/blob/develop/src/wrapt/importer.py#L94. We could make the upstream contribution, release a new version then vendor it but that seems like a pain.

mabdinur avatar Dec 21 '23 13:12 mabdinur

@mabdinur I see your point. We can make this change here and also contribute upstream.

We'll want to run all tests given this change.

majorgreys avatar Jan 02 '24 14:01 majorgreys

We may need to bump pybind11: https://github.com/pybind/pybind11/pull/4941

mabdinur avatar Jan 04 '24 22:01 mabdinur