chore: add a registry for supported integrations #13215
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
scripts/integration_registry/_update_integration_registry_versions.py
Outdated
Show resolved
Hide resolved
tests/contrib/integration_registry/test_external_dependencies.py
Outdated
Show resolved
Hide resolved
tests/contrib/integration_registry/test_external_dependencies.py
Outdated
Show resolved
Hide resolved
tests/contrib/integration_registry/test_external_dependencies.py
Outdated
Show resolved
Hide resolved
scripts/integration_registry/_update_integration_registry_versions.py
Outdated
Show resolved
Hide resolved
Bootstrap import analysisComparison of import times between this PR and base. SummaryThe average import time from this PR is: 233 ± 2 ms. The average import time from base is: 235 ± 2 ms. The import time difference between this PR and base is: -1.9 ± 0.1 ms. Import time breakdownThe following import paths have shrunk:
|
BenchmarksBenchmark execution time: 2025-04-24 21:32:37 Comparing candidate commit f174820 in PR branch Found 0 performance improvements and 5 performance regressions! Performance is the same for 491 metrics, 4 unstable metrics. scenario:iast_aspects-format_map_aspect
scenario:iast_aspects-modulo_aspect_for_bytes_bytearray
scenario:iast_aspects-replace_aspect
scenario:iast_aspects-swapcase_aspect
scenario:telemetryaddmetric-1-distribution-metric-1-times
|
tests/contrib/integration_registry/registry_update_helpers/integration_registry_manager.py
Outdated
Show resolved
Hide resolved
tests/contrib/integration_registry/registry_update_helpers/update_orchestrator.py
Outdated
Show resolved
Hide resolved
tests/contrib/integration_registry/registry_update_helpers/update_orchestrator.py
Outdated
Show resolved
Hide resolved
tests/contrib/integration_registry/registry_update_helpers/integration_update_orchestrator.py
Show resolved
Hide resolved
wconti27
changed the title
wconti27
requested review from
erikayasuda,
daniel-romano-DD,
Yun-Kim and
wantsui
wantsui
approved these changes
Apr 23, 2025
Comment on lines
+175
to
+183
| # UNCOMMENT TO RUN THE UPDATER LOCALLY, and comment out the above few lines, requires updating riotfile.py | ||
| # to include the following within the riot env: | ||
| # - "filelock" | ||
| # - "pyyaml" | ||
| # from tests.contrib.integration_registry.registry_update_helpers.integration_registry_updater import ( | ||
| # IntegrationRegistryUpdater | ||
| # ) | ||
| # updater = IntegrationRegistryUpdater() | ||
| # updater_succeeded = updater.run(data_file_path) |
There was a problem hiding this comment.
nit: Is it possible to have an environment variable for running it locally, that applies these changes?
There was a problem hiding this comment.
No, because uncommenting this requires the user to add pyyaml and filelock to the riot dependencies for the venv suite they are running.
There was a problem hiding this comment.
Maybe call this out in the README if we can't make it an env var flag?
erikayasuda
approved these changes
Apr 24, 2025
Comment on lines
+238
to
+241
| elif ( | ||
| package in DEPENDENCY_TO_INTEGRATION_MAPPING | ||
| and DEPENDENCY_TO_INTEGRATION_MAPPING[package] == integration | ||
| ): |
There was a problem hiding this comment.
Check if this is redundant with the previous check?
Comment on lines
112
to
116
| min_final = min_v if min_v and min_v != "N/A" else None | ||
| max_final = max_v if max_v and max_v != "N/A" else None | ||
| if min_final is None and max_final is None: | ||
| return None | ||
| return {"min": min_final or "N/A", "max": max_final or "N/A"} |
There was a problem hiding this comment.
nit:
Suggested change
| min_final = min_v if min_v and min_v != "N/A" else None | |
| max_final = max_v if max_v and max_v != "N/A" else None | |
| if min_final is None and max_final is None: | |
| return None | |
| return {"min": min_final or "N/A", "max": max_final or "N/A"} | |
| min_final = min_v if min_v and min_v != "N/A" else "N/A" | |
| max_final = max_v if max_v and max_v != "N/A" else "N/A" | |
| if min_final is "N/A" and max_final is "N/A": | |
| return None | |
| return {"min": min_final, "max": max_final} |
Comment on lines
153
to
154
| for entry in current_integrations: | ||
| if not isinstance(entry, dict): |
There was a problem hiding this comment.
Maybe refactor so it's less nested?
|
|
||
| @pytest.fixture(scope="module") | ||
| def riot_venv_names() -> set[str]: | ||
| """Recursively finds all Venv names defined in riotfile.py.""" |
There was a problem hiding this comment.
nit:
Suggested change
| """Recursively finds all Venv names defined in riotfile.py.""" | |
| """Finds all Venv names defined in riotfile.py.""" |
| - '@core' | ||
| - '@contrib' | ||
| - '@tracing' | ||
| - '@integration_registry' |
There was a problem hiding this comment.
I don't think you added this above?
| * Runs [`scripts/integration_registry/_update_integration_registry_versions.py`](../../../scripts/integration_registry/_update_integration_registry_versions.py) to update the registry | ||
| * Formats the registry YAML for consistency | ||
|
|
||
| * NOTE: Manual script update does not guarantee that newly added patched dependencies (such as for a new integration) will be added to the registry.yaml. A full test run is required for this process. |
There was a problem hiding this comment.
I'm slightly confused by this. So running the script manually would only take the existing registry and update its values, but won't recognize that the riotfile has been updated with new tests/integrations?
Comment on lines
+135
to
+137
| if entry.get("is_external_package") is True: | ||
| integration_name = entry.get("integration_name", "UNKNOWN_ENTRY") | ||
| dependency_names = entry.get("dependency_name", []) |
There was a problem hiding this comment.
nit:
Suggested change
| if entry.get("is_external_package") is True: | |
| integration_name = entry.get("integration_name", "UNKNOWN_ENTRY") | |
| dependency_names = entry.get("dependency_name", []) | |
| if not entry.get("is_external_package"): | |
| continue | |
| integration_name = entry.get("integration_name", "UNKNOWN_ENTRY") | |
| dependency_names = entry.get("dependency_name", []) | |
| ... |
Comment on lines
+150
to
+155
| if not isinstance(dep_name, str) or not dep_name: | ||
| errors.append( | ||
| f"External integration '{integration_name}' has invalid item in dependency_name list: " | ||
| f"{dep_name}" | ||
| ) | ||
| continue |
There was a problem hiding this comment.
Is this not guaranteed by the schema you're defining? 🤔
Comment on lines
+175
to
+183
| # UNCOMMENT TO RUN THE UPDATER LOCALLY, and comment out the above few lines, requires updating riotfile.py | ||
| # to include the following within the riot env: | ||
| # - "filelock" | ||
| # - "pyyaml" | ||
| # from tests.contrib.integration_registry.registry_update_helpers.integration_registry_updater import ( | ||
| # IntegrationRegistryUpdater | ||
| # ) | ||
| # updater = IntegrationRegistryUpdater() | ||
| # updater_succeeded = updater.run(data_file_path) |
There was a problem hiding this comment.
Maybe call this out in the README if we can't make it an env var flag?
| contrib directory. | ||
| """ | ||
|
|
||
| UNTESTED_INTEGRATIONS = ["asynctest", "dbapi", "dbapi_async", "integration_registry", "gunicorn"] |
There was a problem hiding this comment.
nit: Should we just have these in some kind of const.py file so you only have to update one place?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR introduces an integration registry, that contains information for all supported integrations.
registry.ymllocated inddtrace/contrib/integration_registry/Falsefor stdlib Python packages or abstract integrations such as dbapiaiorediswhich is untested and planned to be deprecatedminandmaxversions for what is testedAlong with the
registry.yaml, this PR makes quite a few changes to setup the necessary information to connect Riot testing venvs, the dependencies in those tests, to the integration being tested.riotfile.pyupdates:integration_name:sub-testdjango_hosts->django:django_hostspsycopg2(our integration name is psycopg) ->psycopg:psycopg2graphene->graphql:grapheneddtrace/contrib/integration_registry/mappings.pyscripts/freshvenvs.pyupdates.riot/requirements/*.txtto the respective test environment name. This change ensures that dependency versions collected are actually correlated with the test suite venv being tested (as opposed totest suite Xrelying ondependency Y==0.1, whiletest suite YtestsdependencyY>1.0, previously the tested version ofYwould be recorded as0.1, now its>1.0.ddtrace/integration_registry/mappings.py, instead of storing the mapping in this script fileAutomatic updates of integration
registry.ymlfor contrib testingtests/contrib/*, we patch thegetattrfunction and listen forgetattrcalls where_datadog_patchis being called on an object. This call is used in most of our contribs, and allows us access to the modules being patched during tests. If this attribute was used in thegetattrcall, we store the object it was called on, as well as the callstack, for later processing.registry.yamlwith data collected from the patched object and traceback.registry.yamlupdate process during testing:IntegrationRegistryManagerclass, which later processes the objects, checks if they were valid contribs, gets the dependency name of the patched top level module, ie:module kafka -> (using importlib.metadata) -> we get package names producing this module -> ie:confluent-kafka``.- After
IntegrationRegistryManagerruns, we run theIntegrationRegistryUpdaterclass in a separate virtual environment, since the class relies onriotandpyyaml, both of which are not available in riot virtual environments, so we make our own.-
IntegrationRegistryUpdater:IntegrationRegistryManager, which includes tested integration, patched dependency name, patched dependency versionregistry.yml, and decides whether to update the file, depending on if the dependency / integration combo is already present in the file, or if the tested version is outside the current files version range for the dependency, ifTrue, runsscripts/integration_registry/update_and_format_registry.py.New Scripting:
scripts/integration_registry/_format_integration_registry.py: Formats the integration registry with spaces between integration registries, since no python yaml packages offer this functionality. Done for readabilityscripts/integration_registry/_update_integration_registry_versions.py: Updates the registry with new data fromsupported_versions_output.json, which is generated from thescripts/freshvenvs.pyscript.scripts/integration_registry/update_and_format_registry.py: Runsscripts/freshvenvs.py, and the former two scripts to update all of our test information and resave the new registry. This is the main script to call.Other updates:
registry.ymlwas updated during CI, the new check fails the CI job, similar to riot lockfile check, and tells the user to update theregistry.ymlwith the new information.Several tests have been added to ensure that the
registry.yamlstays updated when new contribs are added:Registry Tests
ddtrace/contrib/internalhas an associated registry entryregistry.yamlconforms to strict schemaexternaldependency has corresponding tested version information inregistry.yamlexternaldependency exists on PypiRiotfile Tests
ddtrace/contrib/internalhas a corresponding riot venv name associated with ittests/contrib/uses a venv name that is a directory withinddtrace/contrib/internal, if the venv name is a subtest in the form ofintegration[sub-test], tests that theintegrationis located withinddtrace/contrib/internal.Checklist
Reviewer Checklist