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.

Sign up for GitHub

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

Jump to bottom

Contrib:Make registry a global constant repository #1572

Merged
merged 21 commits into from Jul 26, 2021
Merged

Contrib:Make registry a global constant repository #1572

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
ebfe116
Contrib:Make registry a global constant repository
marcotc Jun 30, 2021
f379daa
Merge branch 'master' into global-registry
marcotc Jul 23, 2021
0967aee
Move requires to outside module
marcotc Jul 23, 2021
72e8d43
Move new contrib file to new namespace
marcotc Jul 23, 2021
741b362
Fix registry= deprecation message
marcotc Jul 23, 2021
f9e67d7
Better user message on registry=
marcotc Jul 23, 2021
8714ae5
Better message for future deprecation removal
marcotc Jul 23, 2021
ac2c6bb
Use Datadog.registry on contrib test
marcotc Jul 23, 2021
0416d9f
Match new warning message correctly in test
marcotc Jul 23, 2021
dca2ae5
Document registry conundrum
marcotc Jul 23, 2021
537a9c0
Add draft release action
marcotc Jul 23, 2021
8ed81ac
Merge branch 'master' into global-registry
marcotc Jul 26, 2021
bf4ff8b
Re-add semantic logger
marcotc Jul 26, 2021
d0faf6d
Remove bad merge restoring require at top leve
marcotc Jul 26, 2021
d1d0c66
Document initialization in constant place
marcotc Jul 26, 2021
92dcfca
Suggest public API
marcotc Jul 26, 2021
d7ff61c
Fix typo
marcotc Jul 26, 2021
25e99d3
Another typo
marcotc Jul 26, 2021
95ed435
Add mock name
marcotc Jul 26, 2021
141736c
Oops file
marcotc Jul 26, 2021
7d14b39
Merge branch 'master' into global-registry
marcotc Jul 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
58 changes: 10 additions & 48 deletions lib/ddtrace.rb
View file
Open in desktop
Expand Up @@ -22,62 +22,24 @@ module Datadog
extend Configuration
extend AutoInstrumentBase

# Load and extend Contrib by default
# Load built-in Datadog integrations
require 'ddtrace/contrib'
marcotc marked this conversation as resolved.
Show resolved Hide resolved

# Load extension to global Datadog objects
require 'ddtrace/contrib/extensions'
extend Contrib::Extensions
extend(Contrib::Extensions)
Configuration::Settings.include(Contrib::Extensions::Configuration::Settings)

# Load Contrib auto instrumentation
require 'ddtrace/contrib/auto_instrument'
extend(Contrib::AutoInstrument)

# Load and extend OpenTelemetry compatibility by default
require 'ddtrace/opentelemetry/extensions'
extend OpenTelemetry::Extensions

# Load and extend AutoInstrument
require 'ddtrace/contrib/auto_instrument'
extend Contrib::AutoInstrument

# Add shutdown hook:
# Ensures the tracer has an opportunity to flush traces
# and cleanup before terminating the process.
marcotc marked this conversation as resolved.
Show resolved Hide resolved
at_exit { Datadog.shutdown! }
end

require 'ddtrace/contrib/action_cable/integration'
require 'ddtrace/contrib/action_pack/integration'
require 'ddtrace/contrib/action_view/integration'
require 'ddtrace/contrib/active_model_serializers/integration'
require 'ddtrace/contrib/active_record/integration'
require 'ddtrace/contrib/active_support/integration'
require 'ddtrace/contrib/aws/integration'
require 'ddtrace/contrib/concurrent_ruby/integration'
require 'ddtrace/contrib/dalli/integration'
require 'ddtrace/contrib/delayed_job/integration'
require 'ddtrace/contrib/elasticsearch/integration'
require 'ddtrace/contrib/ethon/integration'
require 'ddtrace/contrib/excon/integration'
require 'ddtrace/contrib/faraday/integration'
require 'ddtrace/contrib/grape/integration'
require 'ddtrace/contrib/graphql/integration'
require 'ddtrace/contrib/grpc/integration'
require 'ddtrace/contrib/http/integration'
require 'ddtrace/contrib/httpclient/integration'
require 'ddtrace/contrib/httprb/integration'
require 'ddtrace/contrib/integration'
require 'ddtrace/contrib/kafka/integration'
require 'ddtrace/contrib/lograge/integration'
require 'ddtrace/contrib/presto/integration'
require 'ddtrace/contrib/que/integration'
require 'ddtrace/contrib/mysql2/integration'
require 'ddtrace/contrib/mongodb/integration'
require 'ddtrace/contrib/qless/integration'
require 'ddtrace/contrib/racecar/integration'
require 'ddtrace/contrib/rack/integration'
require 'ddtrace/contrib/rails/integration'
require 'ddtrace/contrib/rake/integration'
require 'ddtrace/contrib/redis/integration'
require 'ddtrace/contrib/resque/integration'
require 'ddtrace/contrib/rest_client/integration'
require 'ddtrace/contrib/sequel/integration'
require 'ddtrace/contrib/shoryuken/integration'
require 'ddtrace/contrib/sidekiq/integration'
require 'ddtrace/contrib/sinatra/integration'
require 'ddtrace/contrib/sneakers/integration'
require 'ddtrace/contrib/sucker_punch/integration'
63 changes: 63 additions & 0 deletions lib/ddtrace/contrib.rb
View file
Open in desktop
@@ -0,0 +1,63 @@
require 'ddtrace/contrib/registry'

marcotc marked this conversation as resolved.
Show resolved Hide resolved
module Datadog
module Contrib
# Registry is a global, declarative repository of all available integrations.
#
# Integrations should register themselves with the registry as early as
# possible as the initial tracer configuration can only activate integrations
# if they have already been registered.
#
# Despite that, integrations *can* be appended to the registry at any point
# of the program execution. Newly appended integrations can then be
# activated during tracer reconfiguration.
#
# The registry does not depend on runtime configuration and registered integrations
# must execute correctly after successive configuration changes.
# The registered integrations themselves can depend on the stateful configuration
# of the tracer.
REGISTRY = Registry.new
end
end

require 'ddtrace/contrib/action_cable/integration'
require 'ddtrace/contrib/action_pack/integration'
require 'ddtrace/contrib/action_view/integration'
require 'ddtrace/contrib/active_model_serializers/integration'
require 'ddtrace/contrib/active_record/integration'
require 'ddtrace/contrib/active_support/integration'
require 'ddtrace/contrib/aws/integration'
require 'ddtrace/contrib/concurrent_ruby/integration'
require 'ddtrace/contrib/dalli/integration'
require 'ddtrace/contrib/delayed_job/integration'
require 'ddtrace/contrib/elasticsearch/integration'
require 'ddtrace/contrib/ethon/integration'
require 'ddtrace/contrib/excon/integration'
require 'ddtrace/contrib/faraday/integration'
require 'ddtrace/contrib/grape/integration'
require 'ddtrace/contrib/graphql/integration'
require 'ddtrace/contrib/grpc/integration'
require 'ddtrace/contrib/http/integration'
require 'ddtrace/contrib/httpclient/integration'
require 'ddtrace/contrib/httprb/integration'
require 'ddtrace/contrib/integration'
require 'ddtrace/contrib/kafka/integration'
require 'ddtrace/contrib/lograge/integration'
require 'ddtrace/contrib/presto/integration'
require 'ddtrace/contrib/que/integration'
require 'ddtrace/contrib/mysql2/integration'
require 'ddtrace/contrib/mongodb/integration'
require 'ddtrace/contrib/qless/integration'
require 'ddtrace/contrib/racecar/integration'
require 'ddtrace/contrib/rack/integration'
require 'ddtrace/contrib/rails/integration'
require 'ddtrace/contrib/rake/integration'
require 'ddtrace/contrib/redis/integration'
require 'ddtrace/contrib/resque/integration'
require 'ddtrace/contrib/rest_client/integration'
require 'ddtrace/contrib/sequel/integration'
require 'ddtrace/contrib/shoryuken/integration'
require 'ddtrace/contrib/sidekiq/integration'
require 'ddtrace/contrib/sinatra/integration'
require 'ddtrace/contrib/sneakers/integration'
require 'ddtrace/contrib/sucker_punch/integration'
2 changes: 1 addition & 1 deletion lib/ddtrace/contrib/auto_instrument.rb
View file
Open in desktop
Expand Up @@ -25,7 +25,7 @@ def add_auto_instrument
def self.patch_all
integrations = []

Datadog.registry.each do |integration|
Contrib::REGISTRY.each do |integration|
# some instrumentations are automatically enabled when the `rails` instrumentation is enabled,
# patching them on their own automatically outside of the rails integration context would
# cause undesirable service naming, so we exclude them based their auto_instrument? setting.
marcotc marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
41 changes: 30 additions & 11 deletions lib/ddtrace/contrib/extensions.rb
View file
Open in desktop
@@ -1,21 +1,28 @@
require 'set'
require 'ddtrace/contrib/registry'
require 'ddtrace/contrib'
require 'ddtrace/configuration/settings'

module Datadog
module Contrib
Comment on lines 5 to 6
Copy link
Member

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 😭

# Extensions that can be added to the base library
# Adds registry, configuration access for integrations.
module Extensions
def self.extended(base)
Datadog.extend(Helpers)
Datadog.extend(Configuration)
Datadog::Configuration::Settings.include(Configuration::Settings)
base.extend(Helpers)
base.extend(Configuration)
end

# 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.
marcotc marked this conversation as resolved.
Show resolved Hide resolved
#
# External use of this method is always safe.
def registry
configuration.registry
Contrib::REGISTRY
end
end
Comment on lines 21 to 33
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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 😐

Copy link
Member

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 :)


Expand Down Expand Up @@ -60,11 +67,23 @@ def configure(target = configuration, opts = {})
module Settings
InvalidIntegrationError = Class.new(StandardError)

def self.included(base)
# Add the additional options to the global configuration settings
base.instance_eval do
option :registry, default: Registry.new
end
# The registry only holds declarative constant values and cannot be modified.
# This option is a no-op and will be removed in the future.
#
# @deprecated Use `Datadog::Contrib::REGISTRY` instead
marcotc marked this conversation as resolved.
Show resolved Hide resolved
def registry
Datadog.logger.warn('Deprecated access to `Datadog.configuration.registry`, use `Datadog.registry` instead.')
Contrib::REGISTRY
Copy link
Member

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?

Copy link
Member Author

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.

end

# The registry only holds declarative constant values and cannot be modified.
# This option is a no-op and will be removed in the future.
#
# @deprecated Use `Datadog::Contrib::REGISTRY` instead
def registry=(_arg)
marcotc marked this conversation as resolved.
Show resolved Hide resolved
Datadog.logger.warn('Deprecated configuration attempt of the tracer registry. ' \
'The register is a global constant and cannot be directly overwritten.' \
'No changes too place in this call.')
marcotc marked this conversation as resolved.
Show resolved Hide resolved
end

# For the provided `integration_name`, resolves a matching configuration
Expand Down Expand Up @@ -125,7 +144,7 @@ def reset!
end

def fetch_integration(name)
registry[name] ||
Contrib::REGISTRY[name] ||
raise(InvalidIntegrationError, "'#{name}' is not a valid integration.")
end

Expand Down
4 changes: 2 additions & 2 deletions lib/ddtrace/contrib/registerable.rb
View file
Open in desktop
@@ -1,4 +1,4 @@
require 'ddtrace/contrib/registry'
require 'ddtrace/contrib'

module Datadog
module Contrib
Expand All @@ -12,7 +12,7 @@ def self.included(base)
# Class methods for registerable behavior
module ClassMethods
def register_as(name, options = {})
registry = options.fetch(:registry, Datadog.registry)
registry = options.fetch(:registry, Contrib::REGISTRY)
auto_patch = options.fetch(:auto_patch, false)

registry.add(name, new(name, options), auto_patch)
Expand Down
2 changes: 1 addition & 1 deletion spec/ddtrace/auto_instrument_spec.rb
View file
Open in desktop
Expand Up @@ -24,7 +24,7 @@
require 'ddtrace/auto_instrument'
end

after { Datadog.registry[:sinatra].reset_configuration! }
after { Datadog::Contrib::REGISTRY[:sinatra].reset_configuration! }
marcotc marked this conversation as resolved.
Show resolved Hide resolved

shared_context 'ActiveRecord database' do
let(:application_record_class) do
Expand Down
4 changes: 2 additions & 2 deletions spec/ddtrace/contrib/action_cable/patcher_spec.rb
View file
Open in desktop
Expand Up @@ -32,9 +32,9 @@

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
Copy link
Member

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 }.

Copy link
Member Author

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 :)


context 'with server' do
Expand Down
4 changes: 2 additions & 2 deletions spec/ddtrace/contrib/active_model_serializers/patcher_spec.rb
View file
Open in desktop
Expand Up @@ -35,9 +35,9 @@

around do |example|
# Reset before and after each example; don't allow global state to linger.
Datadog.registry[:active_model_serializers].reset_configuration!
Datadog::Contrib::REGISTRY[:active_model_serializers].reset_configuration!
example.run
Datadog.registry[:active_model_serializers].reset_configuration!
Datadog::Contrib::REGISTRY[:active_model_serializers].reset_configuration!
end

describe 'on render' do
Expand Down
4 changes: 2 additions & 2 deletions spec/ddtrace/contrib/active_record/multi_db_spec.rb
View file
Open in desktop
Expand Up @@ -83,7 +83,7 @@ def mysql_connection_string
end

before do
Datadog.registry[:active_record].reset_configuration!
Datadog::Contrib::REGISTRY[:active_record].reset_configuration!

Datadog.configure do |c|
c.use :active_record, configuration_options
Expand All @@ -93,7 +93,7 @@ def mysql_connection_string
end

after do
Datadog.registry[:active_record].reset_configuration!
Datadog::Contrib::REGISTRY[:active_record].reset_configuration!
end

context 'when databases are configured with' do
Expand Down
2 changes: 1 addition & 1 deletion spec/ddtrace/contrib/active_record/performance_spec.rb
View file
Open in desktop
Expand Up @@ -21,7 +21,7 @@
end
end

after { Datadog.registry[:active_record].reset_configuration! }
after { Datadog::Contrib::REGISTRY[:active_record].reset_configuration! }

describe 'for an in-memory database' do
let!(:connection) do
Expand Down
4 changes: 2 additions & 2 deletions spec/ddtrace/contrib/active_record/tracer_spec.rb
View file
Open in desktop
Expand Up @@ -26,9 +26,9 @@

around do |example|
# Reset before and after each example; don't allow global state to linger.
Datadog.registry[:active_record].reset_configuration!
Datadog::Contrib::REGISTRY[:active_record].reset_configuration!
example.run
Datadog.registry[:active_record].reset_configuration!
Datadog::Contrib::REGISTRY[:active_record].reset_configuration!
end

context 'when query is made' do
Expand Down
4 changes: 2 additions & 2 deletions spec/ddtrace/contrib/aws/instrumentation_spec.rb
View file
Open in desktop
Expand Up @@ -21,9 +21,9 @@

around do |example|
# Reset before and after each example; don't allow global state to linger.
Datadog.registry[:aws].reset_configuration!
Datadog::Contrib::REGISTRY[:aws].reset_configuration!
example.run
Datadog.registry[:aws].reset_configuration!
Datadog::Contrib::REGISTRY[:aws].reset_configuration!
end

context 'when the client runs' do
Expand Down
4 changes: 2 additions & 2 deletions spec/ddtrace/contrib/dalli/instrumentation_spec.rb
View file
Open in desktop
Expand Up @@ -22,9 +22,9 @@

around do |example|
# Reset before and after each example; don't allow global state to linger.
Datadog.registry[:dalli].reset_configuration!
Datadog::Contrib::REGISTRY[:dalli].reset_configuration!
example.run
Datadog.registry[:dalli].reset_configuration!
Datadog::Contrib::REGISTRY[:dalli].reset_configuration!
end

describe 'when a client calls #set' do
Expand Down
4 changes: 2 additions & 2 deletions spec/ddtrace/contrib/delayed_job/plugin_spec.rb
View file
Open in desktop
Expand Up @@ -35,9 +35,9 @@ def job_data

around do |example|
# Reset before and after each example; don't allow global state to linger.
Datadog.registry[:delayed_job].reset_configuration!
Datadog::Contrib::REGISTRY[:delayed_job].reset_configuration!
example.run
Datadog.registry[:delayed_job].reset_configuration!
Datadog::Contrib::REGISTRY[:delayed_job].reset_configuration!
end

describe 'instrumenting worker execution' do
Expand Down
4 changes: 2 additions & 2 deletions spec/ddtrace/contrib/elasticsearch/patcher_spec.rb
View file
Open in desktop
Expand Up @@ -23,9 +23,9 @@

around do |example|
# Reset before and after each example; don't allow global state to linger.
Datadog.registry[:elasticsearch].reset_configuration!
Datadog::Contrib::REGISTRY[:elasticsearch].reset_configuration!
example.run
Datadog.registry[:elasticsearch].reset_configuration!
Datadog::Contrib::REGISTRY[:elasticsearch].reset_configuration!
end

describe 'cluster health request' do
Expand Down
2 changes: 1 addition & 1 deletion spec/ddtrace/contrib/elasticsearch/transport_spec.rb
View file
Open in desktop
Expand Up @@ -31,7 +31,7 @@
end
end

after { Datadog.registry[:elasticsearch].reset_configuration! }
after { Datadog::Contrib::REGISTRY[:elasticsearch].reset_configuration! }

context 'when configured with middleware' do
let(:client) do
Expand Down
4 changes: 2 additions & 2 deletions spec/ddtrace/contrib/ethon/easy_patch_spec.rb
View file
Open in desktop
Expand Up @@ -19,9 +19,9 @@

around do |example|
# Reset before and after each example; don't allow global state to linger.
Datadog.registry[:ethon].reset_configuration!
Datadog::Contrib::REGISTRY[:ethon].reset_configuration!
example.run
Datadog.registry[:ethon].reset_configuration!
Datadog::Contrib::REGISTRY[:ethon].reset_configuration!
end

describe '#http_request' do
Expand Down
4 changes: 2 additions & 2 deletions spec/ddtrace/contrib/ethon/integration_context.rb
View file
Open in desktop
Expand Up @@ -76,8 +76,8 @@

around do |example|
# Reset before and after each example; don't allow global state to linger.
Datadog.registry[:ethon].reset_configuration!
Datadog::Contrib::REGISTRY[:ethon].reset_configuration!
example.run
Datadog.registry[:ethon].reset_configuration!
Datadog::Contrib::REGISTRY[:ethon].reset_configuration!
end
end
4 changes: 2 additions & 2 deletions spec/ddtrace/contrib/ethon/multi_patch_spec.rb
View file
Open in desktop
Expand Up @@ -18,9 +18,9 @@

around do |example|
# Reset before and after each example; don't allow global state to linger.
Datadog.registry[:ethon].reset_configuration!
Datadog::Contrib::REGISTRY[:ethon].reset_configuration!
example.run
Datadog.registry[:ethon].reset_configuration!
Datadog::Contrib::REGISTRY[:ethon].reset_configuration!
end

describe '#add' do
Expand Down