From 29d371cc24900e3e458d20dac8eba5f8668faa6f Mon Sep 17 00:00:00 2001 From: David Elner Date: Thu, 29 Apr 2021 15:06:30 -0400 Subject: [PATCH 1/2] Changed: Rubocop RSpec hook rule. --- .rubocop.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.rubocop.yml b/.rubocop.yml index ce7d842ce4..931770f0ab 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -239,6 +239,10 @@ RSpec/LeadingSubject: RSpec/ImplicitSubject: Enabled: false +# Enforces empty line after hook declaration. +RSpec/EmptyLineAfterHook: + Enabled: false + # Enforces empty line after subject declaration. RSpec/EmptyLineAfterSubject: Enabled: false From 8a35b6e733cf951a3d3fb487428d4ae4d33db5f3 Mon Sep 17 00:00:00 2001 From: David Elner Date: Thu, 29 Apr 2021 15:07:10 -0400 Subject: [PATCH 2/2] Fixed: ActionPack instrumentation #starts_with? error when running standalone. --- .../action_controller/instrumentation.rb | 2 +- lib/ddtrace/contrib/action_pack/utils.rb | 2 +- .../action_controller/instrumentation_spec.rb | 78 +++++++++++++++++++ 3 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 spec/ddtrace/contrib/action_pack/action_controller/instrumentation_spec.rb diff --git a/lib/ddtrace/contrib/action_pack/action_controller/instrumentation.rb b/lib/ddtrace/contrib/action_pack/action_controller/instrumentation.rb index 06bd64159d..f1fce69c2e 100644 --- a/lib/ddtrace/contrib/action_pack/action_controller/instrumentation.rb +++ b/lib/ddtrace/contrib/action_pack/action_controller/instrumentation.rb @@ -61,7 +61,7 @@ def finish_processing(payload) # [christian] in some cases :status is not defined, # rather than firing an error, simply acknowledge we don't know it. status = payload.fetch(:status, '?').to_s - span.status = 1 if status.starts_with?('5') + span.status = 1 if status.start_with?('5') elsif Utils.exception_is_error?(exception) span.set_error(exception) end diff --git a/lib/ddtrace/contrib/action_pack/utils.rb b/lib/ddtrace/contrib/action_pack/utils.rb index 14b8a5626e..4e56a8fe53 100644 --- a/lib/ddtrace/contrib/action_pack/utils.rb +++ b/lib/ddtrace/contrib/action_pack/utils.rb @@ -11,7 +11,7 @@ def self.exception_is_error?(exception) # You can add custom errors via `config.action_dispatch.rescue_responses` status = ::ActionDispatch::ExceptionWrapper.status_code_for_exception(exception.class.name) # Only 5XX exceptions are actually errors (e.g. don't flag 404s) - status.to_s.starts_with?('5') + status.to_s.start_with?('5') else true end diff --git a/spec/ddtrace/contrib/action_pack/action_controller/instrumentation_spec.rb b/spec/ddtrace/contrib/action_pack/action_controller/instrumentation_spec.rb new file mode 100644 index 0000000000..a0ad23475f --- /dev/null +++ b/spec/ddtrace/contrib/action_pack/action_controller/instrumentation_spec.rb @@ -0,0 +1,78 @@ +require 'ddtrace/contrib/support/spec_helper' + +require 'action_controller' +require 'ddtrace' + +# TODO: We plan on rewriting much of this instrumentation to bring it up to +# present day patterns/conventions. For now, just test a few known cases. +RSpec.describe Datadog::Contrib::ActionPack::ActionController::Instrumentation do + describe '::finish_processing' do + subject(:finish_processing) { described_class.finish_processing(payload) } + + context 'given a payload that been started' do + before { described_class.start_processing(payload) } + after { span.finish } + + let(:action_dispatch_exception) { nil } + let(:action_name) { 'index' } + let(:controller_class) { stub_const('TestController', Class.new(ActionController::Base)) } + let(:env) { { 'rack.url_scheme' => 'http' } } + let(:payload) do + { + controller: controller_class, + action: action_name, + env: env, + headers: { + # The exception this controller was given in the request, + # which is typical if the controller is configured to handle exceptions. + request_exception: action_dispatch_exception + }, + tracing_context: {} + } + end + + let(:span) { payload[:tracing_context][:dd_request_span] } + + context 'with a 200 OK response' do + before do + expect(Datadog.logger).to_not receive(:error) + finish_processing + end + + describe 'the Datadog span' do + it do + expect(span).to_not have_error + end + end + end + + context 'with a 500 Server Error response' do + let(:error) do + begin + raise 'Test error' + rescue StandardError => e + e + end + end + + let(:payload) do + super().merge( + exception: [error.class.name, error.message], + exception_object: error + ) + end + + before do + expect(Datadog.logger).to_not receive(:error) + finish_processing + end + + describe 'the Datadog span' do + it do + expect(span).to have_error + end + end + end + end + end +end