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
Contrib:Make registry a global constant repository #1572
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1572 +/- ##
========================================
Coverage 98.30% 98.30%
========================================
Files 900 901 +1
Lines 43194 43294 +100
========================================
+ Hits 42461 42560 +99
- Misses 733 734 +1
Continue to review full report at Codecov.
|
I've moved some stuff around, specially in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few notes, but nothing big/blocking. 👍
# Helper methods for Datadog module. | ||
module Helpers | ||
# Returns the global integration registry. | ||
# | ||
# This method is not safe to use while the tracer is initializing, | ||
# thus access to the registry should go through | ||
# ::Datadog::Contrib::REGISTRY for internal tracer work. | ||
# | ||
# External use of this method is always safe. | ||
def registry | ||
configuration.registry | ||
Contrib::REGISTRY | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This got me thinking: I understand that the integrations are not part of the core. Would it make sense to make the registry part of the core itself? It seems to be a bit "too modular" to have the extension support itself be modular, rather than something that's always there as part of the core.
TL;DR the main difference would be that we'd move this method directly inside the Datadog
module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can take a stab at that and see how it looks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking into it, I agree with this.
But I think this will modify too much to be able to fit nicely in this PR.
I documented this sentiment in code, so we know this is the direction we want for this part of our code in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: I agree with avoiding the bigger change, but on my second go around reviewing this I was thinking that we may be able to just inline the Helper
module into datadog.rb
, and thus start a bit on the correct path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to refactor Helper
, but it looked very ugly without moving the rest of extensions.rb
with it 😐
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, thanks for making a try :)
lib/ddtrace/contrib/extensions.rb
Outdated
def registry | ||
Datadog.logger.warn('Deprecated access to `Datadog.configuration.registry`, use `Datadog.registry` instead.') | ||
Contrib::REGISTRY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor:
- I recommend that we also state "Deprecated for removal" in the log message, to make it clear that we don't plan on leaving this around for long
- Should the last line just be
Datadog.registry
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the message.
The reason to use Datadog::Contrib::REGISTRY
internally in the tracer is that it's a direct access to the constant, and it's the only safe way to access the registry before the tracer initializes.
Datadog.registry
is only available after the tracer has fully initialized.
Today this makes no different because calls to Datadog.registry
lazily trigger the initialization of the whole tracer, but after we eager initialize the tracer that won't be the case. Users will always receive the tracer initialized, but internal components might see it uninitialized during gem loading.
module Datadog | ||
module Contrib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely not for this PR, but this whole file makes me sad. We shouldn't be monkey patching our own code to support extensions 😭
around do |example| | ||
# Reset before and after each example; don't allow global state to linger. | ||
Datadog.registry[:action_cable].reset_configuration! | ||
Datadog::Contrib::REGISTRY[:action_cable].reset_configuration! | ||
example.run | ||
Datadog.registry[:action_cable].reset_configuration! | ||
Datadog::Contrib::REGISTRY[:action_cable].reset_configuration! | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: This seems to be repeated so often that I wonder if we should have a nice helper for this -- something along the lines of with_clean_configuration(:action_cable) { example.run }
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spoiler: 99% of these reset_configuration!
will go away when the tracer is eager initialized (more so when the lifecyle object is correctly disposed and re-created on every run). So we'll soon remove these altogether instead :)
@ivoanjo I think I addresses all points you brought up, please let me know if anything is still missing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! Thanks for addressing my comments so far :)
# Helper methods for Datadog module. | ||
module Helpers | ||
# Returns the global integration registry. | ||
# | ||
# This method is not safe to use while the tracer is initializing, | ||
# thus access to the registry should go through | ||
# ::Datadog::Contrib::REGISTRY for internal tracer work. | ||
# | ||
# External use of this method is always safe. | ||
def registry | ||
configuration.registry | ||
Contrib::REGISTRY | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: I agree with avoiding the bigger change, but on my second go around reviewing this I was thinking that we may be able to just inline the Helper
module into datadog.rb
, and thus start a bit on the correct path.
Changelog text
The tracer's integration registry cannot be directly configured anymore (e.g.
c.registry = CustomRegistry.new
). As we make the tracer initialization process more strict, to ensure correctness, we removed the ability to arbitrarily modify the global registry.It is still possible to register custom integrations into the global registry at any point of the application execution.
PR description
As part of #1527, this PR make the integration registry a global constant, instead of a configurable value.
The registry today is effectively a declarative, additive list of supported integrations. These integrations are made available for configuration by being added to such register.
With changes to initialization coming in #1527, the registry will need to be strict with it's data policy: arbitrary modifications of the registry won't be allowed, and registered integrations will have to support being reconfigured without requiring re-registration.
This is already the case today, but will be enforced to ensure strict initialization corrected in #1527.
Accessing the registry through
Datadog.registry
is allowed, but internal references should useDatadog::Contrib::REGISTRY
, asDatadog.registry
is only available after the tracer has fully initialized.This PR also creates the
lib/ddtrace/contrib.rb
, which declares the registry, plus takes care of requiring all integrations. This helps with isolating contrib from the top-level ddtrace. This caused changes to the top-levellib/ddtrace.rb
file, which I believe more explicitly shows what's modified by loadingcontrib
extensions.