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

fix(integration config): copy and deepcopy implementations #1381

Merged
merged 2 commits into from
Apr 29, 2020

Conversation

Kyle-Verhoog
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog commented Apr 25, 2020

IntegrationConfig has a broken implementation of deepcopy and a non-existent/broken implementation for copy since it falls back to using dict.copy.

This PR fixes the bug in the deepcopy implementation and adds an implementation for copy as well as some basic tests.

@Kyle-Verhoog Kyle-Verhoog requested a review from a team as a code owner April 25, 2020 03:57
Copy link
Contributor

@jd jd left a comment

Choose a reason for hiding this comment

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

WFM I hope we have a use case for copy if we provide it?

@Kyle-Verhoog
Copy link
Member Author

👍 @jd yes, we do. It's actually broken right now to use our config instances with the Pin because it does a copy.

dd-trace-py/ddtrace/pin.py

Lines 163 to 176 in 1dd0d14

@debtcollector.removals.removed_kwarg("app_type")
def clone(self, service=None, app=None, app_type=None, tags=None, tracer=None):
"""Return a clone of the pin with the given attributes replaced."""
# do a shallow copy of Pin dicts
if not tags and self.tags:
tags = self.tags.copy()
# we use a copy instead of a deepcopy because we expect configurations
# to have only a root level dictionary without nested objects. Using
# deepcopy introduces a big overhead:
#
# copy: 0.00654911994934082
# deepcopy: 0.2787208557128906
config = self._config.copy()

@Kyle-Verhoog Kyle-Verhoog merged commit 6a14a9b into master Apr 29, 2020
@Kyle-Verhoog Kyle-Verhoog deleted the kylev/integration-config-fixes branch April 29, 2020 19:38
@Kyle-Verhoog Kyle-Verhoog added this to the 0.38.0 milestone May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants