From 508b8dfae9c39238043fb0d1597389d8e2224c11 Mon Sep 17 00:00:00 2001 From: David Davis Date: Mon, 6 Jan 2020 10:54:41 -0800 Subject: [PATCH 01/64] rack: Add Gemfile, gemspec, version * rack-test: "simple testing API for Rack apps" * fix: "Could not find rake-12.3.3 in any of the sources" --- adapters/rack/Gemfile | 15 +++++++ .../opentelemetry/adapters/rack/version.rb | 13 +++++++ .../rack/opentelemetry-adapters-rack.gemspec | 39 +++++++++++++++++++ 3 files changed, 67 insertions(+) create mode 100644 adapters/rack/Gemfile create mode 100644 adapters/rack/lib/opentelemetry/adapters/rack/version.rb create mode 100644 adapters/rack/opentelemetry-adapters-rack.gemspec diff --git a/adapters/rack/Gemfile b/adapters/rack/Gemfile new file mode 100644 index 000000000..a022b2023 --- /dev/null +++ b/adapters/rack/Gemfile @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +# Copyright 2019 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +source 'https://rubygems.org' + +gemspec + +gem 'opentelemetry-api', path: '../../api' + +group :test do + gem 'opentelemetry-sdk', path: '../../sdk' +end diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/version.rb b/adapters/rack/lib/opentelemetry/adapters/rack/version.rb new file mode 100644 index 000000000..4d3816493 --- /dev/null +++ b/adapters/rack/lib/opentelemetry/adapters/rack/version.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +# Copyright 2019 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +module OpenTelemetry + module Adapters + module Rack + VERSION = '0.0.0' + end + end +end diff --git a/adapters/rack/opentelemetry-adapters-rack.gemspec b/adapters/rack/opentelemetry-adapters-rack.gemspec new file mode 100644 index 000000000..7500071b4 --- /dev/null +++ b/adapters/rack/opentelemetry-adapters-rack.gemspec @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +# Copyright 2019 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +lib = File.expand_path('lib', __dir__) +$LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib) +require 'opentelemetry/adapters/rack/version' + +Gem::Specification.new do |spec| + spec.name = 'opentelemetry-adapters-rack' + spec.version = OpenTelemetry::Adapters::Rack::VERSION + spec.authors = ['OpenTelemetry Authors'] + spec.email = ['cncf-opentelemetry-contributors@lists.cncf.io'] + + spec.summary = 'Rack instrumentation adapter for the OpenTelemetry framework' + spec.description = 'Rack instrumentation adapter for the OpenTelemetry framework' + spec.homepage = 'https://github.com/open-telemetry/opentelemetry-ruby' + spec.license = 'Apache-2.0' + + spec.files = ::Dir.glob('lib/**/*.rb') + + ::Dir.glob('*.md') + + ['LICENSE'] + spec.require_paths = ['lib'] + spec.required_ruby_version = '>= 2.4.0' + + spec.add_dependency 'opentelemetry-api', '~> 0.0' + + spec.add_development_dependency 'appraisal', '~> 2.2.0' + spec.add_development_dependency 'bundler', '~> 2.0.2' + spec.add_development_dependency 'minitest', '~> 5.0' + spec.add_development_dependency 'opentelemetry-sdk', '~> 0.0' + spec.add_development_dependency 'rack', '~> 2.0.8' + spec.add_development_dependency 'rack-test', '~> 1.1.0' + spec.add_development_dependency 'rake', '~> 12.3.3' + spec.add_development_dependency 'simplecov', '~> 0.17.1' + spec.add_development_dependency 'webmock', '~> 3.7.6' +end From 367c78c59a45ef923af50160251e02f2e549a612 Mon Sep 17 00:00:00 2001 From: David Davis Date: Mon, 6 Jan 2020 15:50:24 -0800 Subject: [PATCH 02/64] Add Rakefile e.g., $ bundle exec rake test --- adapters/rack/Rakefile | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 adapters/rack/Rakefile diff --git a/adapters/rack/Rakefile b/adapters/rack/Rakefile new file mode 100644 index 000000000..4919bebd5 --- /dev/null +++ b/adapters/rack/Rakefile @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +# Copyright 2019 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +require 'bundler/gem_tasks' +require 'rake/testtask' + +Rake::TestTask.new :test do |t| + t.libs << 'test' + t.libs << 'lib' + t.test_files = FileList['test/**/*_test.rb'] +end \ No newline at end of file From 6f5c5e16bcdf3ecb3329f5010325d141b0f9667f Mon Sep 17 00:00:00 2001 From: David Davis Date: Mon, 6 Jan 2020 13:02:59 -0800 Subject: [PATCH 03/64] tests: Add test_helper --- adapters/rack/test/test_helper.rb | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 adapters/rack/test/test_helper.rb diff --git a/adapters/rack/test/test_helper.rb b/adapters/rack/test/test_helper.rb new file mode 100644 index 000000000..188e350fe --- /dev/null +++ b/adapters/rack/test/test_helper.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +# Copyright 2020 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +require 'rack' + +require 'opentelemetry/sdk' + +require 'minitest/autorun' +require 'webmock/minitest' + +# global opentelemetry-sdk setup: +sdk = OpenTelemetry::SDK +exporter = sdk::Trace::Export::InMemorySpanExporter.new +span_processor = sdk::Trace::Export::SimpleSpanProcessor.new(exporter) +OpenTelemetry.tracer_factory = sdk::Trace::TracerFactory.new.tap do |factory| + factory.add_span_processor(span_processor) +end + +EXPORTER = exporter From 3ed377c4a58b28c794c918495113dcfce4771817 Mon Sep 17 00:00:00 2001 From: David Davis Date: Mon, 6 Jan 2020 15:49:59 -0800 Subject: [PATCH 04/64] Add example: trace_demonstration e.g., $ docker-compose run --rm ex-adapter-rack bash-5.0$ ruby trace_demonstration.rb --- adapters/rack/example/Gemfile | 8 +++++ adapters/rack/example/trace_demonstration.rb | 38 ++++++++++++++++++++ docker-compose.yml | 4 +++ 3 files changed, 50 insertions(+) create mode 100644 adapters/rack/example/Gemfile create mode 100644 adapters/rack/example/trace_demonstration.rb diff --git a/adapters/rack/example/Gemfile b/adapters/rack/example/Gemfile new file mode 100644 index 000000000..837779aa3 --- /dev/null +++ b/adapters/rack/example/Gemfile @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +source 'https://rubygems.org' + +gem 'rack' +gem 'rack-test' +gem 'opentelemetry-api', path: '../../../api' +gem 'opentelemetry-sdk', path: '../../../sdk' diff --git a/adapters/rack/example/trace_demonstration.rb b/adapters/rack/example/trace_demonstration.rb new file mode 100644 index 000000000..621be2a7f --- /dev/null +++ b/adapters/rack/example/trace_demonstration.rb @@ -0,0 +1,38 @@ +require 'rubygems' +require 'bundler/setup' + +require 'opentelemetry/sdk' + +require 'rack' +require_relative '../lib/opentelemetry/adapters/rack' + +# Set preferred tracer implementation: +SDK = OpenTelemetry::SDK + +# global initialization: +factory = OpenTelemetry.tracer_factory = SDK::Trace::TracerFactory.new +factory.add_span_processor( + SDK::Trace::Export::SimpleSpanProcessor.new( + SDK::Trace::Export::ConsoleSpanExporter.new + ) +) + +# setup fake rack application: +builder = Rack::Builder.new do + # can't use TracerMiddleware constant before calling Adapters::Rack.install: + #use OpenTelemetry::Adapters::Rack::Middlewares::TracerMiddleware +end +app = lambda { |env| [200, {'Content-Type' => 'text/plain'}, ['All responses are OK']] } +builder.run app + +# demonstrate rack configuration options: +config = {} +config[:retain_middleware_names] = true +config[:application] = builder +config[:record_frontend_span] = true +OpenTelemetry::Adapters::Rack.install(config) + +# integrate/activate tracing middleware: +builder.use OpenTelemetry::Adapters::Rack::Middlewares::TracerMiddleware + +puts Rack::MockRequest.new(builder).get('/') diff --git a/docker-compose.yml b/docker-compose.yml index e288ee399..7ddf2a822 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -28,6 +28,10 @@ services: <<: *base working_dir: /app/adapters/faraday/example + ex-adapter-rack: + <<: *base + working_dir: /app/adapters/rack/example + ex-adapter-sinatra: <<: *base working_dir: /app/adapters/sinatra/example From acb435c24765bbffb19134e22703e9dbb2c3c0a7 Mon Sep 17 00:00:00 2001 From: David Davis Date: Mon, 13 Jan 2020 11:15:23 -0800 Subject: [PATCH 05/64] Add QueueTime * from dd-trace-rb * translate spec to minitest --- .../adapters/rack/util/queue_time.rb | 47 ++++++++++ .../adapters/rack/util/queue_time_test.rb | 90 +++++++++++++++++++ 2 files changed, 137 insertions(+) create mode 100644 adapters/rack/lib/opentelemetry/adapters/rack/util/queue_time.rb create mode 100644 adapters/rack/test/opentelemetry/adapters/rack/util/queue_time_test.rb diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/util/queue_time.rb b/adapters/rack/lib/opentelemetry/adapters/rack/util/queue_time.rb new file mode 100644 index 000000000..bfe549c53 --- /dev/null +++ b/adapters/rack/lib/opentelemetry/adapters/rack/util/queue_time.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +# Copyright 2020 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +module OpenTelemetry + module Adapters + module Rack + module Util + # QueueTime simply... + module QueueTime + REQUEST_START = 'HTTP_X_REQUEST_START'.freeze + QUEUE_START = 'HTTP_X_QUEUE_START'.freeze + MINIMUM_ACCEPTABLE_TIME_VALUE = 1_000_000_000 + + module_function + + def get_request_start(env, now = Time.now.utc) + header = env[REQUEST_START] || env[QUEUE_START] + return unless header + + # nginx header is seconds in the format "t=1512379167.574" + # apache header is microseconds in the format "t=1570633834463123" + # heroku header is milliseconds in the format "1570634024294" + time_string = header.to_s.delete('^0-9') + return if time_string.nil? + + # Return nil if the time is clearly invalid + time_value = "#{time_string[0, 10]}.#{time_string[10, 6]}".to_f + return if time_value.zero? || time_value < MINIMUM_ACCEPTABLE_TIME_VALUE + + # return the request_start only if it's lesser than + # current time, to avoid significant clock skew + request_start = Time.at(time_value) + request_start.utc > now ? nil : request_start + rescue StandardError => e + # in case of an Exception we don't create a + # `request.queuing` span + OpenTelemetry.logger.debug("[rack] unable to parse request queue headers: #{e}") + nil + end + end + end + end + end +end diff --git a/adapters/rack/test/opentelemetry/adapters/rack/util/queue_time_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/util/queue_time_test.rb new file mode 100644 index 000000000..25f029208 --- /dev/null +++ b/adapters/rack/test/opentelemetry/adapters/rack/util/queue_time_test.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true + +# Copyright 2020 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +require 'test_helper' + +require 'rack' +require_relative '../../../../../lib/opentelemetry/adapters/rack/util/queue_time' + +describe OpenTelemetry::Adapters::Rack::Util::QueueTime do + let(:described_class) { OpenTelemetry::Adapters::Rack::Util::QueueTime } + + describe '#get_request_start' do + let(:request_start) { described_class.get_request_start(env) } + + describe 'given a Rack env with' do + describe 'milliseconds' do + describe 'REQUEST_START' do + let(:env) { { described_class::REQUEST_START => "t=#{expected}" } } + let(:expected) { 1512379167.574 } + it { expect(request_start.to_f).must_equal(expected) } + + describe 'but does not start with t=' do + let(:env) { { described_class::REQUEST_START => expected } } + it { expect(request_start.to_f).must_equal(expected) } + end + + describe 'without decimal places' do + let(:env) { { described_class::REQUEST_START => expected } } + let(:expected) { 1512379167574 } + it { expect(request_start.to_f).must_equal(1512379167.574) } + end + + describe 'but a malformed expected' do + let(:expected) { 'foobar' } + it { _(request_start).must_be_nil } + end + + describe 'before the start of the acceptable time range' do + let(:expected) { 999_999_999.000 } + it { _(request_start).must_be_nil } + end + end + + describe 'QUEUE_START' do + let(:env) { { described_class::QUEUE_START => "t=#{expected}" } } + let(:expected) { 1512379167.574 } + it { expect(request_start.to_f).must_equal(expected) } + end + end + + describe 'microseconds' do + describe 'REQUEST_START' do + let(:env) { { described_class::REQUEST_START => "t=#{expected}" } } + let(:expected) { 1570633834.463123 } + it { expect(request_start.to_f).must_equal(expected) } + + describe 'but does not start with t=' do + let(:env) { { described_class::REQUEST_START => expected } } + it { expect(request_start.to_f).must_equal(expected) } + end + + describe 'without decimal places' do + let(:env) { { described_class::REQUEST_START => expected } } + let(:expected) { 1570633834463123 } + it { expect(request_start.to_f).must_equal(1570633834.463123) } + end + + describe 'but a malformed expected' do + let(:expected) { 'foobar' } + it { _(request_start).must_be_nil } + end + end + + describe 'QUEUE_START' do + let(:env) { { described_class::QUEUE_START => "t=#{expected}" } } + let(:expected) { 1570633834.463123 } + it { expect(request_start.to_f).must_equal(expected) } + end + end + + describe 'nothing' do + let(:env) { {} } + it { _(request_start).must_be_nil } + end + end + end +end From 384e33192c211bbc771dd31320c14bf144b96fa1 Mon Sep 17 00:00:00 2001 From: David Davis Date: Tue, 17 Dec 2019 16:44:10 -0800 Subject: [PATCH 06/64] Add TracerMiddleware * use dup._call env pattern * RE: thread safety: "... easiest and most efficient way to do this is to dup the middleware object that is created in runtime." * do we need 'quantization' now? --- .../rack/middlewares/tracer_middleware.rb | 248 ++++++++++++++++++ .../middlewares/tracer_middleware_test.rb | 136 ++++++++++ 2 files changed, 384 insertions(+) create mode 100644 adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb create mode 100644 adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb new file mode 100644 index 000000000..bb4b207c2 --- /dev/null +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -0,0 +1,248 @@ +# frozen_string_literal: true + +# Copyright 2020 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +require_relative '../util/queue_time' + +module OpenTelemetry + module Adapters + module Rack + module Middlewares + # TODO: implement 'resource name' + class TracerMiddleware + def initialize(app) + @app = app + end + + def call(env) + # since middleware may only be initialized once (memoized), + # but we want to be sure to have a clean slate on each request: + dup._call env + end + + def _call(env) + @env = env + @original_env = env.dup + + frontend_span.start if record_frontend_span? + extract_parent_context if extract_parent_context? + + start_request_span + app.call(env).tap do |status, headers, response| + set_attributes_after_request(request_span, status, headers, response) + end + rescue Exception => e + record_and_reraise_error(e) + ensure + request_span.finish + frontend_span.finish if record_frontend_span? + end + + private + + attr_reader :app, + :env, + :original_env, + :parent_context, + :request_span + + ### parent context + + def extract_parent_context? + # TODO: compare to ':distributed_tracing' option name/semantics + Rack::Adapter.config[:extract_parent_context] + end + + def extract_parent_context + @parent_context ||= OpenTelemetry::Adapters::Rack::Adapter.propagator.extract(env) + end + + def tracer + OpenTelemetry::Adapters::Rack::Adapter.tracer + end + + ### request_span + + # Sets as many attributes as are available before control + # is handed off to next middleware. + def start_request_span + @request_span ||= tracer.start_span(request_span_name, + with_parent_context: parent_context, + # NOTE: try to set as many attributes via 'attributes' argument + # instead of via span.set_attribute + attributes: request_span_attributes, + kind: :server) + end + + # Per https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-http.md#http-server-semantic-conventions + # + # One of the following sets is required, in descending preference: + # * http.scheme, http.host, http.target + # * http.scheme, http.server_name, net.host.port, http.target + # * http.scheme, net.host.name, net.host.port, http.target + # * http.url + def request_span_attributes + { + 'component' => 'http', + 'http.method' => env['REQUEST_METHOD'], + 'http.url' => full_http_request_url, + 'http.host' => env['HOST'], + 'http.scheme' => env['rack.url_scheme'], + 'http.base_url' => base_url # TODO: check attribute naming/semantics + }.merge(allowed_request_headers) + end + + def full_http_request_url + # NOTE: dd-trace-rb has implemented 'quantization'? (which lowers url cardinality) + # see Datadog::Quantization::HTTP.url + + rack_request.url + end + + # https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-http.md#name + # + def request_span_name + # uri_path_value: + env['PATH_INFO'] + end + + def base_url + if rack_request.respond_to?(:base_url) + rack_request.base_url + else + # Compatibility for older Rack versions + rack_request.url.chomp(rack_request.fullpath) + end + end + + def rack_request + @rack_request ||= ::Rack::Request.new(env) + end + + def record_and_reraise_error(error) + # TODO: implement span.set_error? + + # catch exceptions that may be raised in the middleware chain + # Note: if a middleware catches an Exception without re raising, + # the Exception cannot be recorded here. + request_span.set_error(error) unless request_span.nil? + raise error + end + + def set_attributes_after_request(span, status, headers, _response) + span.status = OpenTelemetry::Trace::Status.http_to_status(status) + span.set_attribute('http.status_code', status) + + # TODO: set_attribute('http.route', ... if it's available + span.set_attribute('http.status_text', ::Rack::Utils::HTTP_STATUS_CODES[status]) + + allowed_response_headers(headers).each { |k, v| span.set_attribute(k, v) } + end + + # @return Hash + def allowed_request_headers + {}.tap do |result| + Array(allowed_request_header_names).each do |header| + rack_header = "HTTP_#{header.to_s.upcase.gsub(/[-\s]/, '_')}" + if env.key?(rack_header) + result[build_attribute_name('http.request.headers.', header)] = env[rack_header] + end + end + end + end + + def allowed_request_header_names + Array(config[:allowed_request_headers]) + end + + # @return Hash + def allowed_response_headers(headers) + return {} if headers.nil? + + {}.tap do |result| + Array(allowed_response_header_names).each do |header| + new_key_name = build_attribute_name('http.response.headers.', header) + if headers.key?(header) + result[new_key_name] = headers[header] + else + # Try a case-insensitive lookup + uppercased_header = header.to_s.upcase + matching_header = headers.keys.find { |h| h.upcase == uppercased_header } + if matching_header + result[new_key_name] = headers[matching_header] + end + end + end + end + end + + def allowed_response_header_names + Array(config[:allowed_response_headers]) + end + + def build_attribute_name(prefix, suffix) + prefix + suffix.to_s.downcase.gsub(/[-\s]/, '_') + end + + def config + Rack::Adapter.config + end + + ### frontend span + + def record_frontend_span? + Rack::Adapter.config[:record_frontend_span] && + frontend_span.recordable? + end + + def frontend_span + @frontend_span ||= FrontendSpan.new(env: env, + tracer: tracer, + parent_context: parent_context) + end + + class FrontendSpan + def initialize(env:, tracer:, parent_context:) + @env = env + @tracer = tracer + @parent_context = parent_context + + # NOTE: get_request_start may return nil + @request_start_time = OpenTelemetry::Adapters::Rack::Util::QueueTime.get_request_start(env) + end + + def recordable? + !request_start_time.nil? + end + + def start + @span ||= tracer.start_span('http_server.queue', # TODO: check this name + with_parent_context: parent_context, # TODO: check for correct parent + # NOTE: initialize with as many attributes as possible: + attributes: { + 'component' => 'http', + # TODO: 'service' => config[:web_service_name] + # TODO: 'span_type' => PROXY + 'start_time' => request_start_time + }) + end + + def finish + span&.finish + end + + private + + attr_reader :env, + :parent_context, + :request_start_time, + :span, + :tracer + end + end + end + end + end +end diff --git a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb new file mode 100644 index 000000000..28d1aaa15 --- /dev/null +++ b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb @@ -0,0 +1,136 @@ +# frozen_string_literal: true + +# Copyright 2020 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +require 'test_helper' + + #require Adapter so .install method is found: +require_relative '../../../../../lib/opentelemetry/adapters/rack' +require_relative '../../../../../lib/opentelemetry/adapters/rack/adapter' +require_relative '../../../../../lib/opentelemetry/adapters/rack/middlewares/tracer_middleware' + +describe OpenTelemetry::Adapters::Rack::Middlewares::TracerMiddleware do + let(:adapter_module) { OpenTelemetry::Adapters::Rack } + let(:adapter) { adapter_module::Adapter } + let(:app) { lambda { |env| [200, {'Content-Type' => 'text/plain'}, ['OK']] } } + let(:described_class) { OpenTelemetry::Adapters::Rack::Middlewares::TracerMiddleware } + let(:exporter) { EXPORTER } + + let(:rack_builder) { Rack::Builder.new } + + let(:middleware) { described_class.new(app) } + let(:first_span) { exporter.finished_spans.first } + + let(:default_config) { Hash.new } + let(:config) { default_config } + let(:env) { Hash.new } + + before do + adapter_module.install(config) + exporter.reset + + rack_builder.run app + rack_builder.use described_class + end + + after do + # installation is 'global', so it should be reset: + adapter.instance_variable_set('@installed', false) + adapter.install(default_config) + adapter.instance_variable_set('@installed', false) + end + + describe '#call' do + before do + Rack::MockRequest.new(rack_builder).get('/', env) + end + + it 'records attributes' do + _(first_span.attributes['component']).must_equal 'http' + _(first_span.attributes['http.method']).must_equal 'GET' + _(first_span.attributes['http.status_code']).must_equal 200 + _(first_span.attributes['http.status_text']).must_equal 'OK' + _(first_span.status.canonical_code).must_equal OpenTelemetry::Trace::Status::OK + _(first_span.attributes['http.url']).must_equal 'http://example.org/' + _(first_span.name).must_equal '/' + end + + describe 'config[:allowed_request_headers]' do + let(:env) { Hash('HTTP_FOO_BAR' => 'http foo bar value') } + + it 'defaults to nil' do + _(first_span.attributes['http.request.headers.foo_bar']).must_be_nil + end + + describe 'when configured' do + let(:config) { default_config.merge(allowed_request_headers: ['foo_BAR']) } + + it 'returns attribute' do + _(first_span.attributes['http.request.headers.foo_bar']).must_equal 'http foo bar value' + end + end + end + + describe 'config[:allowed_response_headers]' do + let(:app) do + lambda { |env| [200, {'Foo-Bar' => 'foo bar response header'}, ['OK']] } + end + + it 'defaults to nil' do + _(first_span.attributes['http.response.headers.foo_bar']).must_be_nil + end + + describe 'when configured' do + let(:config) { default_config.merge(allowed_response_headers: ['Foo-Bar']) } + + it 'returns attribute' do + _(first_span.attributes['http.response.headers.foo_bar']).must_equal 'foo bar response header' + end + + describe "case-sensitively" do + let(:config) { default_config.merge(allowed_response_headers: ['fOO-bAR']) } + + it 'returns attribute' do + _(first_span.attributes['http.response.headers.foo_bar']).must_equal 'foo bar response header' + end + end + end + end + + describe 'config[:extract_parent_context]' do + describe 'default' do + it 'starts a trace without parent context' do + _(first_span.parent_span_id).must_equal '0000000000000000' + end + end + + describe 'when true' do + let(:config) { default_config.merge(extract_parent_context: true) } + + it 'extracts parent context' do + _(first_span.parent_span_id).wont_equal '0000000000000000' + end + end + end + + describe 'config[:record_frontend_span]' do + describe 'default' do + it 'does not record span' do + _(exporter.finished_spans.size).must_equal 1 + end + end + + describe 'when recordable' do + let(:config) { default_config.merge(record_frontend_span: true)} + let(:env) { Hash('HTTP_X_REQUEST_START' => Time.now.to_i) } + + it 'records span' do + _(exporter.finished_spans.size).must_equal 2 + _(exporter.finished_spans.last.name).must_equal 'http_server.queue' + end + end + end + end +end From 6a6fd0aec77ba1e5631cf9562c85210d08a4226e Mon Sep 17 00:00:00 2001 From: David Davis Date: Mon, 6 Jan 2020 12:04:28 -0800 Subject: [PATCH 07/64] Add Adapters::Rack::Adapter * retain middleware names (feature exists in dd-trace-rb) --- .../opentelemetry/adapters/rack/adapter.rb | 77 +++++++++++++++ .../adapters/rack/adapter_test.rb | 93 +++++++++++++++++++ 2 files changed, 170 insertions(+) create mode 100644 adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb create mode 100644 adapters/rack/test/opentelemetry/adapters/rack/adapter_test.rb diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb b/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb new file mode 100644 index 000000000..124e54de3 --- /dev/null +++ b/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +# Copyright 2020 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +module OpenTelemetry + module Adapters + module Rack + class Adapter + class << self + attr_reader :config, + :propagator + + def install(config = {}) + @config = config + @propagator = OpenTelemetry.tracer_factory.http_text_format + + new.install + end + + def tracer + @tracer ||= OpenTelemetry::tracer_factory.tracer( + Rack.name, + Rack.version + ) + end + + attr_accessor :installed + alias_method :installed?, :installed + end + + def install + return :already_installed if self.class.installed? + + require_relative 'middlewares/tracer_middleware' + + retain_middleware_names if config[:retain_middleware_names] + + self.class.installed = true + end + + private + + MissingApplicationError = Class.new(StandardError) + + def config + self.class.config + end + + # intercept all middleware-compatible calls, retain class name + def retain_middleware_names + next_middleware = config[:application] + raise MissingApplicationError unless next_middleware + + while next_middleware + if next_middleware.respond_to?(:call) + next_middleware.singleton_class.class_eval do + alias_method :__call, :call + + def call(env) + env['RESPONSE_MIDDLEWARE'] = self.class.to_s + __call(env) + end + end + end + + next_middleware = next_middleware.instance_variable_defined?('@app') && + next_middleware.instance_variable_get('@app') + end + end + end + end + end +end + +require_relative '../rack' diff --git a/adapters/rack/test/opentelemetry/adapters/rack/adapter_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/adapter_test.rb new file mode 100644 index 000000000..e5e8cb621 --- /dev/null +++ b/adapters/rack/test/opentelemetry/adapters/rack/adapter_test.rb @@ -0,0 +1,93 @@ +# frozen_string_literal: true + +# Copyright 2020 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +require 'test_helper' + +require_relative '../../../../lib/opentelemetry/adapters/rack/adapter' + +describe OpenTelemetry::Adapters::Rack::Adapter do + let(:adapter) { OpenTelemetry::Adapters::Rack::Adapter } + + after do + # installation is 'global', so reset as much as possible: + adapter.instance_variable_set('@installed', false) + adapter.install({}) + adapter.instance_variable_set('@installed', false) + end + + describe 'installation' do + it 'gets installed' do + _(adapter.install).must_equal true + end + + it 'only installs once' do + adapter.install + + _(adapter.install).must_equal :already_installed + end + end + + describe 'defaults' do + before do + adapter.install + end + + it 'has propagator' do + _(adapter.propagator).wont_be_nil + end + + it 'has tracer' do + _(adapter.tracer).wont_be_nil + end + end + + describe 'config[:retain_middleware_names]' do + let(:config) { Hash(retain_middleware_names: true) } + + describe 'without config[:application]' do + it 'raises error' do + assert_raises adapter::MissingApplicationError do + adapter.install(config) + end + end + end + + describe 'default' do + class MyAppClass + attr_reader :env + + def call(env) + @env = env + [200, {'Content-Type' => 'text/plain'}, ['OK']] + end + end + + let(:app) { MyAppClass.new } + + describe 'without config' do + it 'does not set RESPONSE_MIDDLEWARE' do + app.call({}) + + _(app.env['RESPONSE_MIDDLEWARE']).must_be_nil + end + end + + describe 'with config[:application]' do + let(:config) do + { retain_middleware_names: true, + application: app } + end + + it 'retains RESPONSE_MIDDLEWARE after .call' do + adapter.install(config) + app.call({}) + + _(app.env['RESPONSE_MIDDLEWARE']).must_equal 'MyAppClass' + end + end + end + end +end From 16e8d32be9a95e8c0f297c1c564e2a937e3904a5 Mon Sep 17 00:00:00 2001 From: David Davis Date: Mon, 6 Jan 2020 12:59:04 -0800 Subject: [PATCH 08/64] Add Adapters::Rack --- .../rack/lib/opentelemetry/adapters/rack.rb | 30 +++++++++++++++++++ .../test/opentelemetry/adapters/rack_test.rb | 28 +++++++++++++++++ 2 files changed, 58 insertions(+) create mode 100644 adapters/rack/lib/opentelemetry/adapters/rack.rb create mode 100644 adapters/rack/test/opentelemetry/adapters/rack_test.rb diff --git a/adapters/rack/lib/opentelemetry/adapters/rack.rb b/adapters/rack/lib/opentelemetry/adapters/rack.rb new file mode 100644 index 000000000..5c165b5e6 --- /dev/null +++ b/adapters/rack/lib/opentelemetry/adapters/rack.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +# Copyright 2020 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +module OpenTelemetry + module Adapters + module Rack + module_function + + def install(config = {}) + require_relative 'rack/adapter' + Rack::Adapter.install(config) + end + + # Convenience method to access the nested module name + def name + Module.nesting[0].to_s + end + + # Convenience method to access the adapter version + def version + VERSION + end + end + end +end + +require_relative './rack/version' diff --git a/adapters/rack/test/opentelemetry/adapters/rack_test.rb b/adapters/rack/test/opentelemetry/adapters/rack_test.rb new file mode 100644 index 000000000..6b0ac748e --- /dev/null +++ b/adapters/rack/test/opentelemetry/adapters/rack_test.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +# Copyright 2020 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +require 'test_helper' + +require_relative '../../../lib/opentelemetry/adapters/rack' + +describe OpenTelemetry::Adapters::Rack do + let(:adapter) { OpenTelemetry::Adapters::Rack } + + it 'has #name' do + _(adapter.name).must_equal 'OpenTelemetry::Adapters::Rack' + end + + it 'has #version' do + _(adapter.version).wont_be_nil + _(adapter.version).wont_be_empty + end + + describe '#install' do + it 'accepts argument' do + adapter.install({}) + end + end +end From 4da0f61c8d053d5f89bec6d7891b61da879ed7c2 Mon Sep 17 00:00:00 2001 From: David Davis Date: Tue, 21 Jan 2020 11:59:30 -0800 Subject: [PATCH 09/64] Update to 2020 copyright --- adapters/rack/Gemfile | 2 +- adapters/rack/Rakefile | 4 ++-- adapters/rack/lib/opentelemetry/adapters/rack/version.rb | 2 +- adapters/rack/opentelemetry-adapters-rack.gemspec | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/adapters/rack/Gemfile b/adapters/rack/Gemfile index a022b2023..8af0cd4fc 100644 --- a/adapters/rack/Gemfile +++ b/adapters/rack/Gemfile @@ -1,6 +1,6 @@ # frozen_string_literal: true -# Copyright 2019 OpenTelemetry Authors +# Copyright 2020 OpenTelemetry Authors # # SPDX-License-Identifier: Apache-2.0 diff --git a/adapters/rack/Rakefile b/adapters/rack/Rakefile index 4919bebd5..59f0fddd5 100644 --- a/adapters/rack/Rakefile +++ b/adapters/rack/Rakefile @@ -1,6 +1,6 @@ # frozen_string_literal: true -# Copyright 2019 OpenTelemetry Authors +# Copyright 2020 OpenTelemetry Authors # # SPDX-License-Identifier: Apache-2.0 @@ -11,4 +11,4 @@ Rake::TestTask.new :test do |t| t.libs << 'test' t.libs << 'lib' t.test_files = FileList['test/**/*_test.rb'] -end \ No newline at end of file +end diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/version.rb b/adapters/rack/lib/opentelemetry/adapters/rack/version.rb index 4d3816493..89afa11ba 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/version.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/version.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -# Copyright 2019 OpenTelemetry Authors +# Copyright 2020 OpenTelemetry Authors # # SPDX-License-Identifier: Apache-2.0 diff --git a/adapters/rack/opentelemetry-adapters-rack.gemspec b/adapters/rack/opentelemetry-adapters-rack.gemspec index 7500071b4..7f67534dd 100644 --- a/adapters/rack/opentelemetry-adapters-rack.gemspec +++ b/adapters/rack/opentelemetry-adapters-rack.gemspec @@ -1,6 +1,6 @@ # frozen_string_literal: true -# Copyright 2019 OpenTelemetry Authors +# Copyright 2020 OpenTelemetry Authors # # SPDX-License-Identifier: Apache-2.0 From b62e0faffc738f1b6174e67b67205db8cac385bf Mon Sep 17 00:00:00 2001 From: David Davis Date: Tue, 21 Jan 2020 12:01:58 -0800 Subject: [PATCH 10/64] Initialize 'now' later --- .../rack/lib/opentelemetry/adapters/rack/util/queue_time.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/util/queue_time.rb b/adapters/rack/lib/opentelemetry/adapters/rack/util/queue_time.rb index bfe549c53..fada6b398 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/util/queue_time.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/util/queue_time.rb @@ -16,7 +16,7 @@ module QueueTime module_function - def get_request_start(env, now = Time.now.utc) + def get_request_start(env, now = nil) header = env[REQUEST_START] || env[QUEUE_START] return unless header @@ -33,6 +33,7 @@ def get_request_start(env, now = Time.now.utc) # return the request_start only if it's lesser than # current time, to avoid significant clock skew request_start = Time.at(time_value) + now ||= Time.now.utc request_start.utc > now ? nil : request_start rescue StandardError => e # in case of an Exception we don't create a From 5e2be0054347dbcbc6777b041d98432df86647c3 Mon Sep 17 00:00:00 2001 From: David Davis Date: Tue, 21 Jan 2020 21:20:25 -0800 Subject: [PATCH 11/64] Adapt to Instrumentation Auto Installer interface * PR #164 --- .../lib/opentelemetry/adapters/adapters.rb | 16 +++++++ .../rack/lib/opentelemetry/adapters/rack.rb | 19 ++------ .../opentelemetry/adapters/rack/adapter.rb | 39 ++++------------- .../rack/middlewares/tracer_middleware.rb | 10 ++--- .../adapters/rack/adapter_test.rb | 43 ++++++------------- .../middlewares/tracer_middleware_test.rb | 13 ++++-- .../test/opentelemetry/adapters/rack_test.rb | 2 +- 7 files changed, 56 insertions(+), 86 deletions(-) create mode 100644 adapters/rack/lib/opentelemetry/adapters/adapters.rb diff --git a/adapters/rack/lib/opentelemetry/adapters/adapters.rb b/adapters/rack/lib/opentelemetry/adapters/adapters.rb new file mode 100644 index 000000000..c9446df58 --- /dev/null +++ b/adapters/rack/lib/opentelemetry/adapters/adapters.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +# Copyright 2019 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +module OpenTelemetry + # "Instrumentation adapters" are specified by + # https://github.com/open-telemetry/opentelemetry-specification/blob/57714f7547fe4dcb342ad0ad10a80d86118431c7/specification/overview.md#instrumentation-adapters + # + # Adapters should be able to handle the case when the library is not installed on a user's system. + module Adapters + end +end + +require_relative './adapters/rack' diff --git a/adapters/rack/lib/opentelemetry/adapters/rack.rb b/adapters/rack/lib/opentelemetry/adapters/rack.rb index 5c165b5e6..e2d7716d9 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack.rb @@ -4,27 +4,14 @@ # # SPDX-License-Identifier: Apache-2.0 +require 'opentelemetry' + module OpenTelemetry module Adapters module Rack - module_function - - def install(config = {}) - require_relative 'rack/adapter' - Rack::Adapter.install(config) - end - - # Convenience method to access the nested module name - def name - Module.nesting[0].to_s - end - - # Convenience method to access the adapter version - def version - VERSION - end end end end +require_relative './rack/adapter' require_relative './rack/version' diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb b/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb index 124e54de3..8869d1a2c 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb @@ -7,47 +7,26 @@ module OpenTelemetry module Adapters module Rack - class Adapter - class << self - attr_reader :config, - :propagator + class Adapter < OpenTelemetry::Instrumentation::Adapter - def install(config = {}) - @config = config - @propagator = OpenTelemetry.tracer_factory.http_text_format + install do |config| + require_dependencies - new.install - end - - def tracer - @tracer ||= OpenTelemetry::tracer_factory.tracer( - Rack.name, - Rack.version - ) - end + retain_middleware_names if config[:retain_middleware_names] + end - attr_accessor :installed - alias_method :installed?, :installed + present do + Gem.loaded_specs.include?('rack') end - def install - return :already_installed if self.class.installed? + private + def require_dependencies require_relative 'middlewares/tracer_middleware' - - retain_middleware_names if config[:retain_middleware_names] - - self.class.installed = true end - private - MissingApplicationError = Class.new(StandardError) - def config - self.class.config - end - # intercept all middleware-compatible calls, retain class name def retain_middleware_names next_middleware = config[:application] diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index bb4b207c2..09842173a 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -52,15 +52,15 @@ def _call(env) def extract_parent_context? # TODO: compare to ':distributed_tracing' option name/semantics - Rack::Adapter.config[:extract_parent_context] + config[:extract_parent_context] end def extract_parent_context - @parent_context ||= OpenTelemetry::Adapters::Rack::Adapter.propagator.extract(env) + @parent_context ||= OpenTelemetry.tracer_factory.http_text_format.extract(env) end def tracer - OpenTelemetry::Adapters::Rack::Adapter.tracer + OpenTelemetry::Adapters::Rack::Adapter.instance.tracer end ### request_span @@ -187,13 +187,13 @@ def build_attribute_name(prefix, suffix) end def config - Rack::Adapter.config + Rack::Adapter.instance.config end ### frontend span def record_frontend_span? - Rack::Adapter.config[:record_frontend_span] && + Rack::Adapter.instance.config[:record_frontend_span] && frontend_span.recordable? end diff --git a/adapters/rack/test/opentelemetry/adapters/rack/adapter_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/adapter_test.rb index e5e8cb621..ba6043e97 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/adapter_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/adapter_test.rb @@ -9,39 +9,14 @@ require_relative '../../../../lib/opentelemetry/adapters/rack/adapter' describe OpenTelemetry::Adapters::Rack::Adapter do - let(:adapter) { OpenTelemetry::Adapters::Rack::Adapter } + let(:adapter_class) { OpenTelemetry::Adapters::Rack::Adapter } + let(:adapter) { adapter_class.instance } + let(:config) { Hash.new } after do - # installation is 'global', so reset as much as possible: + # simulate a fresh install: adapter.instance_variable_set('@installed', false) - adapter.install({}) - adapter.instance_variable_set('@installed', false) - end - - describe 'installation' do - it 'gets installed' do - _(adapter.install).must_equal true - end - - it 'only installs once' do - adapter.install - - _(adapter.install).must_equal :already_installed - end - end - - describe 'defaults' do - before do - adapter.install - end - - it 'has propagator' do - _(adapter.propagator).wont_be_nil - end - - it 'has tracer' do - _(adapter.tracer).wont_be_nil - end + adapter.install(Hash.new) end describe 'config[:retain_middleware_names]' do @@ -49,7 +24,10 @@ describe 'without config[:application]' do it 'raises error' do - assert_raises adapter::MissingApplicationError do + # allow for re-installation with new config: + adapter.instance_variable_set('@installed', false) + + assert_raises adapter_class::MissingApplicationError do adapter.install(config) end end @@ -82,6 +60,9 @@ def call(env) end it 'retains RESPONSE_MIDDLEWARE after .call' do + # allow for re-installation with new config: + adapter.instance_variable_set('@installed', false) + adapter.install(config) app.call({}) diff --git a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb index 28d1aaa15..504a6b83b 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb @@ -13,7 +13,8 @@ describe OpenTelemetry::Adapters::Rack::Middlewares::TracerMiddleware do let(:adapter_module) { OpenTelemetry::Adapters::Rack } - let(:adapter) { adapter_module::Adapter } + let(:adapter_class) { adapter_module::Adapter } + let(:adapter) { adapter_class.instance } let(:app) { lambda { |env| [200, {'Content-Type' => 'text/plain'}, ['OK']] } } let(:described_class) { OpenTelemetry::Adapters::Rack::Middlewares::TracerMiddleware } let(:exporter) { EXPORTER } @@ -27,10 +28,17 @@ let(:config) { default_config } let(:env) { Hash.new } + let(:registry) { OpenTelemetry::Instrumentation::Registry.new } + before do - adapter_module.install(config) + # clear captured spans: exporter.reset + # simulate a fresh install: + adapter.instance_variable_set('@installed', false) + adapter.install(config) + + # integrate tracer middleware: rack_builder.run app rack_builder.use described_class end @@ -39,7 +47,6 @@ # installation is 'global', so it should be reset: adapter.instance_variable_set('@installed', false) adapter.install(default_config) - adapter.instance_variable_set('@installed', false) end describe '#call' do diff --git a/adapters/rack/test/opentelemetry/adapters/rack_test.rb b/adapters/rack/test/opentelemetry/adapters/rack_test.rb index 6b0ac748e..284bf606c 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack_test.rb @@ -9,7 +9,7 @@ require_relative '../../../lib/opentelemetry/adapters/rack' describe OpenTelemetry::Adapters::Rack do - let(:adapter) { OpenTelemetry::Adapters::Rack } + let(:adapter) { OpenTelemetry::Adapters::Rack::Adapter.instance } it 'has #name' do _(adapter.name).must_equal 'OpenTelemetry::Adapters::Rack' From 423397a16de20b6663d18cd5f402f2174bff4b81 Mon Sep 17 00:00:00 2001 From: David Davis Date: Wed, 22 Jan 2020 15:38:38 -0800 Subject: [PATCH 12/64] Fix example to use updated Instrumentation Auto Installer * verified by running via, e.g., $ docker-compose run --rm ex-adapter-rack bash-5.0$ bundle bash-5.0$ ruby trace_demonstration.rb Expected: console output of span data --- adapters/rack/example/trace_demonstration.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/adapters/rack/example/trace_demonstration.rb b/adapters/rack/example/trace_demonstration.rb index 621be2a7f..9e762c812 100644 --- a/adapters/rack/example/trace_demonstration.rb +++ b/adapters/rack/example/trace_demonstration.rb @@ -30,7 +30,7 @@ config[:retain_middleware_names] = true config[:application] = builder config[:record_frontend_span] = true -OpenTelemetry::Adapters::Rack.install(config) +OpenTelemetry::Adapters::Rack::Adapter.instance.install(config) # integrate/activate tracing middleware: builder.use OpenTelemetry::Adapters::Rack::Middlewares::TracerMiddleware From 0be8aff24c623d17fd435460dd6e6bae82d1a4bf Mon Sep 17 00:00:00 2001 From: David Davis Date: Wed, 22 Jan 2020 16:54:22 -0800 Subject: [PATCH 13/64] Handle errors by setting span.status, leave a TODO and rescue clause --- .../rack/middlewares/tracer_middleware.rb | 14 +++++--- .../middlewares/tracer_middleware_test.rb | 32 ++++++++++++++++--- 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 09842173a..d83d9b24d 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -121,13 +121,17 @@ def rack_request @rack_request ||= ::Rack::Request.new(env) end + # catch exceptions that may be raised in the middleware chain + # Note: if a middleware catches an Exception without re raising, + # the Exception cannot be recorded here. def record_and_reraise_error(error) - # TODO: implement span.set_error? + request_span.status = OpenTelemetry::Trace::Status.new( + OpenTelemetry::Trace::Status::INTERNAL_ERROR, + description: error.to_s) - # catch exceptions that may be raised in the middleware chain - # Note: if a middleware catches an Exception without re raising, - # the Exception cannot be recorded here. - request_span.set_error(error) unless request_span.nil? + # TODO: implement span.set_error? (this is a specification-level issue): + # request_span.set_error(error) unless request_span.nil? + # raise error end diff --git a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb index 504a6b83b..98a81f8e9 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb @@ -15,21 +15,20 @@ let(:adapter_module) { OpenTelemetry::Adapters::Rack } let(:adapter_class) { adapter_module::Adapter } let(:adapter) { adapter_class.instance } - let(:app) { lambda { |env| [200, {'Content-Type' => 'text/plain'}, ['OK']] } } + let(:described_class) { OpenTelemetry::Adapters::Rack::Middlewares::TracerMiddleware } - let(:exporter) { EXPORTER } + let(:app) { lambda { |env| [200, {'Content-Type' => 'text/plain'}, ['OK']] } } + let(:middleware) { described_class.new(app) } let(:rack_builder) { Rack::Builder.new } - let(:middleware) { described_class.new(app) } + let(:exporter) { EXPORTER } let(:first_span) { exporter.finished_spans.first } let(:default_config) { Hash.new } let(:config) { default_config } let(:env) { Hash.new } - let(:registry) { OpenTelemetry::Instrumentation::Registry.new } - before do # clear captured spans: exporter.reset @@ -140,4 +139,27 @@ end end end + + describe '#call with error' do + SimulatedError = Class.new(StandardError) + + let(:app) do + lambda { |env| raise SimulatedError } + end + + it 'records error in span and then re-raises' do + assert_raises SimulatedError do + Rack::MockRequest.new(rack_builder).get('/', env) + end + end + + it 'records error in span' do + begin + Rack::MockRequest.new(rack_builder).get('/', env) + rescue SimulatedError => _expected_and_ignored + end + + _(first_span.status.canonical_code).must_equal OpenTelemetry::Trace::Status::INTERNAL_ERROR + end + end end From 14b1198742f3e602d0ff04444b0126349bee1f3f Mon Sep 17 00:00:00 2001 From: David Davis Date: Thu, 23 Jan 2020 11:48:19 -0800 Subject: [PATCH 14/64] Allow config[:quantization] * to allow for lower cardinality span names --- .../rack/middlewares/tracer_middleware.rb | 32 ++++++++++++++++--- .../middlewares/tracer_middleware_test.rb | 25 +++++++++++++++ 2 files changed, 52 insertions(+), 5 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index d83d9b24d..6fbd6e9bf 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -95,17 +95,39 @@ def request_span_attributes end def full_http_request_url - # NOTE: dd-trace-rb has implemented 'quantization'? (which lowers url cardinality) - # see Datadog::Quantization::HTTP.url - rack_request.url end # https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-http.md#name # + # recommendation: span.name(s) should be low-cardinality (e.g., + # strip off query param value, keep param name) + # + # see http://github.com/open-telemetry/opentelemetry-specification/pull/416/files def request_span_name - # uri_path_value: - env['PATH_INFO'] + # NOTE: dd-trace-rb has implemented 'quantization' (which lowers url cardinality) + # see Datadog::Quantization::HTTP.url + + if (implementation = config[:url_quantization]) + implementation.call(request_uri_or_path_info) + else + request_uri_or_path_info + end + end + + # http://www.rubydoc.info/github/rack/rack/file/SPEC + # The source of truth in Rack is the PATH_INFO key that holds the + # URL for the current request; but some frameworks may override that + # value, especially during exception handling. + # + # Because of this, we prefer to use REQUEST_URI, if available, which is the + # relative path + query string, and doesn't mutate. + # + # REQUEST_URI is only available depending on what web server is running though. + # So when its not available, we want the original, unmutated PATH_INFO, which + # is just the relative path without query strings. + def request_uri_or_path_info + env['REQUEST_URI'] || original_env['PATH_INFO'] end def base_url diff --git a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb index 98a81f8e9..a31184443 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb @@ -140,6 +140,31 @@ end end + describe 'config[:quantization]' do + + before do + Rack::MockRequest.new(rack_builder).get('/really_long_url', env) + end + + describe 'without quantization' do + it 'span.name is uri path' do + _(first_span.name).must_equal '/really_long_url' + end + end + + describe 'with quantization' do + let(:quantization_example) do + # demonstrate simple shortening of URL: + lambda { |url| url&.to_s[0..5] } + end + let(:config) { default_config.merge(url_quantization: quantization_example) } + + it 'mutates url according to url_quantization' do + _(first_span.name).must_equal '/reall' + end + end + end + describe '#call with error' do SimulatedError = Class.new(StandardError) From 9f0e2d8e9e697130e73e94bcdf45cfdc23d69b69 Mon Sep 17 00:00:00 2001 From: David Davis Date: Thu, 23 Jan 2020 13:04:30 -0800 Subject: [PATCH 15/64] Remove optional parent context extraction * defeats purpose of opentelemetry, which *is* distributed tracing --- .../rack/middlewares/tracer_middleware.rb | 7 +------ .../middlewares/tracer_middleware_test.rb | 20 ++++--------------- 2 files changed, 5 insertions(+), 22 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 6fbd6e9bf..957780d59 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -27,7 +27,7 @@ def _call(env) @original_env = env.dup frontend_span.start if record_frontend_span? - extract_parent_context if extract_parent_context? + extract_parent_context start_request_span app.call(env).tap do |status, headers, response| @@ -50,11 +50,6 @@ def _call(env) ### parent context - def extract_parent_context? - # TODO: compare to ':distributed_tracing' option name/semantics - config[:extract_parent_context] - end - def extract_parent_context @parent_context ||= OpenTelemetry.tracer_factory.http_text_format.extract(env) end diff --git a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb index a31184443..7468f317f 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb @@ -63,6 +63,10 @@ _(first_span.name).must_equal '/' end + it 'extracts parent context' do + _(first_span.parent_span_id).wont_equal '0000000000000000' + end + describe 'config[:allowed_request_headers]' do let(:env) { Hash('HTTP_FOO_BAR' => 'http foo bar value') } @@ -105,22 +109,6 @@ end end - describe 'config[:extract_parent_context]' do - describe 'default' do - it 'starts a trace without parent context' do - _(first_span.parent_span_id).must_equal '0000000000000000' - end - end - - describe 'when true' do - let(:config) { default_config.merge(extract_parent_context: true) } - - it 'extracts parent context' do - _(first_span.parent_span_id).wont_equal '0000000000000000' - end - end - end - describe 'config[:record_frontend_span]' do describe 'default' do it 'does not record span' do From fb1bf71bd017fd6e673cced8231d11b765c2b066 Mon Sep 17 00:00:00 2001 From: David Davis Date: Thu, 23 Jan 2020 13:18:06 -0800 Subject: [PATCH 16/64] Resolve 'http.base_url' TODO * add 'http.target' --- .../adapters/rack/middlewares/tracer_middleware.rb | 8 +++++++- .../adapters/rack/middlewares/tracer_middleware_test.rb | 1 + 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 957780d59..915988a69 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -85,7 +85,8 @@ def request_span_attributes 'http.url' => full_http_request_url, 'http.host' => env['HOST'], 'http.scheme' => env['rack.url_scheme'], - 'http.base_url' => base_url # TODO: check attribute naming/semantics + 'http.target' => full_path, + 'http.base_url' => base_url # NOTE: 'http.base_url' isn't officially defined }.merge(allowed_request_headers) end @@ -134,6 +135,11 @@ def base_url end end + # e.g., "/webshop/articles/4?s=1" + def full_path + rack_request.fullpath + end + def rack_request @rack_request ||= ::Rack::Request.new(env) end diff --git a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb index 7468f317f..2b1c21c44 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb @@ -58,6 +58,7 @@ _(first_span.attributes['http.method']).must_equal 'GET' _(first_span.attributes['http.status_code']).must_equal 200 _(first_span.attributes['http.status_text']).must_equal 'OK' + _(first_span.attributes['http.target']).must_equal '/' _(first_span.status.canonical_code).must_equal OpenTelemetry::Trace::Status::OK _(first_span.attributes['http.url']).must_equal 'http://example.org/' _(first_span.name).must_equal '/' From ea061d3195af014571db0ded5165362206a667ee Mon Sep 17 00:00:00 2001 From: David Davis Date: Thu, 23 Jan 2020 13:20:26 -0800 Subject: [PATCH 17/64] Resolve 'resource name' TODO * it seems that dd-trace-rb's 'span.resource' is the same as 'span.name', at least in this case --- .../adapters/rack/middlewares/tracer_middleware.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 915988a69..288f1d4c7 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -10,7 +10,9 @@ module OpenTelemetry module Adapters module Rack module Middlewares - # TODO: implement 'resource name' + # Notable implementation differences from dd-trace-rb: + # * missing: 'span.resource', which is set to span.name + # * missing: config[:distributed_tracing] class TracerMiddleware def initialize(app) @app = app From 242b403d78b5cb81ac4b443d91817f729500c706 Mon Sep 17 00:00:00 2001 From: David Davis Date: Thu, 23 Jan 2020 13:31:13 -0800 Subject: [PATCH 18/64] Resolve 'http.route' TODO * in sinatra, data is available via env['sinatra.route'].split.last, but otherwise, doesn't seem to be readily available --- .../adapters/rack/middlewares/tracer_middleware.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 288f1d4c7..1273692ff 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -164,7 +164,9 @@ def set_attributes_after_request(span, status, headers, _response) span.status = OpenTelemetry::Trace::Status.http_to_status(status) span.set_attribute('http.status_code', status) - # TODO: set_attribute('http.route', ... if it's available + # NOTE: if data is available, it would be good to do this: + # set_attribute('http.route', ... + # e.g., "/users/:userID? span.set_attribute('http.status_text', ::Rack::Utils::HTTP_STATUS_CODES[status]) allowed_response_headers(headers).each { |k, v| span.set_attribute(k, v) } From c3b9319dccaf8f5c8260e84f8e2f3cf1a7690326 Mon Sep 17 00:00:00 2001 From: David Davis Date: Thu, 23 Jan 2020 13:31:42 -0800 Subject: [PATCH 19/64] Note: missing 'span.set_error()' for now --- .../opentelemetry/adapters/rack/middlewares/tracer_middleware.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 1273692ff..bc813675d 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -13,6 +13,7 @@ module Middlewares # Notable implementation differences from dd-trace-rb: # * missing: 'span.resource', which is set to span.name # * missing: config[:distributed_tracing] + # * missing: span.set_error() -- spec level change class TracerMiddleware def initialize(app) @app = app From b110cf1391fea953f3ac8ba6c6c4bf9e0389e2d6 Mon Sep 17 00:00:00 2001 From: David Davis Date: Thu, 23 Jan 2020 13:37:50 -0800 Subject: [PATCH 20/64] Resolve FrontendSpan TODOs * resolve 'http_server.queue' TODO * span kind of 'proxy' is not defined, yet --- .../rack/middlewares/tracer_middleware.rb | 18 +++++++++++------- .../rack/middlewares/tracer_middleware_test.rb | 15 ++++++++++++++- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index bc813675d..5fb69687a 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -232,17 +232,19 @@ def record_frontend_span? def frontend_span @frontend_span ||= FrontendSpan.new(env: env, tracer: tracer, - parent_context: parent_context) + parent_context: parent_context, + web_service_name: config[:web_service_name]) end class FrontendSpan - def initialize(env:, tracer:, parent_context:) + def initialize(env:, tracer:, parent_context:, web_service_name:) @env = env @tracer = tracer @parent_context = parent_context # NOTE: get_request_start may return nil @request_start_time = OpenTelemetry::Adapters::Rack::Util::QueueTime.get_request_start(env) + @web_service_name = web_service_name end def recordable? @@ -250,13 +252,14 @@ def recordable? end def start - @span ||= tracer.start_span('http_server.queue', # TODO: check this name - with_parent_context: parent_context, # TODO: check for correct parent + @span ||= tracer.start_span('http_server.queue', # NOTE: span kind of 'proxy' is not defined + with_parent_context: parent_context, # NOTE: initialize with as many attributes as possible: attributes: { 'component' => 'http', - # TODO: 'service' => config[:web_service_name] - # TODO: 'span_type' => PROXY + 'service' => web_service_name, + # NOTE: dd-trace-rb differences: + # 'trace.span_type' => (http) 'proxy' 'start_time' => request_start_time }) end @@ -271,7 +274,8 @@ def finish :parent_context, :request_start_time, :span, - :tracer + :tracer, + :web_service_name end end end diff --git a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb index 2b1c21c44..ddadb3eff 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb @@ -120,10 +120,23 @@ describe 'when recordable' do let(:config) { default_config.merge(record_frontend_span: true)} let(:env) { Hash('HTTP_X_REQUEST_START' => Time.now.to_i) } + let(:frontend_span) { exporter.finished_spans.last } it 'records span' do _(exporter.finished_spans.size).must_equal 2 - _(exporter.finished_spans.last.name).must_equal 'http_server.queue' + _(frontend_span.name).must_equal 'http_server.queue' + _(frontend_span.attributes['service']).must_be_nil + end + + describe 'when config[:web_service_name]' do + let(:config) do + default_config.merge(record_frontend_span: true, + web_service_name: 'my_service_name') + end + + it 'records "service" attribute' do + _(frontend_span.attributes['service']).must_equal 'my_service_name' + end end end end From 4cdca21bc9feb9c74c70fd57cb9ac34f18bf5008 Mon Sep 17 00:00:00 2001 From: David Davis Date: Thu, 23 Jan 2020 16:22:27 -0800 Subject: [PATCH 21/64] Optimize allowed_request_headers * reduce string allocations * TIL: a nested 'before' block doesn't run before *each* test, but the top/first 'before' block does. (weird) --- .../rack/middlewares/tracer_middleware.rb | 37 +++++++++++++++---- .../middlewares/tracer_middleware_test.rb | 6 ++- 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 5fb69687a..300c8e243 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -15,6 +15,32 @@ module Middlewares # * missing: config[:distributed_tracing] # * missing: span.set_error() -- spec level change class TracerMiddleware + class << self + def allowed_rack_request_headers + @allowed_rack_request_header_names ||= allowed_request_header_names.map do |header| + { + header: header, + rack_header: "HTTP_#{header.to_s.upcase.gsub(/[-\s]/, '_')}" + } + end + end + + def allowed_request_header_names + @allowed_request_header_names ||= Array(config[:allowed_request_headers]) + end + + def config + Rack::Adapter.instance.config + end + + private + + def clear_cached_config + @allowed_request_header_names = nil + @allowed_rack_request_header_names = nil + end + end + def initialize(app) @app = app end @@ -176,19 +202,14 @@ def set_attributes_after_request(span, status, headers, _response) # @return Hash def allowed_request_headers {}.tap do |result| - Array(allowed_request_header_names).each do |header| - rack_header = "HTTP_#{header.to_s.upcase.gsub(/[-\s]/, '_')}" - if env.key?(rack_header) - result[build_attribute_name('http.request.headers.', header)] = env[rack_header] + self.class.allowed_rack_request_headers.each do |hash| + if env.key?(hash[:rack_header]) + result[build_attribute_name('http.request.headers.', hash[:header])] = env[hash[:rack_header]] end end end end - def allowed_request_header_names - Array(config[:allowed_request_headers]) - end - # @return Hash def allowed_response_headers(headers) return {} if headers.nil? diff --git a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb index ddadb3eff..1ba997fe2 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb @@ -37,6 +37,9 @@ adapter.instance_variable_set('@installed', false) adapter.install(config) + # clear out cached config: + described_class.send(:clear_cached_config) + # integrate tracer middleware: rack_builder.run app rack_builder.use described_class @@ -100,7 +103,7 @@ _(first_span.attributes['http.response.headers.foo_bar']).must_equal 'foo bar response header' end - describe "case-sensitively" do + describe 'case-sensitively' do let(:config) { default_config.merge(allowed_response_headers: ['fOO-bAR']) } it 'returns attribute' do @@ -143,7 +146,6 @@ end describe 'config[:quantization]' do - before do Rack::MockRequest.new(rack_builder).get('/really_long_url', env) end From ba869414856bde83d03c948fb0f4ab46a4ec2415 Mon Sep 17 00:00:00 2001 From: David Davis Date: Thu, 23 Jan 2020 17:41:41 -0800 Subject: [PATCH 22/64] Optimize allowed_response_headers() * prevent unneeded string and object allocation * once a header key is found, add it to the list of response headers to search for --- .../rack/middlewares/tracer_middleware.rb | 53 +++++++++++-------- 1 file changed, 32 insertions(+), 21 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 300c8e243..9003bf663 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -29,6 +29,24 @@ def allowed_request_header_names @allowed_request_header_names ||= Array(config[:allowed_request_headers]) end + def allowed_response_headers + @allowed_response_headers ||= allowed_response_header_names.map do |header| + { + header: header, + attribute_name: build_attribute_name('http.response.headers.', header), + search_for_keys: [header, header.to_s.upcase] + } + end + end + + def allowed_response_header_names + @allowed_response_header_names ||= Array(config[:allowed_response_headers]) + end + + def build_attribute_name(prefix, suffix) + prefix + suffix.to_s.downcase.gsub(/[-\s]/, '_') + end + def config Rack::Adapter.instance.config end @@ -36,8 +54,11 @@ def config private def clear_cached_config - @allowed_request_header_names = nil @allowed_rack_request_header_names = nil + @allowed_request_header_names = nil + + @allowed_response_header_names = nil + @allowed_response_headers = nil end end @@ -204,7 +225,7 @@ def allowed_request_headers {}.tap do |result| self.class.allowed_rack_request_headers.each do |hash| if env.key?(hash[:rack_header]) - result[build_attribute_name('http.request.headers.', hash[:header])] = env[hash[:rack_header]] + result[self.class.build_attribute_name('http.request.headers.', hash[:header])] = env[hash[:rack_header]] end end end @@ -215,30 +236,20 @@ def allowed_response_headers(headers) return {} if headers.nil? {}.tap do |result| - Array(allowed_response_header_names).each do |header| - new_key_name = build_attribute_name('http.response.headers.', header) - if headers.key?(header) - result[new_key_name] = headers[header] - else - # Try a case-insensitive lookup - uppercased_header = header.to_s.upcase - matching_header = headers.keys.find { |h| h.upcase == uppercased_header } - if matching_header - result[new_key_name] = headers[matching_header] - end + self.class.allowed_response_headers.each do |hash| + key = headers.keys.find { |k| hash[:search_for_keys].find { |k2| k == k2 } } || + headers.keys.find { |k| hash[:search_for_keys].find { |k2| k.upcase == k2 } } + + if key + # cache string for next time: + hash[:search_for_keys] |= [key] + + result[hash[:attribute_name]] = headers[key] end end end end - def allowed_response_header_names - Array(config[:allowed_response_headers]) - end - - def build_attribute_name(prefix, suffix) - prefix + suffix.to_s.downcase.gsub(/[-\s]/, '_') - end - def config Rack::Adapter.instance.config end From e328a7c992a0cc3294bb0d12c698b996a2ca90ec Mon Sep 17 00:00:00 2001 From: David Davis Date: Thu, 23 Jan 2020 17:43:58 -0800 Subject: [PATCH 23/64] Optimize return of EMPTY_HASH frozen constant --- .../adapters/rack/middlewares/tracer_middleware.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 9003bf663..70dfd0c6a 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -62,6 +62,8 @@ def clear_cached_config end end + EMPTY_HASH = {}.freeze + def initialize(app) @app = app end @@ -233,7 +235,7 @@ def allowed_request_headers # @return Hash def allowed_response_headers(headers) - return {} if headers.nil? + return EMPTY_HASH if headers.nil? {}.tap do |result| self.class.allowed_response_headers.each do |hash| From 654483905903399949150c7803a5d17cee7c13df Mon Sep 17 00:00:00 2001 From: David Davis Date: Thu, 23 Jan 2020 18:35:21 -0800 Subject: [PATCH 24/64] Refactor to avoid using dup._call(env) * avoid using instance variables --- .../rack/middlewares/tracer_middleware.rb | 191 +++++++----------- 1 file changed, 69 insertions(+), 122 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 70dfd0c6a..8008cbeb2 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -69,60 +69,82 @@ def initialize(app) end def call(env) - # since middleware may only be initialized once (memoized), - # but we want to be sure to have a clean slate on each request: - dup._call env - end - - def _call(env) - @env = env - @original_env = env.dup - - frontend_span.start if record_frontend_span? - extract_parent_context - - start_request_span - app.call(env).tap do |status, headers, response| + original_env = env.dup + + parent_context = OpenTelemetry.tracer_factory.http_text_format.extract(env) + + ### frontend span + # NOTE: get_request_start may return nil + request_start_time = OpenTelemetry::Adapters::Rack::Util::QueueTime.get_request_start(env) + frontend_span = tracer.start_span('http_server.queue', # NOTE: span kind of 'proxy' is not defined + with_parent_context: parent_context, + # NOTE: initialize with as many attributes as possible: + attributes: { + 'component' => 'http', + 'service' => config[:web_service_name], + # NOTE: dd-trace-rb differences: + # 'trace.span_type' => (http) 'proxy' + 'start_time' => request_start_time + }) + + record_frontend_span = config[:record_frontend_span] && !request_start_time.nil? + frontend_span = if record_frontend_span + tracer.start_span('http_server.queue', # NOTE: span kind of 'proxy' is not defined + with_parent_context: parent_context, + # NOTE: initialize with as many attributes as possible: + attributes: { + 'component' => 'http', + 'service' => config[:web_service_name], + # NOTE: dd-trace-rb differences: + # 'trace.span_type' => (http) 'proxy' + 'start_time' => request_start_time + }) + end + + # http://www.rubydoc.info/github/rack/rack/file/SPEC + # The source of truth in Rack is the PATH_INFO key that holds the + # URL for the current request; but some frameworks may override that + # value, especially during exception handling. + # + # Because of this, we prefer to use REQUEST_URI, if available, which is the + # relative path + query string, and doesn't mutate. + # + # REQUEST_URI is only available depending on what web server is running though. + # So when its not available, we want the original, unmutated PATH_INFO, which + # is just the relative path without query strings. + request_span_name = create_request_span_name(env['REQUEST_URI'] || original_env['PATH_INFO']) + + rack_request = ::Rack::Request.new(env) + # Sets as many attributes as are available before control + # is handed off to next middleware. + request_span = tracer.start_span(request_span_name, + with_parent_context: parent_context, + # NOTE: try to set as many attributes via 'attributes' argument + # instead of via span.set_attribute + attributes: request_span_attributes(env: env, + full_http_request_url: full_http_request_url(rack_request), + full_path: full_path(rack_request), + base_url: base_url(rack_request)), + kind: :server) + + @app.call(env).tap do |status, headers, response| set_attributes_after_request(request_span, status, headers, response) end rescue Exception => e - record_and_reraise_error(e) + record_and_reraise_error(e, request_span: request_span) ensure request_span.finish - frontend_span.finish if record_frontend_span? + frontend_span&.finish if record_frontend_span end private - attr_reader :app, - :env, - :original_env, - :parent_context, - :request_span - - ### parent context - - def extract_parent_context - @parent_context ||= OpenTelemetry.tracer_factory.http_text_format.extract(env) - end - def tracer OpenTelemetry::Adapters::Rack::Adapter.instance.tracer end ### request_span - # Sets as many attributes as are available before control - # is handed off to next middleware. - def start_request_span - @request_span ||= tracer.start_span(request_span_name, - with_parent_context: parent_context, - # NOTE: try to set as many attributes via 'attributes' argument - # instead of via span.set_attribute - attributes: request_span_attributes, - kind: :server) - end - # Per https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-http.md#http-server-semantic-conventions # # One of the following sets is required, in descending preference: @@ -130,7 +152,7 @@ def start_request_span # * http.scheme, http.server_name, net.host.port, http.target # * http.scheme, net.host.name, net.host.port, http.target # * http.url - def request_span_attributes + def request_span_attributes(env:, full_http_request_url:, full_path:, base_url:) { 'component' => 'http', 'http.method' => env['REQUEST_METHOD'], @@ -139,10 +161,10 @@ def request_span_attributes 'http.scheme' => env['rack.url_scheme'], 'http.target' => full_path, 'http.base_url' => base_url # NOTE: 'http.base_url' isn't officially defined - }.merge(allowed_request_headers) + }.merge(allowed_request_headers(env)) end - def full_http_request_url + def full_http_request_url(rack_request) rack_request.url end @@ -152,7 +174,7 @@ def full_http_request_url # strip off query param value, keep param name) # # see http://github.com/open-telemetry/opentelemetry-specification/pull/416/files - def request_span_name + def create_request_span_name(request_uri_or_path_info) # NOTE: dd-trace-rb has implemented 'quantization' (which lowers url cardinality) # see Datadog::Quantization::HTTP.url @@ -163,22 +185,7 @@ def request_span_name end end - # http://www.rubydoc.info/github/rack/rack/file/SPEC - # The source of truth in Rack is the PATH_INFO key that holds the - # URL for the current request; but some frameworks may override that - # value, especially during exception handling. - # - # Because of this, we prefer to use REQUEST_URI, if available, which is the - # relative path + query string, and doesn't mutate. - # - # REQUEST_URI is only available depending on what web server is running though. - # So when its not available, we want the original, unmutated PATH_INFO, which - # is just the relative path without query strings. - def request_uri_or_path_info - env['REQUEST_URI'] || original_env['PATH_INFO'] - end - - def base_url + def base_url(rack_request) if rack_request.respond_to?(:base_url) rack_request.base_url else @@ -188,18 +195,14 @@ def base_url end # e.g., "/webshop/articles/4?s=1" - def full_path + def full_path(rack_request) rack_request.fullpath end - def rack_request - @rack_request ||= ::Rack::Request.new(env) - end - # catch exceptions that may be raised in the middleware chain # Note: if a middleware catches an Exception without re raising, # the Exception cannot be recorded here. - def record_and_reraise_error(error) + def record_and_reraise_error(error, request_span:) request_span.status = OpenTelemetry::Trace::Status.new( OpenTelemetry::Trace::Status::INTERNAL_ERROR, description: error.to_s) @@ -223,7 +226,7 @@ def set_attributes_after_request(span, status, headers, _response) end # @return Hash - def allowed_request_headers + def allowed_request_headers(env) {}.tap do |result| self.class.allowed_rack_request_headers.each do |hash| if env.key?(hash[:rack_header]) @@ -255,62 +258,6 @@ def allowed_response_headers(headers) def config Rack::Adapter.instance.config end - - ### frontend span - - def record_frontend_span? - Rack::Adapter.instance.config[:record_frontend_span] && - frontend_span.recordable? - end - - def frontend_span - @frontend_span ||= FrontendSpan.new(env: env, - tracer: tracer, - parent_context: parent_context, - web_service_name: config[:web_service_name]) - end - - class FrontendSpan - def initialize(env:, tracer:, parent_context:, web_service_name:) - @env = env - @tracer = tracer - @parent_context = parent_context - - # NOTE: get_request_start may return nil - @request_start_time = OpenTelemetry::Adapters::Rack::Util::QueueTime.get_request_start(env) - @web_service_name = web_service_name - end - - def recordable? - !request_start_time.nil? - end - - def start - @span ||= tracer.start_span('http_server.queue', # NOTE: span kind of 'proxy' is not defined - with_parent_context: parent_context, - # NOTE: initialize with as many attributes as possible: - attributes: { - 'component' => 'http', - 'service' => web_service_name, - # NOTE: dd-trace-rb differences: - # 'trace.span_type' => (http) 'proxy' - 'start_time' => request_start_time - }) - end - - def finish - span&.finish - end - - private - - attr_reader :env, - :parent_context, - :request_start_time, - :span, - :tracer, - :web_service_name - end end end end From 22b69cd08f3f94ba8a1a2a135393142384ba6e7c Mon Sep 17 00:00:00 2001 From: David Davis Date: Tue, 28 Jan 2020 10:15:26 -0800 Subject: [PATCH 25/64] Add Appraisals, integrate into circleci --- .circleci/config.yml | 9 +++++++++ adapters/rack/Appraisals | 13 +++++++++++++ adapters/rack/gemfiles/rack_2.0.gemfile | 12 ++++++++++++ adapters/rack/gemfiles/rack_2.1.gemfile | 12 ++++++++++++ 4 files changed, 46 insertions(+) create mode 100644 adapters/rack/Appraisals create mode 100644 adapters/rack/gemfiles/rack_2.0.gemfile create mode 100644 adapters/rack/gemfiles/rack_2.1.gemfile diff --git a/.circleci/config.yml b/.circleci/config.yml index b58b5ce61..850ae67a6 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -59,6 +59,15 @@ commands: bundle install --jobs=3 --retry=3 bundle exec appraisal install bundle exec appraisal rake test + - run: + name: Bundle + CI (Adapters - Rack) + command: | + cd adapters/rack + gem uninstall -aIx bundler + gem install --no-document bundler -v '~> 2.0.2' + bundle install --jobs=3 --retry=3 + bundle exec appraisal install + bundle exec appraisal rake test - run: name: Bundle + CI (Adapters - Sinatra) command: | diff --git a/adapters/rack/Appraisals b/adapters/rack/Appraisals new file mode 100644 index 000000000..afa4b7cab --- /dev/null +++ b/adapters/rack/Appraisals @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +# Copyright 2020 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +appraise 'rack-2.1' do + gem 'rack', '~> 2.1.2' +end + +appraise 'rack-2.0' do + gem 'rack', '2.0.8' +end diff --git a/adapters/rack/gemfiles/rack_2.0.gemfile b/adapters/rack/gemfiles/rack_2.0.gemfile new file mode 100644 index 000000000..19d162252 --- /dev/null +++ b/adapters/rack/gemfiles/rack_2.0.gemfile @@ -0,0 +1,12 @@ +# This file was generated by Appraisal + +source "https://rubygems.org" + +gem "opentelemetry-api", path: "../../../api" +gem "rack", "2.0.8" + +group :test do + gem "opentelemetry-sdk", path: "../../../sdk" +end + +gemspec path: "../" diff --git a/adapters/rack/gemfiles/rack_2.1.gemfile b/adapters/rack/gemfiles/rack_2.1.gemfile new file mode 100644 index 000000000..4a1ed5a7f --- /dev/null +++ b/adapters/rack/gemfiles/rack_2.1.gemfile @@ -0,0 +1,12 @@ +# This file was generated by Appraisal + +source "https://rubygems.org" + +gem "opentelemetry-api", path: "../../../api" +gem "rack", "~> 2.1.2" + +group :test do + gem "opentelemetry-sdk", path: "../../../sdk" +end + +gemspec path: "../" From 44ddb04e60c40b0e07a249cd6a59e297f44b07bf Mon Sep 17 00:00:00 2001 From: David Davis Date: Tue, 28 Jan 2020 10:43:32 -0800 Subject: [PATCH 26/64] Integrate rubocop, fix violations, add adapters to top-level rake task * per work in #179 --- Rakefile | 6 ++++ adapters/rack/.rubocop.yml | 27 +++++++++++++++ adapters/rack/Rakefile | 14 ++++++++ adapters/rack/example/Gemfile | 5 +-- adapters/rack/example/trace_demonstration.rb | 10 ++++-- .../rack/lib/opentelemetry/adapters/rack.rb | 1 + .../opentelemetry/adapters/rack/adapter.rb | 5 +-- .../rack/middlewares/tracer_middleware.rb | 34 ++++++++++--------- .../adapters/rack/util/queue_time.rb | 6 ++-- .../rack/opentelemetry-adapters-rack.gemspec | 3 ++ adapters/rack/test/.rubocop.yml | 4 +++ .../adapters/rack/adapter_test.rb | 6 ++-- .../middlewares/tracer_middleware_test.rb | 24 +++++-------- .../adapters/rack/util/queue_time_test.rb | 16 ++++----- 14 files changed, 109 insertions(+), 52 deletions(-) create mode 100644 adapters/rack/.rubocop.yml create mode 100644 adapters/rack/test/.rubocop.yml diff --git a/Rakefile b/Rakefile index ace92c62e..431dead6d 100644 --- a/Rakefile +++ b/Rakefile @@ -63,6 +63,12 @@ GEM_INFO = { OpenTelemetry::Adapters::Faraday::VERSION } }, + "opentelemetry-adapters-rack" => { + version_getter: ->() { + require './lib/opentelemetry/adapters/rack/version.rb' + OpenTelemetry::Adapters::Rack::VERSION + } + } "opentelemetry-adapters-sinatra" => { version_getter: ->() { require './lib/opentelemetry/adapters/sinatra/version.rb' diff --git a/adapters/rack/.rubocop.yml b/adapters/rack/.rubocop.yml new file mode 100644 index 000000000..7284b77cd --- /dev/null +++ b/adapters/rack/.rubocop.yml @@ -0,0 +1,27 @@ +AllCops: + TargetRubyVersion: '2.4.0' + +Bundler/OrderedGems: + Exclude: + - gemfiles/**/* +Lint/UnusedMethodArgument: + Enabled: false +Metrics/AbcSize: + Max: 18 +Metrics/LineLength: + Enabled: false +Metrics/MethodLength: + Max: 20 +Metrics/ParameterLists: + Enabled: false +Naming/FileName: + Exclude: + - "lib/opentelemetry-adapters-rack.rb" +Style/FrozenStringLiteralComment: + Exclude: + - gemfiles/**/* +Style/ModuleFunction: + Enabled: false +Style/StringLiterals: + Exclude: + - gemfiles/**/* diff --git a/adapters/rack/Rakefile b/adapters/rack/Rakefile index 59f0fddd5..197514f1d 100644 --- a/adapters/rack/Rakefile +++ b/adapters/rack/Rakefile @@ -6,9 +6,23 @@ require 'bundler/gem_tasks' require 'rake/testtask' +require 'yard' +require 'rubocop/rake_task' + +RuboCop::RakeTask.new Rake::TestTask.new :test do |t| t.libs << 'test' t.libs << 'lib' t.test_files = FileList['test/**/*_test.rb'] end + +YARD::Rake::YardocTask.new do |t| + t.stats_options = ['--list-undoc'] +end + +if RUBY_ENGINE == 'truffleruby' + task default: %i[test] +else + task default: %i[test rubocop yard] +end diff --git a/adapters/rack/example/Gemfile b/adapters/rack/example/Gemfile index 837779aa3..251efbc10 100644 --- a/adapters/rack/example/Gemfile +++ b/adapters/rack/example/Gemfile @@ -2,7 +2,8 @@ source 'https://rubygems.org' -gem 'rack' -gem 'rack-test' +gem 'opentelemetry-adapters-rack', path: '../../../adapters/rack' gem 'opentelemetry-api', path: '../../../api' gem 'opentelemetry-sdk', path: '../../../sdk' +gem 'rack' +gem 'rack-test' diff --git a/adapters/rack/example/trace_demonstration.rb b/adapters/rack/example/trace_demonstration.rb index 9e762c812..6f1f06036 100644 --- a/adapters/rack/example/trace_demonstration.rb +++ b/adapters/rack/example/trace_demonstration.rb @@ -1,3 +1,9 @@ +# frozen_string_literal: true + +# Copyright 2020 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + require 'rubygems' require 'bundler/setup' @@ -20,9 +26,9 @@ # setup fake rack application: builder = Rack::Builder.new do # can't use TracerMiddleware constant before calling Adapters::Rack.install: - #use OpenTelemetry::Adapters::Rack::Middlewares::TracerMiddleware + # use OpenTelemetry::Adapters::Rack::Middlewares::TracerMiddleware end -app = lambda { |env| [200, {'Content-Type' => 'text/plain'}, ['All responses are OK']] } +app = ->(_env) { [200, { 'Content-Type' => 'text/plain' }, ['All responses are OK']] } builder.run app # demonstrate rack configuration options: diff --git a/adapters/rack/lib/opentelemetry/adapters/rack.rb b/adapters/rack/lib/opentelemetry/adapters/rack.rb index e2d7716d9..9ef82145a 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack.rb @@ -8,6 +8,7 @@ module OpenTelemetry module Adapters + # Contains the OpenTelemetry adapter for the Rack gem module Rack end end diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb b/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb index 8869d1a2c..507ca5c28 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb @@ -7,8 +7,9 @@ module OpenTelemetry module Adapters module Rack + # The Adapter class contains logic to detect and install the Rack + # instrumentation adapter class Adapter < OpenTelemetry::Instrumentation::Adapter - install do |config| require_dependencies @@ -45,7 +46,7 @@ def call(env) end next_middleware = next_middleware.instance_variable_defined?('@app') && - next_middleware.instance_variable_get('@app') + next_middleware.instance_variable_get('@app') end end end diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 8008cbeb2..870df8a68 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -10,14 +10,17 @@ module OpenTelemetry module Adapters module Rack module Middlewares + # TracerMiddleware propagates context and instruments Rack requests + # by way of its middleware system + # # Notable implementation differences from dd-trace-rb: # * missing: 'span.resource', which is set to span.name # * missing: config[:distributed_tracing] # * missing: span.set_error() -- spec level change - class TracerMiddleware + class TracerMiddleware # rubocop:disable Metrics/ClassLength class << self def allowed_rack_request_headers - @allowed_rack_request_header_names ||= allowed_request_header_names.map do |header| + @allowed_rack_request_headers ||= allowed_request_header_names.map do |header| { header: header, rack_header: "HTTP_#{header.to_s.upcase.gsub(/[-\s]/, '_')}" @@ -54,11 +57,11 @@ def config private def clear_cached_config - @allowed_rack_request_header_names = nil + @allowed_rack_request_headers = nil @allowed_request_header_names = nil - @allowed_response_header_names = nil @allowed_response_headers = nil + @allowed_response_header_names = nil end end @@ -68,7 +71,7 @@ def initialize(app) @app = app end - def call(env) + def call(env) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength original_env = env.dup parent_context = OpenTelemetry.tracer_factory.http_text_format.extract(env) @@ -130,7 +133,7 @@ def call(env) @app.call(env).tap do |status, headers, response| set_attributes_after_request(request_span, status, headers, response) end - rescue Exception => e + rescue StandardError => e record_and_reraise_error(e, request_span: request_span) ensure request_span.finish @@ -205,7 +208,8 @@ def full_path(rack_request) def record_and_reraise_error(error, request_span:) request_span.status = OpenTelemetry::Trace::Status.new( OpenTelemetry::Trace::Status::INTERNAL_ERROR, - description: error.to_s) + description: error.to_s + ) # TODO: implement span.set_error? (this is a specification-level issue): # request_span.set_error(error) unless request_span.nil? @@ -229,9 +233,7 @@ def set_attributes_after_request(span, status, headers, _response) def allowed_request_headers(env) {}.tap do |result| self.class.allowed_rack_request_headers.each do |hash| - if env.key?(hash[:rack_header]) - result[self.class.build_attribute_name('http.request.headers.', hash[:header])] = env[hash[:rack_header]] - end + result[self.class.build_attribute_name('http.request.headers.', hash[:header])] = env[hash[:rack_header]] if env.key?(hash[:rack_header]) end end end @@ -243,14 +245,14 @@ def allowed_response_headers(headers) {}.tap do |result| self.class.allowed_response_headers.each do |hash| key = headers.keys.find { |k| hash[:search_for_keys].find { |k2| k == k2 } } || - headers.keys.find { |k| hash[:search_for_keys].find { |k2| k.upcase == k2 } } + headers.keys.find { |k| hash[:search_for_keys].find { |k2| k.upcase == k2 } } + + next unless key - if key - # cache string for next time: - hash[:search_for_keys] |= [key] + # cache string for next time: + hash[:search_for_keys] |= [key] - result[hash[:attribute_name]] = headers[key] - end + result[hash[:attribute_name]] = headers[key] end end end diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/util/queue_time.rb b/adapters/rack/lib/opentelemetry/adapters/rack/util/queue_time.rb index fada6b398..e2870a645 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/util/queue_time.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/util/queue_time.rb @@ -10,13 +10,13 @@ module Rack module Util # QueueTime simply... module QueueTime - REQUEST_START = 'HTTP_X_REQUEST_START'.freeze - QUEUE_START = 'HTTP_X_QUEUE_START'.freeze + REQUEST_START = 'HTTP_X_REQUEST_START' + QUEUE_START = 'HTTP_X_QUEUE_START' MINIMUM_ACCEPTABLE_TIME_VALUE = 1_000_000_000 module_function - def get_request_start(env, now = nil) + def get_request_start(env, now = nil) # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity header = env[REQUEST_START] || env[QUEUE_START] return unless header diff --git a/adapters/rack/opentelemetry-adapters-rack.gemspec b/adapters/rack/opentelemetry-adapters-rack.gemspec index 7f67534dd..3bec1727c 100644 --- a/adapters/rack/opentelemetry-adapters-rack.gemspec +++ b/adapters/rack/opentelemetry-adapters-rack.gemspec @@ -34,6 +34,9 @@ Gem::Specification.new do |spec| spec.add_development_dependency 'rack', '~> 2.0.8' spec.add_development_dependency 'rack-test', '~> 1.1.0' spec.add_development_dependency 'rake', '~> 12.3.3' + spec.add_development_dependency 'rubocop', '~> 0.73.0' spec.add_development_dependency 'simplecov', '~> 0.17.1' spec.add_development_dependency 'webmock', '~> 3.7.6' + spec.add_development_dependency 'yard', '~> 0.9' + spec.add_development_dependency 'yard-doctest', '~> 0.1.6' end diff --git a/adapters/rack/test/.rubocop.yml b/adapters/rack/test/.rubocop.yml new file mode 100644 index 000000000..dd9425858 --- /dev/null +++ b/adapters/rack/test/.rubocop.yml @@ -0,0 +1,4 @@ +inherit_from: ../.rubocop.yml + +Metrics/BlockLength: + Enabled: false diff --git a/adapters/rack/test/opentelemetry/adapters/rack/adapter_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/adapter_test.rb index ba6043e97..3bf4d494b 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/adapter_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/adapter_test.rb @@ -11,12 +11,12 @@ describe OpenTelemetry::Adapters::Rack::Adapter do let(:adapter_class) { OpenTelemetry::Adapters::Rack::Adapter } let(:adapter) { adapter_class.instance } - let(:config) { Hash.new } + let(:config) { {} } after do # simulate a fresh install: adapter.instance_variable_set('@installed', false) - adapter.install(Hash.new) + adapter.install({}) end describe 'config[:retain_middleware_names]' do @@ -39,7 +39,7 @@ class MyAppClass def call(env) @env = env - [200, {'Content-Type' => 'text/plain'}, ['OK']] + [200, { 'Content-Type' => 'text/plain' }, ['OK']] end end diff --git a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb index 1ba997fe2..db86dc12e 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb @@ -6,7 +6,7 @@ require 'test_helper' - #require Adapter so .install method is found: +# require Adapter so .install method is found: require_relative '../../../../../lib/opentelemetry/adapters/rack' require_relative '../../../../../lib/opentelemetry/adapters/rack/adapter' require_relative '../../../../../lib/opentelemetry/adapters/rack/middlewares/tracer_middleware' @@ -18,16 +18,16 @@ let(:described_class) { OpenTelemetry::Adapters::Rack::Middlewares::TracerMiddleware } - let(:app) { lambda { |env| [200, {'Content-Type' => 'text/plain'}, ['OK']] } } + let(:app) { ->(_env) { [200, { 'Content-Type' => 'text/plain' }, ['OK']] } } let(:middleware) { described_class.new(app) } let(:rack_builder) { Rack::Builder.new } let(:exporter) { EXPORTER } let(:first_span) { exporter.finished_spans.first } - let(:default_config) { Hash.new } + let(:default_config) { {} } let(:config) { default_config } - let(:env) { Hash.new } + let(:env) { {} } before do # clear captured spans: @@ -89,7 +89,7 @@ describe 'config[:allowed_response_headers]' do let(:app) do - lambda { |env| [200, {'Foo-Bar' => 'foo bar response header'}, ['OK']] } + ->(_env) { [200, { 'Foo-Bar' => 'foo bar response header' }, ['OK']] } end it 'defaults to nil' do @@ -121,7 +121,7 @@ end describe 'when recordable' do - let(:config) { default_config.merge(record_frontend_span: true)} + let(:config) { default_config.merge(record_frontend_span: true) } let(:env) { Hash('HTTP_X_REQUEST_START' => Time.now.to_i) } let(:frontend_span) { exporter.finished_spans.last } @@ -159,7 +159,7 @@ describe 'with quantization' do let(:quantization_example) do # demonstrate simple shortening of URL: - lambda { |url| url&.to_s[0..5] } + ->(url) { url.to_s[0..5] } end let(:config) { default_config.merge(url_quantization: quantization_example) } @@ -173,21 +173,13 @@ SimulatedError = Class.new(StandardError) let(:app) do - lambda { |env| raise SimulatedError } + ->(_env) { raise SimulatedError } end it 'records error in span and then re-raises' do assert_raises SimulatedError do Rack::MockRequest.new(rack_builder).get('/', env) end - end - - it 'records error in span' do - begin - Rack::MockRequest.new(rack_builder).get('/', env) - rescue SimulatedError => _expected_and_ignored - end - _(first_span.status.canonical_code).must_equal OpenTelemetry::Trace::Status::INTERNAL_ERROR end end diff --git a/adapters/rack/test/opentelemetry/adapters/rack/util/queue_time_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/util/queue_time_test.rb index 25f029208..cedd4bcdb 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/util/queue_time_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/util/queue_time_test.rb @@ -19,7 +19,7 @@ describe 'milliseconds' do describe 'REQUEST_START' do let(:env) { { described_class::REQUEST_START => "t=#{expected}" } } - let(:expected) { 1512379167.574 } + let(:expected) { 1_512_379_167.574 } it { expect(request_start.to_f).must_equal(expected) } describe 'but does not start with t=' do @@ -29,8 +29,8 @@ describe 'without decimal places' do let(:env) { { described_class::REQUEST_START => expected } } - let(:expected) { 1512379167574 } - it { expect(request_start.to_f).must_equal(1512379167.574) } + let(:expected) { 1_512_379_167_574 } + it { expect(request_start.to_f).must_equal(1_512_379_167.574) } end describe 'but a malformed expected' do @@ -46,7 +46,7 @@ describe 'QUEUE_START' do let(:env) { { described_class::QUEUE_START => "t=#{expected}" } } - let(:expected) { 1512379167.574 } + let(:expected) { 1_512_379_167.574 } it { expect(request_start.to_f).must_equal(expected) } end end @@ -54,7 +54,7 @@ describe 'microseconds' do describe 'REQUEST_START' do let(:env) { { described_class::REQUEST_START => "t=#{expected}" } } - let(:expected) { 1570633834.463123 } + let(:expected) { 1_570_633_834.463123 } it { expect(request_start.to_f).must_equal(expected) } describe 'but does not start with t=' do @@ -64,8 +64,8 @@ describe 'without decimal places' do let(:env) { { described_class::REQUEST_START => expected } } - let(:expected) { 1570633834463123 } - it { expect(request_start.to_f).must_equal(1570633834.463123) } + let(:expected) { 1_570_633_834_463_123 } + it { expect(request_start.to_f).must_equal(1_570_633_834.463123) } end describe 'but a malformed expected' do @@ -76,7 +76,7 @@ describe 'QUEUE_START' do let(:env) { { described_class::QUEUE_START => "t=#{expected}" } } - let(:expected) { 1570633834.463123 } + let(:expected) { 1_570_633_834.463123 } it { expect(request_start.to_f).must_equal(expected) } end end From 197ade6c61a38bb60e7c27458c48703a88a7b190 Mon Sep 17 00:00:00 2001 From: David Davis Date: Tue, 28 Jan 2020 11:09:59 -0800 Subject: [PATCH 27/64] Update example to use new config * per #171, #177 --- adapters/rack/example/trace_demonstration.rb | 36 +++++-------------- .../opentelemetry/adapters/rack/adapter.rb | 3 +- .../adapters/rack/adapter_test.rb | 2 ++ 3 files changed, 12 insertions(+), 29 deletions(-) diff --git a/adapters/rack/example/trace_demonstration.rb b/adapters/rack/example/trace_demonstration.rb index 6f1f06036..2ebe3e5ec 100644 --- a/adapters/rack/example/trace_demonstration.rb +++ b/adapters/rack/example/trace_demonstration.rb @@ -7,38 +7,18 @@ require 'rubygems' require 'bundler/setup' -require 'opentelemetry/sdk' - -require 'rack' -require_relative '../lib/opentelemetry/adapters/rack' - -# Set preferred tracer implementation: -SDK = OpenTelemetry::SDK - -# global initialization: -factory = OpenTelemetry.tracer_factory = SDK::Trace::TracerFactory.new -factory.add_span_processor( - SDK::Trace::Export::SimpleSpanProcessor.new( - SDK::Trace::Export::ConsoleSpanExporter.new - ) -) +Bundler.require # setup fake rack application: -builder = Rack::Builder.new do - # can't use TracerMiddleware constant before calling Adapters::Rack.install: - # use OpenTelemetry::Adapters::Rack::Middlewares::TracerMiddleware -end +builder = Rack::Builder.new app = ->(_env) { [200, { 'Content-Type' => 'text/plain' }, ['All responses are OK']] } builder.run app -# demonstrate rack configuration options: -config = {} -config[:retain_middleware_names] = true -config[:application] = builder -config[:record_frontend_span] = true -OpenTelemetry::Adapters::Rack::Adapter.instance.install(config) - -# integrate/activate tracing middleware: -builder.use OpenTelemetry::Adapters::Rack::Middlewares::TracerMiddleware +OpenTelemetry::SDK.configure do |c| + c.use 'OpenTelemetry::Adapters::Rack', retain_middleware_names: true, + application: builder, + record_frontend_span: true +end +# demonstrate tracing (span output to console): puts Rack::MockRequest.new(builder).get('/') diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb b/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb index 507ca5c28..d0f9bf732 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb @@ -14,10 +14,11 @@ class Adapter < OpenTelemetry::Instrumentation::Adapter require_dependencies retain_middleware_names if config[:retain_middleware_names] + config[:application]&.use Middlewares::TracerMiddleware end present do - Gem.loaded_specs.include?('rack') + defined?(::Rack) end private diff --git a/adapters/rack/test/opentelemetry/adapters/rack/adapter_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/adapter_test.rb index 3bf4d494b..663611d03 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/adapter_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/adapter_test.rb @@ -37,6 +37,8 @@ class MyAppClass attr_reader :env + def use(*); end + def call(env) @env = env [200, { 'Content-Type' => 'text/plain' }, ['OK']] From 87e74b861483c12f46677dbb0c34931ce4d24f49 Mon Sep 17 00:00:00 2001 From: David Davis Date: Thu, 30 Jan 2020 17:39:01 -0800 Subject: [PATCH 28/64] Rewrite examples * one that is simple, no config options * demonstrate integration via 'Rack::Builder#use' * this is how ddtrace is currently working * demonstrate integration using config[:application] * ultimate goal: 0 config (automagic integration) --- adapters/rack/example/trace_demonstration.rb | 17 +++++++----- adapters/rack/example/trace_demonstration2.rb | 26 +++++++++++++++++++ 2 files changed, 36 insertions(+), 7 deletions(-) create mode 100644 adapters/rack/example/trace_demonstration2.rb diff --git a/adapters/rack/example/trace_demonstration.rb b/adapters/rack/example/trace_demonstration.rb index 2ebe3e5ec..30fcb1872 100644 --- a/adapters/rack/example/trace_demonstration.rb +++ b/adapters/rack/example/trace_demonstration.rb @@ -9,15 +9,18 @@ Bundler.require +OpenTelemetry::SDK.configure do |c| + c.use 'OpenTelemetry::Adapters::Rack' +end + # setup fake rack application: -builder = Rack::Builder.new -app = ->(_env) { [200, { 'Content-Type' => 'text/plain' }, ['All responses are OK']] } -builder.run app +builder = Rack::Builder.app do + # integration should be automatic in web frameworks (like rails), + # but for a plain Rack application, enable it in your config.ru, e.g., + use OpenTelemetry::Adapters::Rack::Middlewares::TracerMiddleware -OpenTelemetry::SDK.configure do |c| - c.use 'OpenTelemetry::Adapters::Rack', retain_middleware_names: true, - application: builder, - record_frontend_span: true + app = ->(_env) { [200, { 'Content-Type' => 'text/plain' }, ['All responses are OK']] } + run app end # demonstrate tracing (span output to console): diff --git a/adapters/rack/example/trace_demonstration2.rb b/adapters/rack/example/trace_demonstration2.rb new file mode 100644 index 000000000..c050c3a25 --- /dev/null +++ b/adapters/rack/example/trace_demonstration2.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +# Copyright 2020 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +require 'rubygems' +require 'bundler/setup' + +Bundler.require + +# setup fake rack application: +builder = Rack::Builder.new +app = ->(_env) { [200, { 'Content-Type' => 'text/plain' }, ['All responses are OK']] } +builder.run app + + +# demonstrate integration using config[:application] instead of 'builder.use': +OpenTelemetry::SDK.configure do |c| + c.use 'OpenTelemetry::Adapters::Rack', retain_middleware_names: true, + application: builder, + record_frontend_span: true +end + +# demonstrate tracing (span output to console): +puts Rack::MockRequest.new(builder).get('/') From de910256a88ed1d52c6dd430fac3807cfe7be05d Mon Sep 17 00:00:00 2001 From: David Davis Date: Fri, 31 Jan 2020 17:43:30 -0800 Subject: [PATCH 29/64] Automatically patch Rack::Builder * in tests, keep an unpatched ('untainted') version of Rack::Builder, restore before and after each test * fix: ruby2.4: private method `define_method' called --- adapters/rack/example/trace_demonstration.rb | 4 --- adapters/rack/example/trace_demonstration2.rb | 1 - .../opentelemetry/adapters/rack/adapter.rb | 9 ++++- .../adapters/rack/patches/rack_builder.rb | 33 +++++++++++++++++++ .../middlewares/tracer_middleware_test.rb | 3 +- .../rack/patches/rack_builder_test.rb | 33 +++++++++++++++++++ adapters/rack/test/test_helper.rb | 25 ++++++++++++++ 7 files changed, 100 insertions(+), 8 deletions(-) create mode 100644 adapters/rack/lib/opentelemetry/adapters/rack/patches/rack_builder.rb create mode 100644 adapters/rack/test/opentelemetry/adapters/rack/patches/rack_builder_test.rb diff --git a/adapters/rack/example/trace_demonstration.rb b/adapters/rack/example/trace_demonstration.rb index 30fcb1872..d921bb027 100644 --- a/adapters/rack/example/trace_demonstration.rb +++ b/adapters/rack/example/trace_demonstration.rb @@ -15,10 +15,6 @@ # setup fake rack application: builder = Rack::Builder.app do - # integration should be automatic in web frameworks (like rails), - # but for a plain Rack application, enable it in your config.ru, e.g., - use OpenTelemetry::Adapters::Rack::Middlewares::TracerMiddleware - app = ->(_env) { [200, { 'Content-Type' => 'text/plain' }, ['All responses are OK']] } run app end diff --git a/adapters/rack/example/trace_demonstration2.rb b/adapters/rack/example/trace_demonstration2.rb index c050c3a25..cbcd001a2 100644 --- a/adapters/rack/example/trace_demonstration2.rb +++ b/adapters/rack/example/trace_demonstration2.rb @@ -14,7 +14,6 @@ app = ->(_env) { [200, { 'Content-Type' => 'text/plain' }, ['All responses are OK']] } builder.run app - # demonstrate integration using config[:application] instead of 'builder.use': OpenTelemetry::SDK.configure do |c| c.use 'OpenTelemetry::Adapters::Rack', retain_middleware_names: true, diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb b/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb index d0f9bf732..e4dfde888 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb @@ -14,7 +14,14 @@ class Adapter < OpenTelemetry::Instrumentation::Adapter require_dependencies retain_middleware_names if config[:retain_middleware_names] - config[:application]&.use Middlewares::TracerMiddleware + + if (app = config[:application]) + app.use Middlewares::TracerMiddleware + else + # monkey patch Rack::Builder: + require_relative 'patches/rack_builder' + ::Rack::Builder.prepend Rack::Patches::RackBuilder + end end present do diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/patches/rack_builder.rb b/adapters/rack/lib/opentelemetry/adapters/rack/patches/rack_builder.rb new file mode 100644 index 000000000..829806d87 --- /dev/null +++ b/adapters/rack/lib/opentelemetry/adapters/rack/patches/rack_builder.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +# Copyright 2020 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +require 'rack/builder' + +module OpenTelemetry + module Adapters + module Rack + module Patches + # Allows Rack::Builder to use middleware by default + module RackBuilder + def self.prepended(base) + old_initialize = base.instance_method(:initialize) + + # NOTE: define_method is private in ruby-2.4: + base.send(:define_method, :initialize) do |*args, &block| + # pass original args to 'super': + old_initialize.bind(self).call(args, &block) + + # add tracer middleware to default stack, once: + use Middlewares::TracerMiddleware if @use.empty? + end + end + end + end + end + end +end + +require_relative '../middlewares/tracer_middleware' diff --git a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb index db86dc12e..d63666fe2 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb @@ -40,9 +40,8 @@ # clear out cached config: described_class.send(:clear_cached_config) - # integrate tracer middleware: + # tracer middleware (automatically integrated): rack_builder.run app - rack_builder.use described_class end after do diff --git a/adapters/rack/test/opentelemetry/adapters/rack/patches/rack_builder_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/patches/rack_builder_test.rb new file mode 100644 index 000000000..95d4c6d56 --- /dev/null +++ b/adapters/rack/test/opentelemetry/adapters/rack/patches/rack_builder_test.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +# Copyright 2020 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +require 'test_helper' + +require_relative '../../../../../lib/opentelemetry/adapters/rack' +require_relative '../../../../../lib/opentelemetry/adapters/rack/patches/rack_builder' + +describe OpenTelemetry::Adapters::Rack::Patches::RackBuilder do + let(:adapter_module) { OpenTelemetry::Adapters::Rack } + let(:adapter_class) { adapter_module::Adapter } + let(:adapter) { adapter_class.instance } + + let(:config) { {} } + + before do + # allow for adapter re-installation: + adapter.instance_variable_set('@installed', false) + end + + describe 'default installation' do + it 'adds middleware to default stack' do + _(::Rack::Builder.new.instance_variable_get(:@use).length).must_equal 0 + + adapter.install(config) + + _(::Rack::Builder.new.instance_variable_get(:@use).length).must_equal 1 + end + end +end diff --git a/adapters/rack/test/test_helper.rb b/adapters/rack/test/test_helper.rb index 188e350fe..62d6a48c0 100644 --- a/adapters/rack/test/test_helper.rb +++ b/adapters/rack/test/test_helper.rb @@ -20,3 +20,28 @@ end EXPORTER = exporter + +### "un-patch" Rack::Builder: +# +require 'rack/builder' +UNTAINTED_RACK_BUILDER = ::Rack::Builder.dup + +module SafeRackBuilder + def after_setup + super + ::Rack.send(:remove_const, :Builder) + ::Rack.const_set(:Builder, UNTAINTED_RACK_BUILDER.dup) + end + + def after_teardown + super + Rack.send(:remove_const, :Builder) + Rack.const_set(:Builder, UNTAINTED_RACK_BUILDER.dup) + end +end + +module Minitest + class Test + include SafeRackBuilder + end +end From e9c021b45c3bfba93accb37e5845acdb78c2a065 Mon Sep 17 00:00:00 2001 From: David Davis Date: Mon, 3 Feb 2020 11:56:05 -0800 Subject: [PATCH 30/64] Port ddtrace Quantization::HTTP --- .../adapters/rack/util/quantization.rb | 97 ++++++++ .../adapters/rack/util/quantization_test.rb | 235 ++++++++++++++++++ 2 files changed, 332 insertions(+) create mode 100644 adapters/rack/lib/opentelemetry/adapters/rack/util/quantization.rb create mode 100644 adapters/rack/test/opentelemetry/adapters/rack/util/quantization_test.rb diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/util/quantization.rb b/adapters/rack/lib/opentelemetry/adapters/rack/util/quantization.rb new file mode 100644 index 000000000..aa89b8941 --- /dev/null +++ b/adapters/rack/lib/opentelemetry/adapters/rack/util/quantization.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +# Copyright 2020 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +require 'uri' +require 'set' + +module OpenTelemetry + module Adapters + module Rack + module Util + # Quantization for HTTP resources + module Quantization + PLACEHOLDER = '?' + + module_function + + def url(url, options = {}) + url!(url, options) + rescue StandardError + options[:placeholder] || PLACEHOLDER + end + + def url!(url, options = {}) + options ||= {} + + URI.parse(url).tap do |uri| + # Format the query string + if uri.query + query = query(uri.query, options[:query]) + uri.query = (!query.nil? && query.empty? ? nil : query) + end + + # Remove any URI framents + uri.fragment = nil unless options[:fragment] == :show + end.to_s + end + + def query(query, options = {}) + query!(query, options) + rescue StandardError + options[:placeholder] || PLACEHOLDER + end + + def query!(query, options = {}) # rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity + options ||= {} + options[:show] = options[:show] || [] + options[:exclude] = options[:exclude] || [] + + # Short circuit if query string is meant to exclude everything + # or if the query string is meant to include everything + return '' if options[:exclude] == :all + return query if options[:show] == :all + + collect_query(query, uniq: true) do |key, value| + if options[:exclude].include?(key) + [nil, nil] + else + value = options[:show].include?(key) ? value : nil + [key, value] + end + end + end + + # Iterate over each key value pair, yielding to the block given. + # Accepts :uniq option, which keeps uniq copies of keys without values. + # e.g. Reduces "foo&bar=bar&bar=bar&foo" to "foo&bar=bar&bar=bar" + def collect_query(query, options = {}) # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity + return query unless block_given? + + uniq = options[:uniq].nil? ? false : options[:uniq] + keys = Set.new + + delims = query.scan(/(^|&|;)/).flatten + query.split(/[&;]/).collect.with_index do |pairs, i| + key, value = pairs.split('=', 2) + key, value = yield(key, value, delims[i]) + if uniq && keys.include?(key) + '' + elsif key && value + "#{delims[i]}#{key}=#{value}" + elsif key + "#{delims[i]}#{key}".tap { keys << key } + else + '' + end + end.join.sub(/^[&;]/, '') + end + + private_class_method :collect_query + end + end + end + end +end diff --git a/adapters/rack/test/opentelemetry/adapters/rack/util/quantization_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/util/quantization_test.rb new file mode 100644 index 000000000..6463169f4 --- /dev/null +++ b/adapters/rack/test/opentelemetry/adapters/rack/util/quantization_test.rb @@ -0,0 +1,235 @@ +# frozen_string_literal: true + +# Copyright 2020 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +require 'test_helper' + +require_relative '../../../../../lib/opentelemetry/adapters/rack/util/quantization' + +describe OpenTelemetry::Adapters::Rack::Util::Quantization do + let(:described_class) { OpenTelemetry::Adapters::Rack::Util::Quantization } + + describe '#url' do + let(:result) { described_class.url(url, options) } + let(:options) { {} } + + describe 'given a URL' do + let(:url) { 'http://example.com/path?category_id=1&sort_by=asc#featured' } + + describe 'default behavior' do + it { _(result).must_equal('http://example.com/path?category_id&sort_by') } + end + + describe 'default behavior for an array' do + let(:url) { 'http://example.com/path?categories[]=1&categories[]=2' } + it { _(result).must_equal('http://example.com/path?categories[]') } + end + + describe 'with query: show: value' do + let(:options) { { query: { show: ['category_id'] } } } + it { _(result).must_equal('http://example.com/path?category_id=1&sort_by') } + end + + describe 'with query: show: :all' do + let(:options) { { query: { show: :all } } } + it { _(result).must_equal('http://example.com/path?category_id=1&sort_by=asc') } + end + + describe 'with query: exclude: value' do + let(:options) { { query: { exclude: ['sort_by'] } } } + it { _(result).must_equal('http://example.com/path?category_id') } + end + + describe 'with query: exclude: :all' do + let(:options) { { query: { exclude: :all } } } + it { _(result).must_equal('http://example.com/path') } + end + + describe 'with show: :all' do + let(:options) { { fragment: :show } } + it { _(result).must_equal('http://example.com/path?category_id&sort_by#featured') } + end + + describe 'with Unicode characters' do + # URLs do not permit unencoded non-ASCII characters in the URL. + let(:url) { 'http://example.com/path?繋がってて' } + it { _(result).must_equal(described_class::PLACEHOLDER) } + end + end + end + + describe '#query' do + let(:result) { described_class.query(query, options) } + + describe 'given a query' do + describe 'and no options' do + let(:options) { {} } + + describe 'with a single parameter' do + let(:query) { 'foo=foo' } + it { _(result).must_equal('foo') } + + describe 'with an invalid byte sequence' do + # \255 is off-limits https://en.wikipedia.org/wiki/UTF-8#Codepage_layout + # There isn't a graceful way to handle this without stripping interesting + # characters out either; so just raise an error and default to the placeholder. + let(:query) { "foo\255=foo" } + it { _(result).must_equal('?') } + end + end + + describe 'with multiple parameters' do + let(:query) { 'foo=foo&bar=bar' } + it { _(result).must_equal('foo&bar') } + end + + describe 'with array-style parameters' do + let(:query) { 'foo[]=bar&foo[]=baz' } + it { _(result).must_equal('foo[]') } + end + + describe 'with semi-colon style parameters' do + let(:query) { 'foo;bar' } + # Notice semicolons aren't preseved... no great way of handling this. + # Semicolons are illegal as of 2014... so this is an edge case. + # See https://www.w3.org/TR/2014/REC-html5-20141028/forms.html#url-encoded-form-data + it { _(result).must_equal('foo;bar') } + end + + describe 'with object-style parameters' do + let(:query) { 'user[id]=1&user[name]=Nathan' } + it { _(result).must_equal('user[id]&user[name]') } + + describe 'that are complex' do + let(:query) { 'users[][id]=1&users[][name]=Nathan&users[][id]=2&users[][name]=Emma' } + it { _(result).must_equal('users[][id]&users[][name]') } + end + end + end + + describe 'and a show: :all option' do + let(:query) { 'foo=foo&bar=bar' } + let(:options) { { show: :all } } + it { _(result).must_equal(query) } + end + + describe 'and a show option' do + describe 'with a single parameter' do + let(:query) { 'foo=foo' } + let(:key) { 'foo' } + let(:options) { { show: [key] } } + it { _(result).must_equal('foo=foo') } + + describe 'that has a Unicode key' do + let(:query) { '繋=foo' } + let(:key) { '繋' } + it { _(result).must_equal('繋=foo') } + + describe 'that is encoded' do + let(:query) { '%E7%B9%8B=foo' } + let(:key) { '%E7%B9%8B' } + it { _(result).must_equal('%E7%B9%8B=foo') } + end + end + + describe 'that has a Unicode value' do + let(:query) { 'foo=繋' } + let(:key) { 'foo' } + it { _(result).must_equal('foo=繋') } + + describe 'that is encoded' do + let(:query) { 'foo=%E7%B9%8B' } + it { _(result).must_equal('foo=%E7%B9%8B') } + end + end + + describe 'that has a Unicode key and value' do + let(:query) { '繋=繋' } + let(:key) { '繋' } + it { _(result).must_equal('繋=繋') } + + describe 'that is encoded' do + let(:query) { '%E7%B9%8B=%E7%B9%8B' } + let(:key) { '%E7%B9%8B' } + it { _(result).must_equal('%E7%B9%8B=%E7%B9%8B') } + end + end + end + + describe 'with multiple parameters' do + let(:query) { 'foo=foo&bar=bar' } + let(:options) { { show: ['foo'] } } + it { _(result).must_equal('foo=foo&bar') } + end + + describe 'with array-style parameters' do + let(:query) { 'foo[]=bar&foo[]=baz' } + let(:options) { { show: ['foo[]'] } } + it { _(result).must_equal('foo[]=bar&foo[]=baz') } + + describe 'that contains encoded braces' do + let(:query) { 'foo[]=%5Bbar%5D&foo[]=%5Bbaz%5D' } + it { _(result).must_equal('foo[]=%5Bbar%5D&foo[]=%5Bbaz%5D') } + + describe 'that exactly matches the key' do + let(:query) { 'foo[]=foo%5B%5D&foo[]=foo%5B%5D' } + it { _(result).must_equal('foo[]=foo%5B%5D&foo[]=foo%5B%5D') } + end + end + end + + describe 'with object-style parameters' do + let(:query) { 'user[id]=1&user[name]=Nathan' } + let(:options) { { show: ['user[id]'] } } + it { _(result).must_equal('user[id]=1&user[name]') } + + describe 'that are complex' do + let(:query) { 'users[][id]=1&users[][name]=Nathan&users[][id]=2&users[][name]=Emma' } + let(:options) { { show: ['users[][id]'] } } + it { _(result).must_equal('users[][id]=1&users[][name]&users[][id]=2') } + end + end + end + + describe 'and an exclude: :all option' do + let(:query) { 'foo=foo&bar=bar' } + let(:options) { { exclude: :all } } + it { _(result).must_equal('') } + end + + describe 'and an exclude option' do + describe 'with a single parameter' do + let(:query) { 'foo=foo' } + let(:options) { { exclude: ['foo'] } } + it { _(result).must_equal('') } + end + + describe 'with multiple parameters' do + let(:query) { 'foo=foo&bar=bar' } + let(:options) { { exclude: ['foo'] } } + it { _(result).must_equal('bar') } + end + + describe 'with array-style parameters' do + let(:query) { 'foo[]=bar&foo[]=baz' } + let(:options) { { exclude: ['foo[]'] } } + it { _(result).must_equal('') } + end + + describe 'with object-style parameters' do + let(:query) { 'user[id]=1&user[name]=Nathan' } + let(:options) { { exclude: ['user[name]'] } } + it { _(result).must_equal('user[id]') } + + describe 'that are complex' do + let(:query) { 'users[][id]=1&users[][name]=Nathan&users[][id]=2&users[][name]=Emma' } + let(:options) { { exclude: ['users[][name]'] } } + it { _(result).must_equal('users[][id]') } + end + end + end + end + end +end From 99f44e847beac5c8ed15b0f28e926def81029546 Mon Sep 17 00:00:00 2001 From: David Davis Date: Mon, 3 Feb 2020 12:18:44 -0800 Subject: [PATCH 31/64] Integrate Util::Quantization --- .../lib/opentelemetry/adapters/rack/adapter.rb | 6 ++++++ .../rack/middlewares/tracer_middleware_test.rb | 16 +++++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb b/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb index e4dfde888..ba1f33768 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb @@ -14,6 +14,7 @@ class Adapter < OpenTelemetry::Instrumentation::Adapter require_dependencies retain_middleware_names if config[:retain_middleware_names] + configure_default_quantization if (app = config[:application]) app.use Middlewares::TracerMiddleware @@ -32,6 +33,7 @@ class Adapter < OpenTelemetry::Instrumentation::Adapter def require_dependencies require_relative 'middlewares/tracer_middleware' + require_relative 'util/quantization' end MissingApplicationError = Class.new(StandardError) @@ -57,6 +59,10 @@ def call(env) next_middleware.instance_variable_get('@app') end end + + def configure_default_quantization + config[:url_quantization] ||= ->(url) { Util::Quantization.url(url, config[:quantization_options]) } + end end end end diff --git a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb index d63666fe2..de092cb09 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb @@ -9,6 +9,7 @@ # require Adapter so .install method is found: require_relative '../../../../../lib/opentelemetry/adapters/rack' require_relative '../../../../../lib/opentelemetry/adapters/rack/adapter' +require_relative '../../../../../lib/opentelemetry/adapters/rack/util/quantization' require_relative '../../../../../lib/opentelemetry/adapters/rack/middlewares/tracer_middleware' describe OpenTelemetry::Adapters::Rack::Middlewares::TracerMiddleware do @@ -146,7 +147,7 @@ describe 'config[:quantization]' do before do - Rack::MockRequest.new(rack_builder).get('/really_long_url', env) + Rack::MockRequest.new(rack_builder).get('/really_long_url?category_id=1&sort_by=asc#featured', env) end describe 'without quantization' do @@ -165,6 +166,19 @@ it 'mutates url according to url_quantization' do _(first_span.name).must_equal '/reall' end + + describe 'with more complex quantization' do + let(:quantization_options) do + { query: { exclude: :all } } + end + let(:quantization_example) do + ->(url) { adapter_module::Util::Quantization.url(url, quantization_options) } + end + + it 'mutates url' do + _(first_span.name).must_equal '/really_long_url' + end + end end end From 33fe594427240a014e308e070f97508a8e110138 Mon Sep 17 00:00:00 2001 From: David Davis Date: Mon, 3 Feb 2020 19:00:28 -0800 Subject: [PATCH 32/64] Revert "Automatically patch Rack::Builder" This reverts commit de910256a88ed1d52c6dd430fac3807cfe7be05d. # Conflicts: # adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb --- adapters/rack/example/trace_demonstration.rb | 4 +++ .../opentelemetry/adapters/rack/adapter.rb | 8 +---- .../adapters/rack/patches/rack_builder.rb | 33 ------------------- .../middlewares/tracer_middleware_test.rb | 3 +- .../rack/patches/rack_builder_test.rb | 33 ------------------- adapters/rack/test/test_helper.rb | 25 -------------- 6 files changed, 7 insertions(+), 99 deletions(-) delete mode 100644 adapters/rack/lib/opentelemetry/adapters/rack/patches/rack_builder.rb delete mode 100644 adapters/rack/test/opentelemetry/adapters/rack/patches/rack_builder_test.rb diff --git a/adapters/rack/example/trace_demonstration.rb b/adapters/rack/example/trace_demonstration.rb index d921bb027..30fcb1872 100644 --- a/adapters/rack/example/trace_demonstration.rb +++ b/adapters/rack/example/trace_demonstration.rb @@ -15,6 +15,10 @@ # setup fake rack application: builder = Rack::Builder.app do + # integration should be automatic in web frameworks (like rails), + # but for a plain Rack application, enable it in your config.ru, e.g., + use OpenTelemetry::Adapters::Rack::Middlewares::TracerMiddleware + app = ->(_env) { [200, { 'Content-Type' => 'text/plain' }, ['All responses are OK']] } run app end diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb b/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb index ba1f33768..b7602282b 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb @@ -16,13 +16,7 @@ class Adapter < OpenTelemetry::Instrumentation::Adapter retain_middleware_names if config[:retain_middleware_names] configure_default_quantization - if (app = config[:application]) - app.use Middlewares::TracerMiddleware - else - # monkey patch Rack::Builder: - require_relative 'patches/rack_builder' - ::Rack::Builder.prepend Rack::Patches::RackBuilder - end + config[:application]&.use Middlewares::TracerMiddleware end present do diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/patches/rack_builder.rb b/adapters/rack/lib/opentelemetry/adapters/rack/patches/rack_builder.rb deleted file mode 100644 index 829806d87..000000000 --- a/adapters/rack/lib/opentelemetry/adapters/rack/patches/rack_builder.rb +++ /dev/null @@ -1,33 +0,0 @@ -# frozen_string_literal: true - -# Copyright 2020 OpenTelemetry Authors -# -# SPDX-License-Identifier: Apache-2.0 - -require 'rack/builder' - -module OpenTelemetry - module Adapters - module Rack - module Patches - # Allows Rack::Builder to use middleware by default - module RackBuilder - def self.prepended(base) - old_initialize = base.instance_method(:initialize) - - # NOTE: define_method is private in ruby-2.4: - base.send(:define_method, :initialize) do |*args, &block| - # pass original args to 'super': - old_initialize.bind(self).call(args, &block) - - # add tracer middleware to default stack, once: - use Middlewares::TracerMiddleware if @use.empty? - end - end - end - end - end - end -end - -require_relative '../middlewares/tracer_middleware' diff --git a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb index de092cb09..c5cd5f035 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb @@ -41,8 +41,9 @@ # clear out cached config: described_class.send(:clear_cached_config) - # tracer middleware (automatically integrated): + # integrate tracer middleware: rack_builder.run app + rack_builder.use described_class end after do diff --git a/adapters/rack/test/opentelemetry/adapters/rack/patches/rack_builder_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/patches/rack_builder_test.rb deleted file mode 100644 index 95d4c6d56..000000000 --- a/adapters/rack/test/opentelemetry/adapters/rack/patches/rack_builder_test.rb +++ /dev/null @@ -1,33 +0,0 @@ -# frozen_string_literal: true - -# Copyright 2020 OpenTelemetry Authors -# -# SPDX-License-Identifier: Apache-2.0 - -require 'test_helper' - -require_relative '../../../../../lib/opentelemetry/adapters/rack' -require_relative '../../../../../lib/opentelemetry/adapters/rack/patches/rack_builder' - -describe OpenTelemetry::Adapters::Rack::Patches::RackBuilder do - let(:adapter_module) { OpenTelemetry::Adapters::Rack } - let(:adapter_class) { adapter_module::Adapter } - let(:adapter) { adapter_class.instance } - - let(:config) { {} } - - before do - # allow for adapter re-installation: - adapter.instance_variable_set('@installed', false) - end - - describe 'default installation' do - it 'adds middleware to default stack' do - _(::Rack::Builder.new.instance_variable_get(:@use).length).must_equal 0 - - adapter.install(config) - - _(::Rack::Builder.new.instance_variable_get(:@use).length).must_equal 1 - end - end -end diff --git a/adapters/rack/test/test_helper.rb b/adapters/rack/test/test_helper.rb index 62d6a48c0..188e350fe 100644 --- a/adapters/rack/test/test_helper.rb +++ b/adapters/rack/test/test_helper.rb @@ -20,28 +20,3 @@ end EXPORTER = exporter - -### "un-patch" Rack::Builder: -# -require 'rack/builder' -UNTAINTED_RACK_BUILDER = ::Rack::Builder.dup - -module SafeRackBuilder - def after_setup - super - ::Rack.send(:remove_const, :Builder) - ::Rack.const_set(:Builder, UNTAINTED_RACK_BUILDER.dup) - end - - def after_teardown - super - Rack.send(:remove_const, :Builder) - Rack.const_set(:Builder, UNTAINTED_RACK_BUILDER.dup) - end -end - -module Minitest - class Test - include SafeRackBuilder - end -end From 1ce1bf54eb36e555851cf8f22e9008a7ab08e2f8 Mon Sep 17 00:00:00 2001 From: David Davis Date: Wed, 5 Feb 2020 11:18:06 -0800 Subject: [PATCH 33/64] Add missing files needed for Bundler.require --- adapters/rack/lib/opentelemetry-adapters-rack.rb | 7 +++++++ adapters/rack/lib/opentelemetry/adapters.rb | 13 +++++++++++++ 2 files changed, 20 insertions(+) create mode 100644 adapters/rack/lib/opentelemetry-adapters-rack.rb create mode 100644 adapters/rack/lib/opentelemetry/adapters.rb diff --git a/adapters/rack/lib/opentelemetry-adapters-rack.rb b/adapters/rack/lib/opentelemetry-adapters-rack.rb new file mode 100644 index 000000000..a2f3a3444 --- /dev/null +++ b/adapters/rack/lib/opentelemetry-adapters-rack.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +# Copyright 2020 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +require_relative './opentelemetry/adapters' diff --git a/adapters/rack/lib/opentelemetry/adapters.rb b/adapters/rack/lib/opentelemetry/adapters.rb new file mode 100644 index 000000000..aa64071a4 --- /dev/null +++ b/adapters/rack/lib/opentelemetry/adapters.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +# Copyright 2020 OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +module OpenTelemetry + # Adapters should be able to handle the case when the library is not installed on a user's system. + module Adapters + end +end + +require_relative './adapters/rack' From 8a34ab2fdb4afb30ed4bb4531d3a66b7b893bb66 Mon Sep 17 00:00:00 2001 From: David Davis Date: Wed, 5 Feb 2020 14:14:57 -0800 Subject: [PATCH 34/64] Update Rakefile --- Rakefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Rakefile b/Rakefile index 431dead6d..0ceb6df40 100644 --- a/Rakefile +++ b/Rakefile @@ -68,7 +68,7 @@ GEM_INFO = { require './lib/opentelemetry/adapters/rack/version.rb' OpenTelemetry::Adapters::Rack::VERSION } - } + }, "opentelemetry-adapters-sinatra" => { version_getter: ->() { require './lib/opentelemetry/adapters/sinatra/version.rb' From 634464217390af87f99894bc2e7c30122bc58e93 Mon Sep 17 00:00:00 2001 From: David Davis Date: Wed, 5 Feb 2020 14:52:52 -0800 Subject: [PATCH 35/64] Avoid patching config[:application] during installation * config[:application] is patched in retain_middleware_names, in which case it is required if config[:retain_middleware_names] is truthy * goal: leave .use call to the user --- adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb b/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb index b7602282b..5124df8cf 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb @@ -15,8 +15,6 @@ class Adapter < OpenTelemetry::Instrumentation::Adapter retain_middleware_names if config[:retain_middleware_names] configure_default_quantization - - config[:application]&.use Middlewares::TracerMiddleware end present do From de81be28924811ab02e8db0b3eb1e5ed71b4e2a0 Mon Sep 17 00:00:00 2001 From: David Davis Date: Wed, 5 Feb 2020 15:10:10 -0800 Subject: [PATCH 36/64] Refine/optimize allowed_request_headers --- .../rack/middlewares/tracer_middleware.rb | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 870df8a68..60f6840be 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -20,11 +20,10 @@ module Middlewares class TracerMiddleware # rubocop:disable Metrics/ClassLength class << self def allowed_rack_request_headers - @allowed_rack_request_headers ||= allowed_request_header_names.map do |header| - { - header: header, - rack_header: "HTTP_#{header.to_s.upcase.gsub(/[-\s]/, '_')}" - } + @allowed_rack_request_headers ||= {}.tap do |result| + allowed_request_header_names.each do |header| + result["HTTP_#{header.to_s.upcase.gsub(/[-\s]/, '_')}"] = build_attribute_name('http.request.headers.', header) + end end end @@ -232,8 +231,8 @@ def set_attributes_after_request(span, status, headers, _response) # @return Hash def allowed_request_headers(env) {}.tap do |result| - self.class.allowed_rack_request_headers.each do |hash| - result[self.class.build_attribute_name('http.request.headers.', hash[:header])] = env[hash[:rack_header]] if env.key?(hash[:rack_header]) + self.class.allowed_rack_request_headers.each do |key, value| + result[value] = env[key] if env.key?(key) end end end @@ -245,7 +244,7 @@ def allowed_response_headers(headers) {}.tap do |result| self.class.allowed_response_headers.each do |hash| key = headers.keys.find { |k| hash[:search_for_keys].find { |k2| k == k2 } } || - headers.keys.find { |k| hash[:search_for_keys].find { |k2| k.upcase == k2 } } + headers.keys.find { |k| hash[:search_for_keys].find { |k2| k.upcase == k2 } } next unless key From 5479ad0c331acd137739732f255c1be052304b32 Mon Sep 17 00:00:00 2001 From: David Davis Date: Wed, 5 Feb 2020 15:47:11 -0800 Subject: [PATCH 37/64] Refine/optimize allowed_response_headers --- .../rack/middlewares/tracer_middleware.rb | 28 ++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 60f6840be..3bc14b3b3 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -32,12 +32,11 @@ def allowed_request_header_names end def allowed_response_headers - @allowed_response_headers ||= allowed_response_header_names.map do |header| - { - header: header, - attribute_name: build_attribute_name('http.response.headers.', header), - search_for_keys: [header, header.to_s.upcase] - } + @allowed_response_headers ||= {}.tap do |result| + allowed_response_header_names.each do |header| + result[header] = build_attribute_name('http.response.headers.', header) + result[header.to_s.upcase] = build_attribute_name('http.response.headers.', header) + end end end @@ -242,16 +241,13 @@ def allowed_response_headers(headers) return EMPTY_HASH if headers.nil? {}.tap do |result| - self.class.allowed_response_headers.each do |hash| - key = headers.keys.find { |k| hash[:search_for_keys].find { |k2| k == k2 } } || - headers.keys.find { |k| hash[:search_for_keys].find { |k2| k.upcase == k2 } } - - next unless key - - # cache string for next time: - hash[:search_for_keys] |= [key] - - result[hash[:attribute_name]] = headers[key] + self.class.allowed_response_headers.each do |key, value| + if headers.key?(key) + result[value] = headers[key] + else + case_insensitive_entry = headers.detect { |h| h.first.upcase == key } + result[value] = case_insensitive_entry&.last + end end end end From edb95f98815e64955f1c9ad33130d2abf85ff544 Mon Sep 17 00:00:00 2001 From: David Davis Date: Wed, 5 Feb 2020 16:09:06 -0800 Subject: [PATCH 38/64] Avoid circular require --- adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb b/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb index 5124df8cf..2c2bb5b0f 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb @@ -4,6 +4,8 @@ # # SPDX-License-Identifier: Apache-2.0 +require 'opentelemetry' + module OpenTelemetry module Adapters module Rack @@ -58,6 +60,4 @@ def configure_default_quantization end end end -end - -require_relative '../rack' +end \ No newline at end of file From 1399250a93f1a0f56cbacf90e97bada883476c94 Mon Sep 17 00:00:00 2001 From: David Davis Date: Wed, 5 Feb 2020 16:13:44 -0800 Subject: [PATCH 39/64] Use SDK.configure to streamline test_helper.rb setup --- adapters/rack/test/test_helper.rb | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/adapters/rack/test/test_helper.rb b/adapters/rack/test/test_helper.rb index 188e350fe..b13701e24 100644 --- a/adapters/rack/test/test_helper.rb +++ b/adapters/rack/test/test_helper.rb @@ -12,11 +12,9 @@ require 'webmock/minitest' # global opentelemetry-sdk setup: -sdk = OpenTelemetry::SDK -exporter = sdk::Trace::Export::InMemorySpanExporter.new -span_processor = sdk::Trace::Export::SimpleSpanProcessor.new(exporter) -OpenTelemetry.tracer_factory = sdk::Trace::TracerFactory.new.tap do |factory| - factory.add_span_processor(span_processor) -end +EXPORTER = OpenTelemetry::SDK::Trace::Export::InMemorySpanExporter.new +span_processor = OpenTelemetry::SDK::Trace::Export::SimpleSpanProcessor.new(EXPORTER) -EXPORTER = exporter +OpenTelemetry::SDK.configure do |c| + c.add_span_processor span_processor +end From 78acca7cd6d3274f7c0a352107c8413ad528e62f Mon Sep 17 00:00:00 2001 From: David Davis Date: Thu, 6 Feb 2020 11:23:17 -0800 Subject: [PATCH 40/64] Revert "Integrate Util::Quantization" This reverts commit 99f44e847beac5c8ed15b0f28e926def81029546. Discussed in SIG: * avoid eager-quantization (heavy, potentially unwanted) * defer to user preferences (via config) * probably better to err on the side of 'too-specific' vs. 'too-general', since 'too-specific' can be made more general, but maybe not vice-versa # Conflicts: # adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb --- .../lib/opentelemetry/adapters/rack/adapter.rb | 8 +------- .../rack/middlewares/tracer_middleware_test.rb | 16 +--------------- 2 files changed, 2 insertions(+), 22 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb b/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb index 2c2bb5b0f..3e6753e96 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb @@ -16,7 +16,6 @@ class Adapter < OpenTelemetry::Instrumentation::Adapter require_dependencies retain_middleware_names if config[:retain_middleware_names] - configure_default_quantization end present do @@ -27,7 +26,6 @@ class Adapter < OpenTelemetry::Instrumentation::Adapter def require_dependencies require_relative 'middlewares/tracer_middleware' - require_relative 'util/quantization' end MissingApplicationError = Class.new(StandardError) @@ -53,11 +51,7 @@ def call(env) next_middleware.instance_variable_get('@app') end end - - def configure_default_quantization - config[:url_quantization] ||= ->(url) { Util::Quantization.url(url, config[:quantization_options]) } - end end end end -end \ No newline at end of file +end diff --git a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb index c5cd5f035..db86dc12e 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb @@ -9,7 +9,6 @@ # require Adapter so .install method is found: require_relative '../../../../../lib/opentelemetry/adapters/rack' require_relative '../../../../../lib/opentelemetry/adapters/rack/adapter' -require_relative '../../../../../lib/opentelemetry/adapters/rack/util/quantization' require_relative '../../../../../lib/opentelemetry/adapters/rack/middlewares/tracer_middleware' describe OpenTelemetry::Adapters::Rack::Middlewares::TracerMiddleware do @@ -148,7 +147,7 @@ describe 'config[:quantization]' do before do - Rack::MockRequest.new(rack_builder).get('/really_long_url?category_id=1&sort_by=asc#featured', env) + Rack::MockRequest.new(rack_builder).get('/really_long_url', env) end describe 'without quantization' do @@ -167,19 +166,6 @@ it 'mutates url according to url_quantization' do _(first_span.name).must_equal '/reall' end - - describe 'with more complex quantization' do - let(:quantization_options) do - { query: { exclude: :all } } - end - let(:quantization_example) do - ->(url) { adapter_module::Util::Quantization.url(url, quantization_options) } - end - - it 'mutates url' do - _(first_span.name).must_equal '/really_long_url' - end - end end end From a69f94ee834ccd6ca977b2e0abbed99af53d6bf0 Mon Sep 17 00:00:00 2001 From: David Davis Date: Thu, 6 Feb 2020 11:29:15 -0800 Subject: [PATCH 41/64] Revert "Port ddtrace Quantization::HTTP" This reverts commit e9c021b45c3bfba93accb37e5845acdb78c2a065. * cleanup remnants that aren't used for now --- .../adapters/rack/util/quantization.rb | 97 -------- .../adapters/rack/util/quantization_test.rb | 235 ------------------ 2 files changed, 332 deletions(-) delete mode 100644 adapters/rack/lib/opentelemetry/adapters/rack/util/quantization.rb delete mode 100644 adapters/rack/test/opentelemetry/adapters/rack/util/quantization_test.rb diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/util/quantization.rb b/adapters/rack/lib/opentelemetry/adapters/rack/util/quantization.rb deleted file mode 100644 index aa89b8941..000000000 --- a/adapters/rack/lib/opentelemetry/adapters/rack/util/quantization.rb +++ /dev/null @@ -1,97 +0,0 @@ -# frozen_string_literal: true - -# Copyright 2020 OpenTelemetry Authors -# -# SPDX-License-Identifier: Apache-2.0 - -require 'uri' -require 'set' - -module OpenTelemetry - module Adapters - module Rack - module Util - # Quantization for HTTP resources - module Quantization - PLACEHOLDER = '?' - - module_function - - def url(url, options = {}) - url!(url, options) - rescue StandardError - options[:placeholder] || PLACEHOLDER - end - - def url!(url, options = {}) - options ||= {} - - URI.parse(url).tap do |uri| - # Format the query string - if uri.query - query = query(uri.query, options[:query]) - uri.query = (!query.nil? && query.empty? ? nil : query) - end - - # Remove any URI framents - uri.fragment = nil unless options[:fragment] == :show - end.to_s - end - - def query(query, options = {}) - query!(query, options) - rescue StandardError - options[:placeholder] || PLACEHOLDER - end - - def query!(query, options = {}) # rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity - options ||= {} - options[:show] = options[:show] || [] - options[:exclude] = options[:exclude] || [] - - # Short circuit if query string is meant to exclude everything - # or if the query string is meant to include everything - return '' if options[:exclude] == :all - return query if options[:show] == :all - - collect_query(query, uniq: true) do |key, value| - if options[:exclude].include?(key) - [nil, nil] - else - value = options[:show].include?(key) ? value : nil - [key, value] - end - end - end - - # Iterate over each key value pair, yielding to the block given. - # Accepts :uniq option, which keeps uniq copies of keys without values. - # e.g. Reduces "foo&bar=bar&bar=bar&foo" to "foo&bar=bar&bar=bar" - def collect_query(query, options = {}) # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity - return query unless block_given? - - uniq = options[:uniq].nil? ? false : options[:uniq] - keys = Set.new - - delims = query.scan(/(^|&|;)/).flatten - query.split(/[&;]/).collect.with_index do |pairs, i| - key, value = pairs.split('=', 2) - key, value = yield(key, value, delims[i]) - if uniq && keys.include?(key) - '' - elsif key && value - "#{delims[i]}#{key}=#{value}" - elsif key - "#{delims[i]}#{key}".tap { keys << key } - else - '' - end - end.join.sub(/^[&;]/, '') - end - - private_class_method :collect_query - end - end - end - end -end diff --git a/adapters/rack/test/opentelemetry/adapters/rack/util/quantization_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/util/quantization_test.rb deleted file mode 100644 index 6463169f4..000000000 --- a/adapters/rack/test/opentelemetry/adapters/rack/util/quantization_test.rb +++ /dev/null @@ -1,235 +0,0 @@ -# frozen_string_literal: true - -# Copyright 2020 OpenTelemetry Authors -# -# SPDX-License-Identifier: Apache-2.0 - -require 'test_helper' - -require_relative '../../../../../lib/opentelemetry/adapters/rack/util/quantization' - -describe OpenTelemetry::Adapters::Rack::Util::Quantization do - let(:described_class) { OpenTelemetry::Adapters::Rack::Util::Quantization } - - describe '#url' do - let(:result) { described_class.url(url, options) } - let(:options) { {} } - - describe 'given a URL' do - let(:url) { 'http://example.com/path?category_id=1&sort_by=asc#featured' } - - describe 'default behavior' do - it { _(result).must_equal('http://example.com/path?category_id&sort_by') } - end - - describe 'default behavior for an array' do - let(:url) { 'http://example.com/path?categories[]=1&categories[]=2' } - it { _(result).must_equal('http://example.com/path?categories[]') } - end - - describe 'with query: show: value' do - let(:options) { { query: { show: ['category_id'] } } } - it { _(result).must_equal('http://example.com/path?category_id=1&sort_by') } - end - - describe 'with query: show: :all' do - let(:options) { { query: { show: :all } } } - it { _(result).must_equal('http://example.com/path?category_id=1&sort_by=asc') } - end - - describe 'with query: exclude: value' do - let(:options) { { query: { exclude: ['sort_by'] } } } - it { _(result).must_equal('http://example.com/path?category_id') } - end - - describe 'with query: exclude: :all' do - let(:options) { { query: { exclude: :all } } } - it { _(result).must_equal('http://example.com/path') } - end - - describe 'with show: :all' do - let(:options) { { fragment: :show } } - it { _(result).must_equal('http://example.com/path?category_id&sort_by#featured') } - end - - describe 'with Unicode characters' do - # URLs do not permit unencoded non-ASCII characters in the URL. - let(:url) { 'http://example.com/path?繋がってて' } - it { _(result).must_equal(described_class::PLACEHOLDER) } - end - end - end - - describe '#query' do - let(:result) { described_class.query(query, options) } - - describe 'given a query' do - describe 'and no options' do - let(:options) { {} } - - describe 'with a single parameter' do - let(:query) { 'foo=foo' } - it { _(result).must_equal('foo') } - - describe 'with an invalid byte sequence' do - # \255 is off-limits https://en.wikipedia.org/wiki/UTF-8#Codepage_layout - # There isn't a graceful way to handle this without stripping interesting - # characters out either; so just raise an error and default to the placeholder. - let(:query) { "foo\255=foo" } - it { _(result).must_equal('?') } - end - end - - describe 'with multiple parameters' do - let(:query) { 'foo=foo&bar=bar' } - it { _(result).must_equal('foo&bar') } - end - - describe 'with array-style parameters' do - let(:query) { 'foo[]=bar&foo[]=baz' } - it { _(result).must_equal('foo[]') } - end - - describe 'with semi-colon style parameters' do - let(:query) { 'foo;bar' } - # Notice semicolons aren't preseved... no great way of handling this. - # Semicolons are illegal as of 2014... so this is an edge case. - # See https://www.w3.org/TR/2014/REC-html5-20141028/forms.html#url-encoded-form-data - it { _(result).must_equal('foo;bar') } - end - - describe 'with object-style parameters' do - let(:query) { 'user[id]=1&user[name]=Nathan' } - it { _(result).must_equal('user[id]&user[name]') } - - describe 'that are complex' do - let(:query) { 'users[][id]=1&users[][name]=Nathan&users[][id]=2&users[][name]=Emma' } - it { _(result).must_equal('users[][id]&users[][name]') } - end - end - end - - describe 'and a show: :all option' do - let(:query) { 'foo=foo&bar=bar' } - let(:options) { { show: :all } } - it { _(result).must_equal(query) } - end - - describe 'and a show option' do - describe 'with a single parameter' do - let(:query) { 'foo=foo' } - let(:key) { 'foo' } - let(:options) { { show: [key] } } - it { _(result).must_equal('foo=foo') } - - describe 'that has a Unicode key' do - let(:query) { '繋=foo' } - let(:key) { '繋' } - it { _(result).must_equal('繋=foo') } - - describe 'that is encoded' do - let(:query) { '%E7%B9%8B=foo' } - let(:key) { '%E7%B9%8B' } - it { _(result).must_equal('%E7%B9%8B=foo') } - end - end - - describe 'that has a Unicode value' do - let(:query) { 'foo=繋' } - let(:key) { 'foo' } - it { _(result).must_equal('foo=繋') } - - describe 'that is encoded' do - let(:query) { 'foo=%E7%B9%8B' } - it { _(result).must_equal('foo=%E7%B9%8B') } - end - end - - describe 'that has a Unicode key and value' do - let(:query) { '繋=繋' } - let(:key) { '繋' } - it { _(result).must_equal('繋=繋') } - - describe 'that is encoded' do - let(:query) { '%E7%B9%8B=%E7%B9%8B' } - let(:key) { '%E7%B9%8B' } - it { _(result).must_equal('%E7%B9%8B=%E7%B9%8B') } - end - end - end - - describe 'with multiple parameters' do - let(:query) { 'foo=foo&bar=bar' } - let(:options) { { show: ['foo'] } } - it { _(result).must_equal('foo=foo&bar') } - end - - describe 'with array-style parameters' do - let(:query) { 'foo[]=bar&foo[]=baz' } - let(:options) { { show: ['foo[]'] } } - it { _(result).must_equal('foo[]=bar&foo[]=baz') } - - describe 'that contains encoded braces' do - let(:query) { 'foo[]=%5Bbar%5D&foo[]=%5Bbaz%5D' } - it { _(result).must_equal('foo[]=%5Bbar%5D&foo[]=%5Bbaz%5D') } - - describe 'that exactly matches the key' do - let(:query) { 'foo[]=foo%5B%5D&foo[]=foo%5B%5D' } - it { _(result).must_equal('foo[]=foo%5B%5D&foo[]=foo%5B%5D') } - end - end - end - - describe 'with object-style parameters' do - let(:query) { 'user[id]=1&user[name]=Nathan' } - let(:options) { { show: ['user[id]'] } } - it { _(result).must_equal('user[id]=1&user[name]') } - - describe 'that are complex' do - let(:query) { 'users[][id]=1&users[][name]=Nathan&users[][id]=2&users[][name]=Emma' } - let(:options) { { show: ['users[][id]'] } } - it { _(result).must_equal('users[][id]=1&users[][name]&users[][id]=2') } - end - end - end - - describe 'and an exclude: :all option' do - let(:query) { 'foo=foo&bar=bar' } - let(:options) { { exclude: :all } } - it { _(result).must_equal('') } - end - - describe 'and an exclude option' do - describe 'with a single parameter' do - let(:query) { 'foo=foo' } - let(:options) { { exclude: ['foo'] } } - it { _(result).must_equal('') } - end - - describe 'with multiple parameters' do - let(:query) { 'foo=foo&bar=bar' } - let(:options) { { exclude: ['foo'] } } - it { _(result).must_equal('bar') } - end - - describe 'with array-style parameters' do - let(:query) { 'foo[]=bar&foo[]=baz' } - let(:options) { { exclude: ['foo[]'] } } - it { _(result).must_equal('') } - end - - describe 'with object-style parameters' do - let(:query) { 'user[id]=1&user[name]=Nathan' } - let(:options) { { exclude: ['user[name]'] } } - it { _(result).must_equal('user[id]') } - - describe 'that are complex' do - let(:query) { 'users[][id]=1&users[][name]=Nathan&users[][id]=2&users[][name]=Emma' } - let(:options) { { exclude: ['users[][name]'] } } - it { _(result).must_equal('users[][id]') } - end - end - end - end - end -end From 70aa3528fdc254f23cd8cf4859b498725fbe74d8 Mon Sep 17 00:00:00 2001 From: David Davis Date: Tue, 11 Feb 2020 10:08:19 -0800 Subject: [PATCH 42/64] Fix example/trace_demonstration2.rb to integrate explicitly, with 'use' --- adapters/rack/example/trace_demonstration2.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/adapters/rack/example/trace_demonstration2.rb b/adapters/rack/example/trace_demonstration2.rb index cbcd001a2..aa0d52d5d 100644 --- a/adapters/rack/example/trace_demonstration2.rb +++ b/adapters/rack/example/trace_demonstration2.rb @@ -21,5 +21,8 @@ record_frontend_span: true end +# integrate instrumentation explicitly: +builder.use OpenTelemetry::Adapters::Rack::Middlewares::TracerMiddleware + # demonstrate tracing (span output to console): puts Rack::MockRequest.new(builder).get('/') From 457e695dc1e910890799668fc7f67c0fec1d1bfb Mon Sep 17 00:00:00 2001 From: David Davis Date: Tue, 11 Feb 2020 10:12:24 -0800 Subject: [PATCH 43/64] Update example/trace_demonstration2.rb documentation --- adapters/rack/example/trace_demonstration2.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/adapters/rack/example/trace_demonstration2.rb b/adapters/rack/example/trace_demonstration2.rb index aa0d52d5d..8b09ceac7 100644 --- a/adapters/rack/example/trace_demonstration2.rb +++ b/adapters/rack/example/trace_demonstration2.rb @@ -14,7 +14,7 @@ app = ->(_env) { [200, { 'Content-Type' => 'text/plain' }, ['All responses are OK']] } builder.run app -# demonstrate integration using config[:application] instead of 'builder.use': +# demonstrate integration using 'retain_middlware_names' and 'application': OpenTelemetry::SDK.configure do |c| c.use 'OpenTelemetry::Adapters::Rack', retain_middleware_names: true, application: builder, From f78c90ec949d88da53c10263d8a466762c1bcbe4 Mon Sep 17 00:00:00 2001 From: David Davis Date: Tue, 11 Feb 2020 10:25:54 -0800 Subject: [PATCH 44/64] Optimize allowed_response_headers to avoid using Hash#detect --- .../adapters/rack/middlewares/tracer_middleware.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 3bc14b3b3..5793427b1 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -245,8 +245,13 @@ def allowed_response_headers(headers) if headers.key?(key) result[value] = headers[key] else - case_insensitive_entry = headers.detect { |h| h.first.upcase == key } - result[value] = case_insensitive_entry&.last + # do case-insensitive match: + headers.each do |k, v| + if k.upcase == key + result[value] = v + break + end + end end end end From c1a3a6e2f116ac7980b08e43678c5490c43926c4 Mon Sep 17 00:00:00 2001 From: David Davis Date: Tue, 11 Feb 2020 10:33:08 -0800 Subject: [PATCH 45/64] Simplify allowed_{rack,request}_header_names to inline config --- .../rack/middlewares/tracer_middleware.rb | 22 +++++-------------- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 5793427b1..913a0c1af 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -20,30 +20,18 @@ module Middlewares class TracerMiddleware # rubocop:disable Metrics/ClassLength class << self def allowed_rack_request_headers - @allowed_rack_request_headers ||= {}.tap do |result| - allowed_request_header_names.each do |header| - result["HTTP_#{header.to_s.upcase.gsub(/[-\s]/, '_')}"] = build_attribute_name('http.request.headers.', header) - end + @allowed_rack_request_headers ||= Array(config[:allowed_request_headers]).each_with_object({}) do |header, memo| + memo["HTTP_#{header.to_s.upcase.gsub(/[-\s]/, '_')}"] = build_attribute_name('http.request.headers.', header) end end - def allowed_request_header_names - @allowed_request_header_names ||= Array(config[:allowed_request_headers]) - end - def allowed_response_headers - @allowed_response_headers ||= {}.tap do |result| - allowed_response_header_names.each do |header| - result[header] = build_attribute_name('http.response.headers.', header) - result[header.to_s.upcase] = build_attribute_name('http.response.headers.', header) - end + @allowed_response_headers ||= Array(config[:allowed_response_headers]).each_with_object({}) do |header, memo| + memo[header] = build_attribute_name('http.response.headers.', header) + memo[header.to_s.upcase] = build_attribute_name('http.response.headers.', header) end end - def allowed_response_header_names - @allowed_response_header_names ||= Array(config[:allowed_response_headers]) - end - def build_attribute_name(prefix, suffix) prefix + suffix.to_s.downcase.gsub(/[-\s]/, '_') end From 7d5fc8b3a348dfaae3422cb2de5e8a007c2655ab Mon Sep 17 00:00:00 2001 From: David Davis Date: Tue, 11 Feb 2020 10:37:29 -0800 Subject: [PATCH 46/64] Optimize to return EMPTY_HASH if allowed_{response,rack_request}_headers.empty? --- .../adapters/rack/middlewares/tracer_middleware.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 913a0c1af..b9b1ee9ef 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -217,6 +217,8 @@ def set_attributes_after_request(span, status, headers, _response) # @return Hash def allowed_request_headers(env) + return EMPTY_HASH if self.class.allowed_rack_request_headers.empty? + {}.tap do |result| self.class.allowed_rack_request_headers.each do |key, value| result[value] = env[key] if env.key?(key) @@ -227,6 +229,7 @@ def allowed_request_headers(env) # @return Hash def allowed_response_headers(headers) return EMPTY_HASH if headers.nil? + return EMPTY_HASH if self.class.allowed_response_headers.empty? {}.tap do |result| self.class.allowed_response_headers.each do |key, value| From dcb150f0c5a15864d27388c7dd4694d483de7d31 Mon Sep 17 00:00:00 2001 From: David Davis Date: Tue, 11 Feb 2020 12:18:58 -0800 Subject: [PATCH 47/64] Adjust to context prop changes --- .../rack/middlewares/tracer_middleware.rb | 125 +++++++++--------- .../middlewares/tracer_middleware_test.rb | 4 +- 2 files changed, 66 insertions(+), 63 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index b9b1ee9ef..67399fbcf 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -60,73 +60,76 @@ def initialize(app) def call(env) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength original_env = env.dup - parent_context = OpenTelemetry.tracer_factory.http_text_format.extract(env) + extracted_context = OpenTelemetry.propagation.extract(env) + + # restore extracted context in this process: + OpenTelemetry::Context.with_current(extracted_context) do + frontend_span = create_frontend_span(env) + + # http://www.rubydoc.info/github/rack/rack/file/SPEC + # The source of truth in Rack is the PATH_INFO key that holds the + # URL for the current request; but some frameworks may override that + # value, especially during exception handling. + # + # Because of this, we prefer to use REQUEST_URI, if available, which is the + # relative path + query string, and doesn't mutate. + # + # REQUEST_URI is only available depending on what web server is running though. + # So when its not available, we want the original, unmutated PATH_INFO, which + # is just the relative path without query strings. + request_span_name = create_request_span_name(env['REQUEST_URI'] || original_env['PATH_INFO']) + + rack_request = ::Rack::Request.new(env) + # Sets as many attributes as are available before control + # is handed off to next middleware. + request_span = tracer.start_span(request_span_name, + # NOTE: try to set as many attributes via 'attributes' argument + # instead of via span.set_attribute + attributes: request_span_attributes(env: env, + full_http_request_url: full_http_request_url(rack_request), + full_path: full_path(rack_request), + base_url: base_url(rack_request)), + kind: :server) + + begin + @app.call(env).tap do |status, headers, response| + set_attributes_after_request(request_span, status, headers, response) + end + rescue StandardError => e + record_and_reraise_error(e, request_span: request_span) + ensure + request_span.finish + frontend_span&.finish + end + end + end - ### frontend span + private + + # return Context + def create_frontend_span(env) # NOTE: get_request_start may return nil request_start_time = OpenTelemetry::Adapters::Rack::Util::QueueTime.get_request_start(env) - frontend_span = tracer.start_span('http_server.queue', # NOTE: span kind of 'proxy' is not defined - with_parent_context: parent_context, - # NOTE: initialize with as many attributes as possible: - attributes: { - 'component' => 'http', - 'service' => config[:web_service_name], - # NOTE: dd-trace-rb differences: - # 'trace.span_type' => (http) 'proxy' - 'start_time' => request_start_time - }) - record_frontend_span = config[:record_frontend_span] && !request_start_time.nil? - frontend_span = if record_frontend_span - tracer.start_span('http_server.queue', # NOTE: span kind of 'proxy' is not defined - with_parent_context: parent_context, - # NOTE: initialize with as many attributes as possible: - attributes: { - 'component' => 'http', - 'service' => config[:web_service_name], - # NOTE: dd-trace-rb differences: - # 'trace.span_type' => (http) 'proxy' - 'start_time' => request_start_time - }) - end - - # http://www.rubydoc.info/github/rack/rack/file/SPEC - # The source of truth in Rack is the PATH_INFO key that holds the - # URL for the current request; but some frameworks may override that - # value, especially during exception handling. - # - # Because of this, we prefer to use REQUEST_URI, if available, which is the - # relative path + query string, and doesn't mutate. - # - # REQUEST_URI is only available depending on what web server is running though. - # So when its not available, we want the original, unmutated PATH_INFO, which - # is just the relative path without query strings. - request_span_name = create_request_span_name(env['REQUEST_URI'] || original_env['PATH_INFO']) - - rack_request = ::Rack::Request.new(env) - # Sets as many attributes as are available before control - # is handed off to next middleware. - request_span = tracer.start_span(request_span_name, - with_parent_context: parent_context, - # NOTE: try to set as many attributes via 'attributes' argument - # instead of via span.set_attribute - attributes: request_span_attributes(env: env, - full_http_request_url: full_http_request_url(rack_request), - full_path: full_path(rack_request), - base_url: base_url(rack_request)), - kind: :server) - - @app.call(env).tap do |status, headers, response| - set_attributes_after_request(request_span, status, headers, response) - end - rescue StandardError => e - record_and_reraise_error(e, request_span: request_span) - ensure - request_span.finish - frontend_span&.finish if record_frontend_span + + return unless record_frontend_span + + span = tracer.start_span('http_server.queue', # NOTE: span kind of 'proxy' is not defined + # NOTE: initialize with as many attributes as possible: + attributes: { + 'component' => 'http', + 'service' => config[:web_service_name], + # NOTE: dd-trace-rb differences: + # 'trace.span_type' => (http) 'proxy' + 'start_time' => request_start_time + }) + + span end - private + def current_span_key + OpenTelemetry::Trace::Propagation::ContextKeys.current_span_key + end def tracer OpenTelemetry::Adapters::Rack::Adapter.instance.tracer diff --git a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb index db86dc12e..e5848db8b 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb @@ -67,8 +67,8 @@ _(first_span.name).must_equal '/' end - it 'extracts parent context' do - _(first_span.parent_span_id).wont_equal '0000000000000000' + it 'has no parent' do + _(first_span.parent_span_id).must_equal '0000000000000000' end describe 'config[:allowed_request_headers]' do From 845aaa6bbb3e3f9664e51a2e786e09dd741cf611 Mon Sep 17 00:00:00 2001 From: David Davis Date: Tue, 11 Feb 2020 15:10:41 -0800 Subject: [PATCH 48/64] Remove unused variables --- .../adapters/rack/middlewares/tracer_middleware.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 67399fbcf..3de2056e6 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -44,10 +44,7 @@ def config def clear_cached_config @allowed_rack_request_headers = nil - @allowed_request_header_names = nil - @allowed_response_headers = nil - @allowed_response_header_names = nil end end From f565ad359a38c8b7ea9da29de59651cfb91babba Mon Sep 17 00:00:00 2001 From: David Davis Date: Tue, 11 Feb 2020 15:14:14 -0800 Subject: [PATCH 49/64] Use kind: :server for both frontend and request span --- .../adapters/rack/middlewares/tracer_middleware.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 3de2056e6..d194c6ac2 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -116,10 +116,9 @@ def create_frontend_span(env) attributes: { 'component' => 'http', 'service' => config[:web_service_name], - # NOTE: dd-trace-rb differences: - # 'trace.span_type' => (http) 'proxy' 'start_time' => request_start_time - }) + }, + kind: :server) span end From 7b0808568c2842bf5ffa410bdcff95a302449ed5 Mon Sep 17 00:00:00 2001 From: David Davis Date: Wed, 12 Feb 2020 12:22:15 -0800 Subject: [PATCH 50/64] Make request_span parented by frontend_span * explicitly manage frontend_span parent context, and prevent automatic span activation * manage frontend_span life-cycle explicitly via a new context, using it as the request_span's parent, if it's available --- .../rack/middlewares/tracer_middleware.rb | 49 +++++++++++-------- .../middlewares/tracer_middleware_test.rb | 13 ++++- 2 files changed, 40 insertions(+), 22 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index d194c6ac2..c8cb7163d 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -61,7 +61,7 @@ def call(env) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength # restore extracted context in this process: OpenTelemetry::Context.with_current(extracted_context) do - frontend_span = create_frontend_span(env) + frontend_context = create_frontend_span(env, extracted_context) # http://www.rubydoc.info/github/rack/rack/file/SPEC # The source of truth in Rack is the PATH_INFO key that holds the @@ -79,39 +79,42 @@ def call(env) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength rack_request = ::Rack::Request.new(env) # Sets as many attributes as are available before control # is handed off to next middleware. - request_span = tracer.start_span(request_span_name, - # NOTE: try to set as many attributes via 'attributes' argument - # instead of via span.set_attribute - attributes: request_span_attributes(env: env, - full_http_request_url: full_http_request_url(rack_request), - full_path: full_path(rack_request), - base_url: base_url(rack_request)), - kind: :server) - - begin - @app.call(env).tap do |status, headers, response| - set_attributes_after_request(request_span, status, headers, response) + tracer.in_span(request_span_name, + with_parent_context: frontend_context || extracted_context, + # NOTE: try to set as many attributes via 'attributes' argument + # instead of via span.set_attribute + attributes: request_span_attributes(env: env, + full_http_request_url: full_http_request_url(rack_request), + full_path: full_path(rack_request), + base_url: base_url(rack_request)), + kind: :server) do |request_span| + begin + @app.call(env).tap do |status, headers, response| + set_attributes_after_request(request_span, status, headers, response) + end + rescue StandardError => e + record_and_reraise_error(e, request_span: request_span) + ensure + finish_frontend_span(frontend_context) end - rescue StandardError => e - record_and_reraise_error(e, request_span: request_span) - ensure - request_span.finish - frontend_span&.finish end end end private - # return Context - def create_frontend_span(env) + # return Context with the frontend span as the current span + def create_frontend_span(env, extracted_context) # NOTE: get_request_start may return nil request_start_time = OpenTelemetry::Adapters::Rack::Util::QueueTime.get_request_start(env) record_frontend_span = config[:record_frontend_span] && !request_start_time.nil? return unless record_frontend_span + # NOTE: start_span assumes context is managed explicitly, + # while in_span and with_span activate span automatically span = tracer.start_span('http_server.queue', # NOTE: span kind of 'proxy' is not defined + with_parent_context: extracted_context, # NOTE: initialize with as many attributes as possible: attributes: { 'component' => 'http', @@ -120,7 +123,11 @@ def create_frontend_span(env) }, kind: :server) - span + extracted_context.set_value(current_span_key, span) + end + + def finish_frontend_span(context) + context[current_span_key]&.finish if context end def current_span_key diff --git a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb index e5848db8b..ea1293ad9 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb @@ -114,16 +114,23 @@ end describe 'config[:record_frontend_span]' do + let(:request_span) { exporter.finished_spans.first } + describe 'default' do it 'does not record span' do _(exporter.finished_spans.size).must_equal 1 end + + it 'does not parent the request_span' do + _(request_span.parent_span_id).must_equal '0000000000000000' + end end describe 'when recordable' do let(:config) { default_config.merge(record_frontend_span: true) } let(:env) { Hash('HTTP_X_REQUEST_START' => Time.now.to_i) } - let(:frontend_span) { exporter.finished_spans.last } + let(:frontend_span) { exporter.finished_spans.first } + let(:request_span) { exporter.finished_spans[1] } it 'records span' do _(exporter.finished_spans.size).must_equal 2 @@ -131,6 +138,10 @@ _(frontend_span.attributes['service']).must_be_nil end + it 'frontend_span parents request_span' do + _(request_span.parent_span_id).must_equal frontend_span.span_id + end + describe 'when config[:web_service_name]' do let(:config) do default_config.merge(record_frontend_span: true, From 8e0e1b46cc6cc9c4f1287e6ec9b069e4f2926da1 Mon Sep 17 00:00:00 2001 From: David Davis Date: Wed, 12 Feb 2020 15:59:12 -0800 Subject: [PATCH 51/64] Implement using helpers to that in_span doesn't have to record and re-raise error --- .../rack/middlewares/tracer_middleware.rb | 85 ++++++++++--------- 1 file changed, 46 insertions(+), 39 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index c8cb7163d..7c41c7f74 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -55,54 +55,61 @@ def initialize(app) end def call(env) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength - original_env = env.dup - extracted_context = OpenTelemetry.propagation.extract(env) + frontend_context = create_frontend_span(env, extracted_context) + request_context = create_request_span(env, frontend_context || extracted_context) + request_span = request_context[current_span_key] # restore extracted context in this process: - OpenTelemetry::Context.with_current(extracted_context) do - frontend_context = create_frontend_span(env, extracted_context) - - # http://www.rubydoc.info/github/rack/rack/file/SPEC - # The source of truth in Rack is the PATH_INFO key that holds the - # URL for the current request; but some frameworks may override that - # value, especially during exception handling. - # - # Because of this, we prefer to use REQUEST_URI, if available, which is the - # relative path + query string, and doesn't mutate. - # - # REQUEST_URI is only available depending on what web server is running though. - # So when its not available, we want the original, unmutated PATH_INFO, which - # is just the relative path without query strings. - request_span_name = create_request_span_name(env['REQUEST_URI'] || original_env['PATH_INFO']) - - rack_request = ::Rack::Request.new(env) - # Sets as many attributes as are available before control - # is handed off to next middleware. - tracer.in_span(request_span_name, - with_parent_context: frontend_context || extracted_context, - # NOTE: try to set as many attributes via 'attributes' argument - # instead of via span.set_attribute - attributes: request_span_attributes(env: env, - full_http_request_url: full_http_request_url(rack_request), - full_path: full_path(rack_request), - base_url: base_url(rack_request)), - kind: :server) do |request_span| - begin - @app.call(env).tap do |status, headers, response| - set_attributes_after_request(request_span, status, headers, response) - end - rescue StandardError => e - record_and_reraise_error(e, request_span: request_span) - ensure - finish_frontend_span(frontend_context) + OpenTelemetry::Context.with_current(request_context) do + begin + @app.call(env).tap do |status, headers, response| + set_attributes_after_request(request_span, status, headers, response) end + rescue StandardError => e + record_and_reraise_error(e, request_span: request_span) + ensure + finish_span(frontend_context) + finish_span(request_context) end end end private + def create_request_span(env, context) + original_env = env.dup + + # http://www.rubydoc.info/github/rack/rack/file/SPEC + # The source of truth in Rack is the PATH_INFO key that holds the + # URL for the current request; but some frameworks may override that + # value, especially during exception handling. + # + # Because of this, we prefer to use REQUEST_URI, if available, which is the + # relative path + query string, and doesn't mutate. + # + # REQUEST_URI is only available depending on what web server is running though. + # So when its not available, we want the original, unmutated PATH_INFO, which + # is just the relative path without query strings. + request_span_name = create_request_span_name(env['REQUEST_URI'] || original_env['PATH_INFO']) + + rack_request = ::Rack::Request.new(env) + + # Sets as many attributes as are available before control + # is handed off to next middleware. + span = tracer.start_span(request_span_name, + with_parent_context: context, + # NOTE: try to set as many attributes via 'attributes' argument + # instead of via span.set_attribute + attributes: request_span_attributes(env: env, + full_http_request_url: full_http_request_url(rack_request), + full_path: full_path(rack_request), + base_url: base_url(rack_request)), + kind: :server) + + context.set_value(current_span_key, span) + end + # return Context with the frontend span as the current span def create_frontend_span(env, extracted_context) # NOTE: get_request_start may return nil @@ -126,7 +133,7 @@ def create_frontend_span(env, extracted_context) extracted_context.set_value(current_span_key, span) end - def finish_frontend_span(context) + def finish_span(context) context[current_span_key]&.finish if context end From dabdf02b10bcda0dc91a77f08d2c36be4834fc8c Mon Sep 17 00:00:00 2001 From: David Davis Date: Wed, 12 Feb 2020 16:16:55 -0800 Subject: [PATCH 52/64] Cleanup some URL wrapper methods * goal: eliminate need for Rack::Request allocation --- .../rack/middlewares/tracer_middleware.rb | 38 +++++-------------- 1 file changed, 9 insertions(+), 29 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 7c41c7f74..ee7ff9b9b 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -54,7 +54,7 @@ def initialize(app) @app = app end - def call(env) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength + def call(env) extracted_context = OpenTelemetry.propagation.extract(env) frontend_context = create_frontend_span(env, extracted_context) request_context = create_request_span(env, frontend_context || extracted_context) @@ -93,18 +93,13 @@ def create_request_span(env, context) # is just the relative path without query strings. request_span_name = create_request_span_name(env['REQUEST_URI'] || original_env['PATH_INFO']) - rack_request = ::Rack::Request.new(env) - # Sets as many attributes as are available before control # is handed off to next middleware. span = tracer.start_span(request_span_name, with_parent_context: context, # NOTE: try to set as many attributes via 'attributes' argument # instead of via span.set_attribute - attributes: request_span_attributes(env: env, - full_http_request_url: full_http_request_url(rack_request), - full_path: full_path(rack_request), - base_url: base_url(rack_request)), + attributes: request_span_attributes(env: env), kind: :server) context.set_value(current_span_key, span) @@ -154,22 +149,21 @@ def tracer # * http.scheme, http.server_name, net.host.port, http.target # * http.scheme, net.host.name, net.host.port, http.target # * http.url - def request_span_attributes(env:, full_http_request_url:, full_path:, base_url:) + def request_span_attributes(env:) + rack_request = ::Rack::Request.new(env) + { 'component' => 'http', 'http.method' => env['REQUEST_METHOD'], - 'http.url' => full_http_request_url, + 'http.url' => rack_request.url, 'http.host' => env['HOST'], 'http.scheme' => env['rack.url_scheme'], - 'http.target' => full_path, - 'http.base_url' => base_url # NOTE: 'http.base_url' isn't officially defined + # e.g., "/webshop/articles/4?s=1": + 'http.target' => rack_request.fullpath, + 'http.base_url' => rack_request.base_url # NOTE: 'http.base_url' isn't officially defined }.merge(allowed_request_headers(env)) end - def full_http_request_url(rack_request) - rack_request.url - end - # https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-http.md#name # # recommendation: span.name(s) should be low-cardinality (e.g., @@ -187,20 +181,6 @@ def create_request_span_name(request_uri_or_path_info) end end - def base_url(rack_request) - if rack_request.respond_to?(:base_url) - rack_request.base_url - else - # Compatibility for older Rack versions - rack_request.url.chomp(rack_request.fullpath) - end - end - - # e.g., "/webshop/articles/4?s=1" - def full_path(rack_request) - rack_request.fullpath - end - # catch exceptions that may be raised in the middleware chain # Note: if a middleware catches an Exception without re raising, # the Exception cannot be recorded here. From 1317d355a7763dbb4fd3115e2c985ec13d08bb8c Mon Sep 17 00:00:00 2001 From: David Davis Date: Wed, 12 Feb 2020 16:32:02 -0800 Subject: [PATCH 53/64] Optimize: return without assigning local variable --- .../adapters/rack/middlewares/tracer_middleware.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index ee7ff9b9b..5ed71732c 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -109,9 +109,8 @@ def create_request_span(env, context) def create_frontend_span(env, extracted_context) # NOTE: get_request_start may return nil request_start_time = OpenTelemetry::Adapters::Rack::Util::QueueTime.get_request_start(env) - record_frontend_span = config[:record_frontend_span] && !request_start_time.nil? - return unless record_frontend_span + return unless config[:record_frontend_span] && !request_start_time.nil? # NOTE: start_span assumes context is managed explicitly, # while in_span and with_span activate span automatically From caeb3bddcc6845869aaffe973fa014bcfbab9837 Mon Sep 17 00:00:00 2001 From: David Davis Date: Wed, 12 Feb 2020 16:34:53 -0800 Subject: [PATCH 54/64] Just use http.{scheme,host,target} (remove url, base_url) --- .../adapters/rack/middlewares/tracer_middleware.rb | 4 +--- .../adapters/rack/middlewares/tracer_middleware_test.rb | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 5ed71732c..ad2c59026 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -154,12 +154,10 @@ def request_span_attributes(env:) { 'component' => 'http', 'http.method' => env['REQUEST_METHOD'], - 'http.url' => rack_request.url, 'http.host' => env['HOST'], 'http.scheme' => env['rack.url_scheme'], # e.g., "/webshop/articles/4?s=1": - 'http.target' => rack_request.fullpath, - 'http.base_url' => rack_request.base_url # NOTE: 'http.base_url' isn't officially defined + 'http.target' => rack_request.fullpath }.merge(allowed_request_headers(env)) end diff --git a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb index ea1293ad9..3e5086659 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb @@ -63,7 +63,7 @@ _(first_span.attributes['http.status_text']).must_equal 'OK' _(first_span.attributes['http.target']).must_equal '/' _(first_span.status.canonical_code).must_equal OpenTelemetry::Trace::Status::OK - _(first_span.attributes['http.url']).must_equal 'http://example.org/' + _(first_span.attributes['http.url']).must_be_nil _(first_span.name).must_equal '/' end From 3659ead854e904cfe4b61f62cb44e46b4dacaf60 Mon Sep 17 00:00:00 2001 From: David Davis Date: Wed, 12 Feb 2020 16:39:02 -0800 Subject: [PATCH 55/64] Inline Rack::Request#fullpath --- .../adapters/rack/middlewares/tracer_middleware.rb | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index ad2c59026..c5d04d301 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -149,18 +149,23 @@ def tracer # * http.scheme, net.host.name, net.host.port, http.target # * http.url def request_span_attributes(env:) - rack_request = ::Rack::Request.new(env) - { 'component' => 'http', 'http.method' => env['REQUEST_METHOD'], 'http.host' => env['HOST'], 'http.scheme' => env['rack.url_scheme'], - # e.g., "/webshop/articles/4?s=1": - 'http.target' => rack_request.fullpath + 'http.target' => fullpath(env) }.merge(allowed_request_headers(env)) end + # e.g., "/webshop/articles/4?s=1": + def fullpath(env) + query_string = env['QUERY_STRING'] + path = env['SCRIPT_NAME'] + env['PATH_INFO'] + + query_string.empty? ? path : "#{path}?#{query_string}" + end + # https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-http.md#name # # recommendation: span.name(s) should be low-cardinality (e.g., From 2c6839288f49de80927ae767c5d76a3b2a958244 Mon Sep 17 00:00:00 2001 From: David Davis Date: Tue, 18 Feb 2020 09:31:58 -0800 Subject: [PATCH 56/64] Fix .circleci/config.yml after conflict --- .circleci/config.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.circleci/config.yml b/.circleci/config.yml index 0a9cf98ef..28e420239 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -68,6 +68,7 @@ commands: bundle install --jobs=3 --retry=3 bundle exec appraisal install bundle exec appraisal rake test + - run: name: Bundle + CI (Adapters - REST Client) command: | cd adapters/restclient From fb3a242625f6a2228f9d20b59cc2b6fbd6532566 Mon Sep 17 00:00:00 2001 From: David Davis Date: Wed, 19 Feb 2020 15:41:39 -0800 Subject: [PATCH 57/64] Adjust error handling according to #184 --- .../adapters/rack/middlewares/tracer_middleware.rb | 10 +++++----- .../rack/middlewares/tracer_middleware_test.rb | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index c5d04d301..bab1b6d16 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -4,6 +4,8 @@ # # SPDX-License-Identifier: Apache-2.0 +require 'opentelemetry/trace/status' + require_relative '../util/queue_time' module OpenTelemetry @@ -187,14 +189,12 @@ def create_request_span_name(request_uri_or_path_info) # Note: if a middleware catches an Exception without re raising, # the Exception cannot be recorded here. def record_and_reraise_error(error, request_span:) + request_span.record_error(error) request_span.status = OpenTelemetry::Trace::Status.new( - OpenTelemetry::Trace::Status::INTERNAL_ERROR, - description: error.to_s + OpenTelemetry::Trace::Status::UNKNOWN_ERROR, + description: "Unhandled exception of type: #{error.class}" ) - # TODO: implement span.set_error? (this is a specification-level issue): - # request_span.set_error(error) unless request_span.nil? - # raise error end diff --git a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb index 3e5086659..61668cbce 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb @@ -191,7 +191,7 @@ assert_raises SimulatedError do Rack::MockRequest.new(rack_builder).get('/', env) end - _(first_span.status.canonical_code).must_equal OpenTelemetry::Trace::Status::INTERNAL_ERROR + _(first_span.status.canonical_code).must_equal OpenTelemetry::Trace::Status::UNKNOWN_ERROR end end end From e763e21c3bd7b895259be9fefaff0e41710346df Mon Sep 17 00:00:00 2001 From: David Davis Date: Fri, 21 Feb 2020 16:34:33 -0800 Subject: [PATCH 58/64] Rewrite to utilize in_span * note that order of finished spans is now swapped --- .../rack/middlewares/tracer_middleware.rb | 58 +++---------------- .../middlewares/tracer_middleware_test.rb | 4 +- 2 files changed, 10 insertions(+), 52 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index bab1b6d16..b3ba202d9 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -57,56 +57,27 @@ def initialize(app) end def call(env) + original_env = env.dup extracted_context = OpenTelemetry.propagation.extract(env) frontend_context = create_frontend_span(env, extracted_context) - request_context = create_request_span(env, frontend_context || extracted_context) - request_span = request_context[current_span_key] # restore extracted context in this process: - OpenTelemetry::Context.with_current(request_context) do - begin + OpenTelemetry::Context.with_current(frontend_context || extracted_context) do + request_span_name = create_request_span_name(env['REQUEST_URI'] || original_env['PATH_INFO']) + tracer.in_span(request_span_name, + attributes: request_span_attributes(env: env), + kind: :server) do |request_span| @app.call(env).tap do |status, headers, response| set_attributes_after_request(request_span, status, headers, response) end - rescue StandardError => e - record_and_reraise_error(e, request_span: request_span) - ensure - finish_span(frontend_context) - finish_span(request_context) end end + ensure + finish_span(frontend_context) end private - def create_request_span(env, context) - original_env = env.dup - - # http://www.rubydoc.info/github/rack/rack/file/SPEC - # The source of truth in Rack is the PATH_INFO key that holds the - # URL for the current request; but some frameworks may override that - # value, especially during exception handling. - # - # Because of this, we prefer to use REQUEST_URI, if available, which is the - # relative path + query string, and doesn't mutate. - # - # REQUEST_URI is only available depending on what web server is running though. - # So when its not available, we want the original, unmutated PATH_INFO, which - # is just the relative path without query strings. - request_span_name = create_request_span_name(env['REQUEST_URI'] || original_env['PATH_INFO']) - - # Sets as many attributes as are available before control - # is handed off to next middleware. - span = tracer.start_span(request_span_name, - with_parent_context: context, - # NOTE: try to set as many attributes via 'attributes' argument - # instead of via span.set_attribute - attributes: request_span_attributes(env: env), - kind: :server) - - context.set_value(current_span_key, span) - end - # return Context with the frontend span as the current span def create_frontend_span(env, extracted_context) # NOTE: get_request_start may return nil @@ -185,19 +156,6 @@ def create_request_span_name(request_uri_or_path_info) end end - # catch exceptions that may be raised in the middleware chain - # Note: if a middleware catches an Exception without re raising, - # the Exception cannot be recorded here. - def record_and_reraise_error(error, request_span:) - request_span.record_error(error) - request_span.status = OpenTelemetry::Trace::Status.new( - OpenTelemetry::Trace::Status::UNKNOWN_ERROR, - description: "Unhandled exception of type: #{error.class}" - ) - - raise error - end - def set_attributes_after_request(span, status, headers, _response) span.status = OpenTelemetry::Trace::Status.http_to_status(status) span.set_attribute('http.status_code', status) diff --git a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb index 61668cbce..f24e7c561 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb @@ -129,8 +129,8 @@ describe 'when recordable' do let(:config) { default_config.merge(record_frontend_span: true) } let(:env) { Hash('HTTP_X_REQUEST_START' => Time.now.to_i) } - let(:frontend_span) { exporter.finished_spans.first } - let(:request_span) { exporter.finished_spans[1] } + let(:frontend_span) { exporter.finished_spans[1] } + let(:request_span) { exporter.finished_spans[0] } it 'records span' do _(exporter.finished_spans.size).must_equal 2 From e5d15c63b4d79c6396849120ff53d5bebc7009d3 Mon Sep 17 00:00:00 2001 From: David Davis Date: Fri, 21 Feb 2020 16:40:58 -0800 Subject: [PATCH 59/64] Reduce comments that were more useful in development/review --- .../rack/middlewares/tracer_middleware.rb | 20 +------------------ 1 file changed, 1 insertion(+), 19 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index b3ba202d9..844bd398e 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -14,11 +14,6 @@ module Rack module Middlewares # TracerMiddleware propagates context and instruments Rack requests # by way of its middleware system - # - # Notable implementation differences from dd-trace-rb: - # * missing: 'span.resource', which is set to span.name - # * missing: config[:distributed_tracing] - # * missing: span.set_error() -- spec level change class TracerMiddleware # rubocop:disable Metrics/ClassLength class << self def allowed_rack_request_headers @@ -80,16 +75,12 @@ def call(env) # return Context with the frontend span as the current span def create_frontend_span(env, extracted_context) - # NOTE: get_request_start may return nil request_start_time = OpenTelemetry::Adapters::Rack::Util::QueueTime.get_request_start(env) return unless config[:record_frontend_span] && !request_start_time.nil? - # NOTE: start_span assumes context is managed explicitly, - # while in_span and with_span activate span automatically - span = tracer.start_span('http_server.queue', # NOTE: span kind of 'proxy' is not defined + span = tracer.start_span('http_server.queue', with_parent_context: extracted_context, - # NOTE: initialize with as many attributes as possible: attributes: { 'component' => 'http', 'service' => config[:web_service_name], @@ -114,13 +105,6 @@ def tracer ### request_span - # Per https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-http.md#http-server-semantic-conventions - # - # One of the following sets is required, in descending preference: - # * http.scheme, http.host, http.target - # * http.scheme, http.server_name, net.host.port, http.target - # * http.scheme, net.host.name, net.host.port, http.target - # * http.url def request_span_attributes(env:) { 'component' => 'http', @@ -168,7 +152,6 @@ def set_attributes_after_request(span, status, headers, _response) allowed_response_headers(headers).each { |k, v| span.set_attribute(k, v) } end - # @return Hash def allowed_request_headers(env) return EMPTY_HASH if self.class.allowed_rack_request_headers.empty? @@ -179,7 +162,6 @@ def allowed_request_headers(env) end end - # @return Hash def allowed_response_headers(headers) return EMPTY_HASH if headers.nil? return EMPTY_HASH if self.class.allowed_response_headers.empty? From 12b6b581305bb381d7e6950ba7dbe9f5d39b0788 Mon Sep 17 00:00:00 2001 From: David Davis Date: Tue, 25 Feb 2020 09:34:24 -0800 Subject: [PATCH 60/64] Update http.host to use HTTP_HOST or 'unknown' Co-Authored-By: Matthew Wear --- .../adapters/rack/middlewares/tracer_middleware.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 844bd398e..7ffad6559 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -109,7 +109,7 @@ def request_span_attributes(env:) { 'component' => 'http', 'http.method' => env['REQUEST_METHOD'], - 'http.host' => env['HOST'], + 'http.host' => env['HTTP_HOST'] || 'unknown', 'http.scheme' => env['rack.url_scheme'], 'http.target' => fullpath(env) }.merge(allowed_request_headers(env)) From bb3b9c279482ce7771fe64c8bb47e8eef0566926 Mon Sep 17 00:00:00 2001 From: David Davis Date: Tue, 25 Feb 2020 09:35:10 -0800 Subject: [PATCH 61/64] Update request_start_time to be number, not timestamp Co-Authored-By: Matthew Wear --- .../adapters/rack/middlewares/tracer_middleware.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 7ffad6559..2180e489e 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -84,7 +84,7 @@ def create_frontend_span(env, extracted_context) attributes: { 'component' => 'http', 'service' => config[:web_service_name], - 'start_time' => request_start_time + 'start_time' => request_start_time.to_f }, kind: :server) From bcf940fdcd649ae1d430fa1a8bb43ccebc111a1a Mon Sep 17 00:00:00 2001 From: David Davis Date: Tue, 25 Feb 2020 09:37:32 -0800 Subject: [PATCH 62/64] Remove request_span comment --- .../adapters/rack/middlewares/tracer_middleware.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 2180e489e..e6545e762 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -103,8 +103,6 @@ def tracer OpenTelemetry::Adapters::Rack::Adapter.instance.tracer end - ### request_span - def request_span_attributes(env:) { 'component' => 'http', From cc14c6494f74fbb5a0ac9baf678841bab0671557 Mon Sep 17 00:00:00 2001 From: David Davis Date: Tue, 25 Feb 2020 09:43:13 -0800 Subject: [PATCH 63/64] Remove 'service' attribute when creating frontend span * value is potentially nil, which is not a valid attribute value * also 'service' is not an official semantic convention and will probably come from the application resource in the future --- .../adapters/rack/middlewares/tracer_middleware.rb | 1 - .../rack/middlewares/tracer_middleware_test.rb | 11 ----------- 2 files changed, 12 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index e6545e762..078b3e828 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -83,7 +83,6 @@ def create_frontend_span(env, extracted_context) with_parent_context: extracted_context, attributes: { 'component' => 'http', - 'service' => config[:web_service_name], 'start_time' => request_start_time.to_f }, kind: :server) diff --git a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb index f24e7c561..f769eaaab 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb @@ -141,17 +141,6 @@ it 'frontend_span parents request_span' do _(request_span.parent_span_id).must_equal frontend_span.span_id end - - describe 'when config[:web_service_name]' do - let(:config) do - default_config.merge(record_frontend_span: true, - web_service_name: 'my_service_name') - end - - it 'records "service" attribute' do - _(frontend_span.attributes['service']).must_equal 'my_service_name' - end - end end end end From 80b2c2d9c6e2b4841d3b31d314c7ef32c5408f40 Mon Sep 17 00:00:00 2001 From: David Davis Date: Fri, 28 Feb 2020 10:25:27 -0800 Subject: [PATCH 64/64] Change frontend_span to 'http_server.proxy', make request_span :internal * request_span.kind is :internal if frontend_span is present * future: change request_span :kind to ':proxy' if/when it gets added to spec --- .../adapters/rack/middlewares/tracer_middleware.rb | 5 +++-- .../adapters/rack/middlewares/tracer_middleware_test.rb | 7 ++++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb index 078b3e828..b3a4f71fe 100644 --- a/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb +++ b/adapters/rack/lib/opentelemetry/adapters/rack/middlewares/tracer_middleware.rb @@ -59,9 +59,10 @@ def call(env) # restore extracted context in this process: OpenTelemetry::Context.with_current(frontend_context || extracted_context) do request_span_name = create_request_span_name(env['REQUEST_URI'] || original_env['PATH_INFO']) + request_span_kind = frontend_context.nil? ? :server : :internal tracer.in_span(request_span_name, attributes: request_span_attributes(env: env), - kind: :server) do |request_span| + kind: request_span_kind) do |request_span| @app.call(env).tap do |status, headers, response| set_attributes_after_request(request_span, status, headers, response) end @@ -79,7 +80,7 @@ def create_frontend_span(env, extracted_context) return unless config[:record_frontend_span] && !request_start_time.nil? - span = tracer.start_span('http_server.queue', + span = tracer.start_span('http_server.proxy', with_parent_context: extracted_context, attributes: { 'component' => 'http', diff --git a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb index f769eaaab..f1af14919 100644 --- a/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb +++ b/adapters/rack/test/opentelemetry/adapters/rack/middlewares/tracer_middleware_test.rb @@ -65,6 +65,7 @@ _(first_span.status.canonical_code).must_equal OpenTelemetry::Trace::Status::OK _(first_span.attributes['http.url']).must_be_nil _(first_span.name).must_equal '/' + _(first_span.kind).must_equal :server end it 'has no parent' do @@ -134,10 +135,14 @@ it 'records span' do _(exporter.finished_spans.size).must_equal 2 - _(frontend_span.name).must_equal 'http_server.queue' + _(frontend_span.name).must_equal 'http_server.proxy' _(frontend_span.attributes['service']).must_be_nil end + it 'changes request_span kind' do + _(request_span.kind).must_equal :internal + end + it 'frontend_span parents request_span' do _(request_span.parent_span_id).must_equal frontend_span.span_id end