Add Datadog::Registry
#200
Conversation
p-lambert
force-pushed
the
pedro/create-module-registry
branch
from
2cce8ca to
fc6e980
Compare
palazzem
added
core
|
Just as a note, a draft of the overall API is available here: https://gist.github.com/p-lambert/fd817fb19b8236e26d40eaca421cc40f The idea is to provide this core API and to migrate one of our integrations so that it can be used as a template. |
p-lambert
force-pushed
the
pedro/create-module-registry
branch
6 times, most recently
from
8534170 to
e179839
Compare
|
I like this |
p-lambert
force-pushed
the
pedro/create-module-registry
branch
from
e179839 to
f0b6e2e
Compare
p-lambert
force-pushed
the
pedro/create-module-registry
branch
from
f0b6e2e to
79941df
Compare
lib/ddtrace.rb
Outdated
| end | ||
|
|
||
| require 'ddtrace/monkey' |
There was a problem hiding this comment.
there is a reason why it has been moved here and not at the top of the file?
There was a problem hiding this comment.
Yes. the problem is, Monkey is loading all integrations, and during their load time they're registering themselves in the Datadog.registry. So by the time monkey is required, we need Datadog.registry defined. Hence the move.
Once we have time I'd like to improve our startup process, so we rely less on ordering etc.
There was a problem hiding this comment.
yes after reading the rest of the PR I got that. Can you add a comment saying ^^? just in case someone else will work in the startup process in a few months.
| @@ -0,0 +1,42 @@ | |||
| require_relative 'registry/registerable' | |||
There was a problem hiding this comment.
should be a require_relative?
There was a problem hiding this comment.
I usually prefer relative paths, but that is more a matter of style, I guess. I think require_relative tend to be more common in gems, specially when you're loading components that are associated.
| # Patchers should expose 2 methods: | ||
| # - patch, which applies our patch if needed. Should be idempotent, | ||
| # can be call twice but should just do nothing the second time. | ||
| # - patched?, which returns true if the module has been succesfully | ||
| # patched (patching might have failed if requirements were not here) | ||
| @patchers = { elasticsearch: Datadog::Contrib::Elasticsearch::Patcher, |
There was a problem hiding this comment.
finally we get rid of that 🎉
| @mutex = Mutex.new | ||
| @registry = Datadog.registry |
There was a problem hiding this comment.
should we really keep Datadog.registry as a reference + attr_accessor? what is the use case to not use directly Datadog.registry?
There was a problem hiding this comment.
Basically by injecting the dependency we can test everything in isolation. Right now testing the Monkey class is a nightmare, because every time we create a new integration we have to update 10 lines of test in the Monkey. In my opinion Monkey tests should be agnostic of the existing contributions. Monkey should be responsible for calling #patch in contribution modules, that's it. By having that, we can write such tests.
| @mutex = Mutex.new | ||
| @registry = Datadog.registry |
| @@ -0,0 +1,42 @@ | |||
| require_relative 'registry/registerable' | |||
This PR provides the
Registryabstraction that will help us to refactor the configuration API. The idea is to provide a clear interface for adding/retrieving different integrations and to decouple theDatadog::Monkeyfrom it.The PR also includes a
Registerablemodule that provides a#register_asconvenience method to let different integrations to "self-register" themselves.