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

Add #add to configuration resolver #940

Merged
merged 3 commits into from
Feb 5, 2020
Merged

Conversation

delner
Copy link
Contributor

@delner delner commented Feb 4, 2020

To support the expansion of "multiplexing" (having multiple configurations per integration) and its applications in #882 and #861/#937, this pull request refactors the Datadog::Contrib::Configuration::Resolver and adds an #add function to it, to allow for certain use cases to operate more efficiently. See linked issues/PRs for example cases.

This was originally a change introduced in #882 by @stormsilver, but extracted it out as a base for other PRs as well.

@delner delner added integrations Involves tracing integrations community Was opened by a community member labels Feb 4, 2020
@delner delner requested review from marcotc and a team February 4, 2020 21:39
@delner delner self-assigned this Feb 4, 2020
@delner delner force-pushed the refactor/configuration_resolver branch from b8b40c6 to 05e68a6 Compare February 4, 2020 21:40
@@ -9,6 +9,16 @@ module Configuration
class Resolver < Contrib::Configuration::Resolver
def initialize(configurations = nil)
@configurations = configurations
@well_known_keys = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be renamed to @resolved_keys.
Would you see any interest in using concurrent ruby data structures here?

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 like that name; I'll update, thanks!

As much as we'd like to, we can't use any libraries (including concurrent-ruby) as they may conflict with user's own copies of these gems. However if you have a custom resolver in your own application or integration, you're welcome to use it.

marcotc
marcotc previously approved these changes Feb 5, 2020
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

👍

@delner
Copy link
Contributor Author

delner commented Feb 5, 2020

Did some refactoring to Configurable here as well.... wanted it to read a little bit more cleanly, and removed the behavior for auto-instantiating default settings everytime there's a configuration cache miss. (This is now done more explicitly as a part of #add_configuration.)

Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

:shipit:

@delner delner merged commit 0c39fec into master Feb 5, 2020
@delner delner deleted the refactor/configuration_resolver branch February 5, 2020 22:36
@delner delner added this to the 0.33.0 milestone Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Was opened by a community member integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants