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

[core] Update config to allow configuration before patching #650

Merged
merged 3 commits into from
Oct 16, 2018

Conversation

brettlangdon
Copy link
Member

@brettlangdon brettlangdon commented Oct 15, 2018

Previously if we tried to set a config setting before the contrib module was imported
we would receive an error that the integration key did not exist.

With this new approach we are allowing any integration setting to be configured by
the user, whenever they want and then when we call config._add() in the contrib
module, we will merge the settings with any existing settings, keeping those that
already exist.

We have also added a Config.__repr__

Previously if we tried to set a config setting before the contrib module was imported
we would receive an error that the integration key did not exist.

With this new approach we are allowing any integration setting to be configured by
the user, whenever they want and then when we call `config._add()` in the contrib
module, we will merge the settings with any existing settings, keeping those that
already exist.

We have also added a Config.__repr__
@brettlangdon
Copy link
Member Author

An explanation of the current problem is probably good too...

If we tried to do:

import ddtrace
ddtrace.patch_all(requests=True)

ddtrace.config.requests['split_by_domain'] = True

We would receive an exception, because ddtrace.config._config['requests'] would not exist until we call import requests for the first time.

With these changes, the above will work, and when we import requests the existing settings we have set will remain and not be overwritten by config._add() that we call in ddtrace.contrib.requests.patch.

@labbati labbati self-requested a review October 16, 2018 08:18
Copy link
Member

@labbati labbati left a comment

Choose a reason for hiding this comment

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

Just added a few comments for typos. Other than that looks great!

tests/test_global_config.py Outdated Show resolved Hide resolved
tests/test_global_config.py Outdated Show resolved Hide resolved
tests/test_global_config.py Outdated Show resolved Hide resolved
palazzem
palazzem previously approved these changes Oct 16, 2018
Copy link

@palazzem palazzem left a comment

Choose a reason for hiding this comment

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

Approach is fine and I prefer to keep this API. Also _add is internal so we can revisit it after if we hit a road blocker when migrating all integrations to the Config API.

Kyle-Verhoog
Kyle-Verhoog previously approved these changes Oct 16, 2018
tests/test_global_config.py Show resolved Hide resolved
),
),
))
eq_(self.config.requests['a']['b']['c'], True)
Copy link
Member

Choose a reason for hiding this comment

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

👍

@brettlangdon
Copy link
Member Author

CI is green, Github rate limited CircleCI again 🙄

Copy link
Member

@labbati labbati left a comment

Choose a reason for hiding this comment

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

LGTM. Really liked this PR, the problem it was trying to solve appeared very clear in the comments and code is clear. ❤️

@brettlangdon brettlangdon merged commit 1c5a3ba into master Oct 16, 2018
@brettlangdon brettlangdon deleted the brettlangdon/config.available.at.startup branch November 20, 2018 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants