Skip to content

chore(tracer): add failsafe to package distribution discovery #13343

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

chore(tracer): add failsafe to package distribution discovery #13343

wants to merge 3 commits into from

Conversation

christophe-papazian
Copy link
Contributor

@christophe-papazian christophe-papazian commented May 7, 2025 •
edited
Loading

Add two failsafes to package distribution discovery.
If internal API fails, fall back to vendored version
If vendored version fails, return empty package distribution.

#APMAPI-1400

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

@christophe-papazian christophe-papazian added the changelog/no-changelog A changelog entry is not required for this PR. label May 7, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 7, 2025

CODEOWNERS have been resolved as:

ddtrace/internal/packages.py                                            @DataDog/apm-core-python

@christophe-papazian christophe-papazian changed the title chore(tracer) add failsafe to package distribution discovery chore(tracer): add failsafe to package distribution discovery May 7, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 7, 2025 •
edited
Loading

Bootstrap import analysis

Comparison of import times between this PR and base.

Summary

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

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

The import time difference between this PR and base is: -2.08 ± 0.06 ms.

Import time breakdown

The following import paths have shrunk:

ddtrace.auto 2.033 ms (0.88%)
ddtrace.bootstrap.sitecustomize 1.330 ms (0.58%)
ddtrace.bootstrap.preload 1.330 ms (0.58%)
ddtrace.internal.remoteconfig.client 0.649 ms (0.28%)
ddtrace 0.702 ms (0.30%)
ddtrace._logger 0.030 ms (0.01%)
logging 0.030 ms (0.01%)
traceback 0.030 ms (0.01%)
contextlib 0.030 ms (0.01%)

@pr-commenter
Copy link

pr-commenter bot commented May 7, 2025 •
edited
Loading

Benchmarks

Benchmark execution time: 2025-05-14 13:48:19

Comparing candidate commit b51d496 in PR branch christophe-papazian/APMAPI-1400 with baseline commit 01bb3a5 in branch main.

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

scenario:iast_aspects-ospathsplitext_aspect

  • 🟥 execution_time [+709.437ns; +822.507ns] or [+15.489%; +17.958%]

scenario:iast_aspects-strip_aspect

  • 🟥 execution_time [+1.229µs; +1.434µs] or [+9.488%; +11.070%]

scenario:telemetryaddmetric-1-gauge-metric-1-times

  • 🟥 execution_time [+153.421ns; +229.578ns] or [+7.175%; +10.737%]

@christophe-papazian christophe-papazian marked this pull request as ready for review May 7, 2025 09:02
@christophe-papazian christophe-papazian requested a review from a team as a code owner May 7, 2025 09:02
brettlangdon
brettlangdon reviewed May 7, 2025
View reviewed changes
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.

just some initial thoughts, I think in general it looks ok, but if we can narrow some of the try/excepts so we can get at least partial data when possible.

ddtrace/internal/packages.py
@@ -46,15 +46,19 @@ def get_distributions() -> t.Mapping[str, str]:
def get_package_distributions() -> t.Mapping[str, t.List[str]]:
"""a mapping of importable package names to their distribution name(s)"""
global _PACKAGE_DISTRIBUTIONS
if _PACKAGE_DISTRIBUTIONS is None:
if not _PACKAGE_DISTRIBUTIONS:
Copy link
Member

Choose a reason for hiding this comment

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

I suppose it should never be "empty" so this should be safe?

it would be weird for it to be empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it can be empty if the previous call failed

ddtrace/internal/packages.py
_PACKAGE_DISTRIBUTIONS = importlib_metadata.packages_distributions()
try:
_PACKAGE_DISTRIBUTIONS = importlib_metadata.packages_distributions()
except Exception:
Copy link
Member

Choose a reason for hiding this comment

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

with this, you could probably just drop the hasattr, but 🤷🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's related to the issue #APMAPI-1400 where in some circonstances on system tests we get an OSError.

ddtrace/internal/packages.py
for pkg in _top_level_declared(dist) or _top_level_inferred(dist):
pkg_to_dist[pkg].append(dist.metadata["Name"])
return dict(pkg_to_dist)
except Exception:
Copy link
Member

Choose a reason for hiding this comment

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

where is the exception happening? should we try to be more targeted?

e.g. just skip specific dists if an exception occurs?

for dist in importlib_metadata.distributions():
    try:
        pass
    except Exception:
        pass
return dict(pkg_to_dist)

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just added that to be extra careful here, we don't want unchecked exception here, I suppose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brettlangdon brettlangdon brettlangdon left review comments

@juanjux juanjux Awaiting requested review from juanjux juanjux is a code owner automatically assigned from DataDog/apm-core-python

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@christophe-papazian @brettlangdon