From 917747bbbe26399f059d3d017b177ce087f96557 Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev <156273877+p-datadog@users.noreply.github.com> Date: Thu, 20 Feb 2025 12:45:07 -0500 Subject: [PATCH] DEBUG-3535 DRY Transport::HTTP::Builder, move to core (#4413) --- lib/datadog/core/remote/transport/http.rb | 19 +- .../core/remote/transport/http/builder.rb | 219 ------------------ .../transport/http/builder.rb | 54 +++-- lib/datadog/tracing/transport/http.rb | 20 +- .../core/remote/transport/http/builder.rbs | 71 ------ .../transport/http/builder.rbs | 8 +- .../transport/http/builder_spec.rb | 17 +- spec/datadog/tracing/integration_spec.rb | 1 - spec/datadog/tracing/transport/http_spec.rb | 12 +- 9 files changed, 81 insertions(+), 340 deletions(-) delete mode 100644 lib/datadog/core/remote/transport/http/builder.rb rename lib/datadog/{tracing => core}/transport/http/builder.rb (83%) delete mode 100644 sig/datadog/core/remote/transport/http/builder.rbs rename sig/datadog/{tracing => core}/transport/http/builder.rbs (87%) rename spec/datadog/{tracing => core}/transport/http/builder_spec.rb (95%) diff --git a/lib/datadog/core/remote/transport/http.rb b/lib/datadog/core/remote/transport/http.rb index 190a9c45646..929064ce34c 100644 --- a/lib/datadog/core/remote/transport/http.rb +++ b/lib/datadog/core/remote/transport/http.rb @@ -5,6 +5,7 @@ require_relative '../../environment/container' require_relative '../../environment/ext' require_relative '../../transport/ext' +require_relative '../../transport/http/builder' require_relative '../../transport/http/adapters/net' require_relative '../../transport/http/adapters/unix_socket' require_relative '../../transport/http/adapters/test' @@ -19,13 +20,7 @@ # require_relative '../../transport/http/api' require_relative 'http/api' -# TODO: Decouple transport/http/builder -# -# See http/builder -# -# Below should be: -# require_relative '../../transport/http/builder' -require_relative 'http/builder' +require_relative 'http/api/instance' # TODO: Decouple transport/http # @@ -53,7 +48,9 @@ module HTTP # Builds a new Transport::HTTP::Client def new(klass, &block) - Builder.new(&block).to_transport(klass) + Core::Transport::HTTP::Builder.new( + api_instance_class: API::Instance, &block + ).to_transport(klass) end # Builds a new Transport::HTTP::Client with default settings @@ -133,15 +130,15 @@ def default_adapter end # Add adapters to registry - Builder::REGISTRY.set( + Core::Transport::HTTP::Builder::REGISTRY.set( Datadog::Core::Transport::HTTP::Adapters::Net, Datadog::Core::Configuration::Ext::Agent::HTTP::ADAPTER ) - Builder::REGISTRY.set( + Core::Transport::HTTP::Builder::REGISTRY.set( Datadog::Core::Transport::HTTP::Adapters::Test, Datadog::Core::Transport::Ext::Test::ADAPTER ) - Builder::REGISTRY.set( + Core::Transport::HTTP::Builder::REGISTRY.set( Datadog::Core::Transport::HTTP::Adapters::UnixSocket, Datadog::Core::Configuration::Ext::Agent::UnixSocket::ADAPTER ) diff --git a/lib/datadog/core/remote/transport/http/builder.rb b/lib/datadog/core/remote/transport/http/builder.rb deleted file mode 100644 index 393e8f6a1d6..00000000000 --- a/lib/datadog/core/remote/transport/http/builder.rb +++ /dev/null @@ -1,219 +0,0 @@ -# frozen_string_literal: true - -require_relative '../../../configuration/agent_settings_resolver' -require_relative '../../../transport/http/adapters/registry' -require_relative '../../../transport/http/api/map' - -# TODO: Decouple standard transport/http/api/instance -# -# Separate classes are needed because transport/http/trace includes -# Trace::API::Instance which closes over and uses a single spec, which is -# negotiated as either /v3 or /v4 for the whole API at the spec level, but we -# need an independent toplevel path at the endpoint level. -# -# Separate classes are needed because of `include Trace::API::Instance`. -# -# Below should be: -# require_relative '../../../../datadog/core/transport/http/api/instance' -require_relative 'api/instance' - -# TODO: Decouple transport/http/client -# -# The standard one does `include Transport::HTTP::Statistics` and performs -# stats updates, which may or may not be desirable in general. -# -# Below should be: -# require_relative '../../../../datadog/core/transport/http/client' -require_relative 'client' - -# TODO: Decouple transport/http/builder -# -# This class is duplicated even though it is tantalisingly close to the -# one in datadog/core/transport mostly because it refers to a different -# `API::Instance` in `#api_instance_class` but also because it operates on a -# different `HTTP::Client`, as well as de-hardcoding the transport class -# `Transport::Traces::Transport` in `#to_transport` -# -# Additionally, its design makes it so there can be only one (API, Client, -# Transport) triplet, requiring a new transport instance for any varying -# element of the triplet. - -module Datadog - module Core - module Remote - module Transport - module HTTP - # Builds new instances of Transport::HTTP::Client - class Builder - REGISTRY = Datadog::Core::Transport::HTTP::Adapters::Registry.new - - attr_reader \ - :apis, - :api_options, - :default_adapter, - :default_api, - :default_headers - - def initialize - # Global settings - @default_adapter = nil - @default_headers = {} - - # Client settings - @apis = Datadog::Core::Transport::HTTP::API::Map.new - @default_api = nil - - # API settings - @api_options = {} - - yield(self) if block_given? - end - - def adapter(config, *args, **kwargs) - @default_adapter = case config - when Core::Configuration::AgentSettingsResolver::AgentSettings - registry_klass = REGISTRY.get(config.adapter) - raise UnknownAdapterError, config.adapter if registry_klass.nil? - - registry_klass.build(config) - when Symbol - registry_klass = REGISTRY.get(config) - raise UnknownAdapterError, config if registry_klass.nil? - - registry_klass.new(*args, **kwargs) - else - config - end - end - - def headers(values = {}) - @default_headers.merge!(values) - end - - # Adds a new API to the client - # Valid options: - # - :adapter - # - :default - # - :fallback - # - :headers - def api(key, spec, options = {}) - options = options.dup - - # Copy spec into API map - @apis[key] = spec - - # Apply as default API, if specified to do so. - @default_api = key if options.delete(:default) || @default_api.nil? - - # Save all other settings for initialization - (@api_options[key] ||= {}).merge!(options) - end - - def default_api=(key) - raise UnknownApiError, key unless @apis.key?(key) - - @default_api = key - end - - def to_transport(klass) - raise NoDefaultApiError if @default_api.nil? - - klass.new(to_api_instances, @default_api) - end - - def to_api_instances - raise NoApisError if @apis.empty? - - @apis.inject(Datadog::Core::Transport::HTTP::API::Map.new) do |instances, (key, spec)| - instances.tap do - api_options = @api_options[key].dup - - # Resolve the adapter to use for this API - adapter = api_options.delete(:adapter) || @default_adapter - raise NoAdapterForApiError, key if adapter.nil? - - # Resolve fallback and merge headers - fallback = api_options.delete(:fallback) - api_options[:headers] = @default_headers.merge((api_options[:headers] || {})) - - # Add API::Instance with all settings - instances[key] = api_instance_class.new( - spec, - adapter, - api_options - ) - - # Configure fallback, if provided. - instances.with_fallbacks(key => fallback) unless fallback.nil? - end - end - end - - def api_instance_class - API::Instance - end - - # Raised when the API key does not match known APIs. - class UnknownApiError < StandardError - attr_reader :key - - def initialize(key) - super() - - @key = key - end - - def message - "Unknown transport API '#{key}'!" - end - end - - # Raised when the identifier cannot be matched to an adapter. - class UnknownAdapterError < StandardError - attr_reader :type - - def initialize(type) - super() - - @type = type - end - - def message - "Unknown transport adapter '#{type}'!" - end - end - - # Raised when an adapter cannot be resolved for an API instance. - class NoAdapterForApiError < StandardError - attr_reader :key - - def initialize(key) - super() - - @key = key - end - - def message - "No adapter resolved for transport API '#{key}'!" - end - end - - # Raised when built without defining APIs. - class NoApisError < StandardError - def message - 'No APIs configured for transport!' - end - end - - # Raised when client built without defining a default API. - class NoDefaultApiError < StandardError - def message - 'No default API configured for transport!' - end - end - end - end - end - end - end -end diff --git a/lib/datadog/tracing/transport/http/builder.rb b/lib/datadog/core/transport/http/builder.rb similarity index 83% rename from lib/datadog/tracing/transport/http/builder.rb rename to lib/datadog/core/transport/http/builder.rb index e870991b70a..9c6413e71fe 100644 --- a/lib/datadog/tracing/transport/http/builder.rb +++ b/lib/datadog/core/transport/http/builder.rb @@ -1,13 +1,11 @@ # frozen_string_literal: true -require_relative '../../../core/configuration/agent_settings_resolver' -require_relative '../../../core/transport/http/adapters/registry' -require_relative '../../../core/transport/http/api/map' -require_relative 'api/instance' -require_relative 'client' +require_relative '../../configuration/agent_settings_resolver' +require_relative 'adapters/registry' +require_relative 'api/map' module Datadog - module Tracing + module Core module Transport module HTTP # Builds new instances of Transport::HTTP::Client @@ -15,13 +13,14 @@ class Builder REGISTRY = Datadog::Core::Transport::HTTP::Adapters::Registry.new attr_reader \ + :api_instance_class, :apis, :api_options, :default_adapter, :default_api, :default_headers - def initialize + def initialize(api_instance_class:) # Global settings @default_adapter = nil @default_headers = {} @@ -33,6 +32,8 @@ def initialize # API settings @api_options = {} + @api_instance_class = api_instance_class + yield(self) if block_given? end @@ -82,11 +83,10 @@ def default_api=(key) @default_api = key end - def to_transport + def to_transport(klass) raise NoDefaultApiError if @default_api.nil? - # DEV: Should not be specific to traces - Transport::Traces::Transport.new(to_api_instances, @default_api) + klass.new(to_api_instances, @default_api) end def to_api_instances @@ -117,28 +117,48 @@ def to_api_instances end end - def api_instance_class - API::Instance - end - # Raised when the API key does not match known APIs. class UnknownApiError < StandardError + attr_reader :key + def initialize(key) - super("Unknown transport API '#{key}'!") + super() + + @key = key + end + + def message + "Unknown transport API '#{key}'!" end end # Raised when the identifier cannot be matched to an adapter. class UnknownAdapterError < StandardError + attr_reader :type + def initialize(type) - super("Unknown transport adapter '#{type}'!") + super() + + @type = type + end + + def message + "Unknown transport adapter '#{type}'!" end end # Raised when an adapter cannot be resolved for an API instance. class NoAdapterForApiError < StandardError + attr_reader :key + def initialize(key) - super("No adapter resolved for transport API '#{key}'!") + super() + + @key = key + end + + def message + "No adapter resolved for transport API '#{key}'!" end end diff --git a/lib/datadog/tracing/transport/http.rb b/lib/datadog/tracing/transport/http.rb index 190dc31e408..1695c44b5eb 100644 --- a/lib/datadog/tracing/transport/http.rb +++ b/lib/datadog/tracing/transport/http.rb @@ -8,8 +8,9 @@ require_relative '../../core/transport/http/adapters/net' require_relative '../../core/transport/http/adapters/test' require_relative '../../core/transport/http/adapters/unix_socket' +require_relative '../../core/transport/http/builder' require_relative 'http/api' -require_relative 'http/builder' +require_relative 'http/api/instance' require_relative '../../../datadog/version' module Datadog @@ -30,8 +31,10 @@ module HTTP module_function # Builds a new Transport::HTTP::Client - def new(&block) - Builder.new(&block).to_transport + def new(klass, &block) + Core::Transport::HTTP::Builder.new( + api_instance_class: API::Instance, &block + ).to_transport(klass) end # Builds a new Transport::HTTP::Client with default settings @@ -40,7 +43,7 @@ def default( agent_settings: DO_NOT_USE_ENVIRONMENT_AGENT_SETTINGS, **options ) - new do |transport| + new(Transport::Traces::Transport) do |transport| transport.adapter(agent_settings) transport.headers default_headers @@ -86,12 +89,15 @@ def default_adapter end # Add adapters to registry - Builder::REGISTRY.set( + Core::Transport::HTTP::Builder::REGISTRY.set( Datadog::Core::Transport::HTTP::Adapters::Net, Datadog::Core::Configuration::Ext::Agent::HTTP::ADAPTER ) - Builder::REGISTRY.set(Datadog::Core::Transport::HTTP::Adapters::Test, Datadog::Core::Transport::Ext::Test::ADAPTER) - Builder::REGISTRY.set( + Core::Transport::HTTP::Builder::REGISTRY.set( + Datadog::Core::Transport::HTTP::Adapters::Test, + Datadog::Core::Transport::Ext::Test::ADAPTER + ) + Core::Transport::HTTP::Builder::REGISTRY.set( Datadog::Core::Transport::HTTP::Adapters::UnixSocket, Datadog::Core::Transport::Ext::UnixSocket::ADAPTER ) diff --git a/sig/datadog/core/remote/transport/http/builder.rbs b/sig/datadog/core/remote/transport/http/builder.rbs deleted file mode 100644 index 4bed7b46c63..00000000000 --- a/sig/datadog/core/remote/transport/http/builder.rbs +++ /dev/null @@ -1,71 +0,0 @@ -module Datadog - module Core - module Remote - module Transport - module HTTP - class Builder - REGISTRY: Datadog::Core::Transport::HTTP::Adapters::Registry - - attr_reader apis: untyped - - attr_reader api_options: untyped - - attr_reader default_adapter: untyped - - attr_reader default_api: untyped - - attr_reader default_headers: untyped - - def initialize: () ?{ (untyped) -> untyped } -> void - - def adapter: (untyped config, *untyped args, **untyped kwargs) -> untyped - - def headers: (?::Hash[untyped, untyped] values) -> untyped - - def api: (untyped key, untyped spec, ?::Hash[untyped, untyped] options) -> untyped - - def default_api=: (untyped key) -> untyped - - def to_transport: (untyped klass) -> untyped - - def to_api_instances: () -> untyped - - def api_instance_class: () -> untyped - - class UnknownApiError < StandardError - attr_reader key: untyped - - def initialize: (untyped key) -> void - - def message: () -> ::String - end - - class UnknownAdapterError < StandardError - attr_reader type: untyped - - def initialize: (untyped `type`) -> void - - def message: () -> ::String - end - - class NoAdapterForApiError < StandardError - attr_reader key: untyped - - def initialize: (untyped key) -> void - - def message: () -> ::String - end - - class NoApisError < StandardError - def message: () -> "No APIs configured for transport!" - end - - class NoDefaultApiError < StandardError - def message: () -> "No default API configured for transport!" - end - end - end - end - end - end -end diff --git a/sig/datadog/tracing/transport/http/builder.rbs b/sig/datadog/core/transport/http/builder.rbs similarity index 87% rename from sig/datadog/tracing/transport/http/builder.rbs rename to sig/datadog/core/transport/http/builder.rbs index 95a11954c11..ec7a2842495 100644 --- a/sig/datadog/tracing/transport/http/builder.rbs +++ b/sig/datadog/core/transport/http/builder.rbs @@ -1,9 +1,9 @@ module Datadog - module Tracing + module Core module Transport module HTTP class Builder - REGISTRY: untyped + REGISTRY: Datadog::Core::Transport::HTTP::Adapters::Registry attr_reader apis: untyped @@ -15,7 +15,7 @@ module Datadog attr_reader default_headers: untyped - def initialize: () { (untyped) -> untyped } -> void + def initialize: (api_instance_class: untyped) ?{ (untyped) -> untyped } -> void def adapter: (untyped config, *untyped args, **untyped kwargs) -> untyped @@ -25,7 +25,7 @@ module Datadog def default_api=: (untyped key) -> untyped - def to_transport: () -> untyped + def to_transport: (untyped klass) -> untyped def to_api_instances: () -> untyped diff --git a/spec/datadog/tracing/transport/http/builder_spec.rb b/spec/datadog/core/transport/http/builder_spec.rb similarity index 95% rename from spec/datadog/tracing/transport/http/builder_spec.rb rename to spec/datadog/core/transport/http/builder_spec.rb index ad6a85f61ae..01d0291355b 100644 --- a/spec/datadog/tracing/transport/http/builder_spec.rb +++ b/spec/datadog/core/transport/http/builder_spec.rb @@ -1,13 +1,20 @@ require 'spec_helper' -require 'datadog/tracing/transport/http/builder' +require 'datadog/core/transport/http/builder' -RSpec.describe Datadog::Tracing::Transport::HTTP::Builder do - subject(:builder) { described_class.new } +RSpec.describe Datadog::Core::Transport::HTTP::Builder do + subject(:builder) { described_class.new(api_instance_class: Datadog::Tracing::Transport::HTTP::API::Instance) } describe '#initialize' do context 'given a block' do - it { expect { |b| described_class.new(&b) }.to yield_with_args(kind_of(described_class)) } + it { + expect do |b| + described_class.new( + api_instance_class: Datadog::Tracing::Transport::HTTP::API::Instance, + &b + ) + end.to yield_with_args(kind_of(described_class)) + } end end @@ -301,7 +308,7 @@ end describe '#to_transport' do - subject(:transport) { builder.to_transport } + subject(:transport) { builder.to_transport(Datadog::Tracing::Transport::Traces::Transport) } context 'when no default API has been defined' do it { expect { transport }.to raise_error(described_class::NoDefaultApiError) } diff --git a/spec/datadog/tracing/integration_spec.rb b/spec/datadog/tracing/integration_spec.rb index 649a2f17505..693747ed2f6 100644 --- a/spec/datadog/tracing/integration_spec.rb +++ b/spec/datadog/tracing/integration_spec.rb @@ -13,7 +13,6 @@ require 'datadog/tracing/writer' require 'datadog/tracing/transport/http' require 'datadog/tracing/transport/http/api' -require 'datadog/tracing/transport/http/builder' require 'datadog/tracing/transport/io' require 'datadog/tracing/transport/io/client' require 'datadog/tracing/transport/traces' diff --git a/spec/datadog/tracing/transport/http_spec.rb b/spec/datadog/tracing/transport/http_spec.rb index f7af5db609c..0db558e3740 100644 --- a/spec/datadog/tracing/transport/http_spec.rb +++ b/spec/datadog/tracing/transport/http_spec.rb @@ -6,15 +6,17 @@ RSpec.describe Datadog::Tracing::Transport::HTTP do describe '.new' do context 'given a block' do - subject(:new_http) { described_class.new(&block) } + subject(:new_http) do + described_class.new(api_instance_class: Datadog::Tracing::Transport::HTTP::API::Instance, &block) + end let(:block) { proc {} } - let(:builder) { instance_double(Datadog::Tracing::Transport::HTTP::Builder) } + let(:builder) { instance_double(Datadog::Core::Transport::HTTP::Builder) } let(:transport) { instance_double(Datadog::Tracing::Transport::Traces::Transport) } before do - expect(Datadog::Tracing::Transport::HTTP::Builder).to receive(:new) do |&blk| + expect(Datadog::Core::Transport::HTTP::Builder).to receive(:new) do |&blk| expect(blk).to be block builder end @@ -123,7 +125,7 @@ context 'that is not defined' do let(:api_version) { double('non-existent API') } - it { expect { default }.to raise_error(Datadog::Tracing::Transport::HTTP::Builder::UnknownApiError) } + it { expect { default }.to raise_error(Datadog::Core::Transport::HTTP::Builder::UnknownApiError) } end end @@ -143,7 +145,7 @@ context 'when given a block' do it do expect { |b| described_class.default(&b) }.to yield_with_args( - kind_of(Datadog::Tracing::Transport::HTTP::Builder) + kind_of(Datadog::Core::Transport::HTTP::Builder) ) end end