From a3ff8ed7197777b70fe1f7d011563721f2c0f82b Mon Sep 17 00:00:00 2001 From: Ariel Valentin Date: Fri, 6 Oct 2023 23:03:27 -0500 Subject: [PATCH] fix!: Drop DelayedJob ActiveRecord in Tests The DelayedJob tests suite relied on the ActiveRecord Backend gem for testing, however incompatabilities with ActiveRecord/Rails 7.1 means would result in failures in the test suite. It is not the responsibility of OTel Ruby Instrumentation to ensure compatability of 3rd party gems or ensure our test suite to come up with workarounds for bugs. For this reason I have rewritten the test suite to use the Test backend that Delayed Job used to test the upstream gem. This does introduce one change, the `created_at` attribute, which is _specific_ to the ActiveRecord backend will _no longer_ be emitted. Users that need this event should pin to earlier versions of the gem. There is a risk here that the upstream DelayedJob gem will no longer be included in future releases, but I think that is less likely than to run into incompatabilities with future Rails versions. --- instrumentation/delayed_job/Appraisals | 12 +++++- .../delayed_job/plugins/tracer_plugin.rb | 1 - ...emetry-instrumentation-delayed_job.gemspec | 3 +- .../delayed_job/plugins/tracer_plugin_test.rb | 29 +++++--------- .../instrumentation/delayed_job_test.rb | 9 +---- .../delayed_job/test/test_helper.rb | 40 +++++-------------- 6 files changed, 33 insertions(+), 61 deletions(-) diff --git a/instrumentation/delayed_job/Appraisals b/instrumentation/delayed_job/Appraisals index c3e9b24a2..61ac8e82d 100644 --- a/instrumentation/delayed_job/Appraisals +++ b/instrumentation/delayed_job/Appraisals @@ -4,6 +4,14 @@ # # SPDX-License-Identifier: Apache-2.0 -appraise 'delayed_job-4.1' do - gem 'delayed_job', '~> 4.1.0' +appraise 'delayed_job_4.1-rails-7.1' do + gem 'activejob', '~> 7.1.0' +end + +appraise 'delayed_job_4.1-rails-7.0' do + gem 'activejob', '~> 7.0.0' +end + +appraise 'delayed_job-4.1-rails-6.1' do + gem 'activejob', '~> 6.1.0' end diff --git a/instrumentation/delayed_job/lib/opentelemetry/instrumentation/delayed_job/plugins/tracer_plugin.rb b/instrumentation/delayed_job/lib/opentelemetry/instrumentation/delayed_job/plugins/tracer_plugin.rb index fccba0641..304868adb 100644 --- a/instrumentation/delayed_job/lib/opentelemetry/instrumentation/delayed_job/plugins/tracer_plugin.rb +++ b/instrumentation/delayed_job/lib/opentelemetry/instrumentation/delayed_job/plugins/tracer_plugin.rb @@ -56,7 +56,6 @@ def build_attributes(job) end def add_events(span, job) - span.add_event('created_at', timestamp: job.created_at) span.add_event('run_at', timestamp: job.run_at) if job.run_at span.add_event('locked_at', timestamp: job.locked_at) if job.locked_at end diff --git a/instrumentation/delayed_job/opentelemetry-instrumentation-delayed_job.gemspec b/instrumentation/delayed_job/opentelemetry-instrumentation-delayed_job.gemspec index 86648b55b..9978617c2 100644 --- a/instrumentation/delayed_job/opentelemetry-instrumentation-delayed_job.gemspec +++ b/instrumentation/delayed_job/opentelemetry-instrumentation-delayed_job.gemspec @@ -30,8 +30,7 @@ Gem::Specification.new do |spec| spec.add_development_dependency 'appraisal', '~> 2.5' spec.add_development_dependency 'bundler', '~> 2.4' - spec.add_development_dependency 'delayed_job', '~> 4.1.0' - spec.add_development_dependency 'delayed_job_active_record' + spec.add_development_dependency 'delayed_job', '~> 4.1.7' spec.add_development_dependency 'minitest', '~> 5.0' spec.add_development_dependency 'opentelemetry-sdk', '~> 1.1' spec.add_development_dependency 'opentelemetry-test-helpers', '~> 0.3' diff --git a/instrumentation/delayed_job/test/opentelemetry/instrumentation/delayed_job/plugins/tracer_plugin_test.rb b/instrumentation/delayed_job/test/opentelemetry/instrumentation/delayed_job/plugins/tracer_plugin_test.rb index 2f358cefe..85f8f7464 100644 --- a/instrumentation/delayed_job/test/opentelemetry/instrumentation/delayed_job/plugins/tracer_plugin_test.rb +++ b/instrumentation/delayed_job/test/opentelemetry/instrumentation/delayed_job/plugins/tracer_plugin_test.rb @@ -16,8 +16,7 @@ let(:span) { exporter.finished_spans.last } before do - TestHelper.setup_active_record - + Delayed::Worker.backend.delete_all stub_const('BasicPayload', Class.new do def perform; end end) @@ -50,8 +49,6 @@ def job_data after do OpenTelemetry.propagation = @orig_propagation - - TestHelper.teardown_active_record end describe 'enqueue callback' do @@ -74,11 +71,9 @@ def job_data _(span.attributes['messaging.operation']).must_equal 'publish' _(span.attributes['messaging.message_id']).must_be_kind_of String - _(span.events.size).must_equal 2 - _(span.events[0].name).must_equal 'created_at' + _(span.events.size).must_equal 1 + _(span.events[0].name).must_equal 'run_at' _(span.events[0].timestamp).must_be_kind_of Integer - _(span.events[1].name).must_equal 'run_at' - _(span.events[1].timestamp).must_be_kind_of Integer end describe 'when queue name is set' do @@ -124,7 +119,6 @@ def job_data _(exporter.finished_spans.size).must_equal 1 _(exporter.finished_spans.first.name).must_equal 'default publish' job_run - _(exporter.finished_spans.size).must_equal 2 _(span).must_be_kind_of OpenTelemetry::SDK::Trace::SpanData _(span.name).must_equal 'default process' @@ -138,17 +132,15 @@ def job_data _(span.attributes['messaging.operation']).must_equal 'process' _(span.attributes['messaging.message_id']).must_be_kind_of String - _(span.events.size).must_equal 3 - _(span.events[0].name).must_equal 'created_at' + _(span.events[0].name).must_equal 'run_at' _(span.events[0].timestamp).must_be_kind_of Integer - _(span.events[1].name).must_equal 'run_at' + _(span.events[1].name).must_equal 'locked_at' _(span.events[1].timestamp).must_be_kind_of Integer - _(span.events[2].name).must_equal 'locked_at' - _(span.events[2].timestamp).must_be_kind_of Integer end describe 'when queue name is set' do let(:job_params) { { queue: 'foobar_queue' } } + let(:job_enqueue) { Delayed::Job.enqueue(@basic_payload.new, job_params) } it 'span tags include queue name' do job_run @@ -181,11 +173,10 @@ def job_data it 'has resource name equal to underlying ActiveJob class name' do job_run _(span.attributes['messaging.delayed_job.name']).must_equal 'ErrorPayload' - _(span.events.size).must_equal 4 - _(span.events[3].name).must_equal 'exception' - _(span.events[3].timestamp).must_be_kind_of Integer - _(span.events[3].attributes['exception.type']).must_equal 'ArgumentError' - _(span.events[3].attributes['exception.message']).must_equal 'This job failed' + _(span.events[2].name).must_equal 'exception' + _(span.events[2].timestamp).must_be_kind_of Integer + _(span.events[2].attributes['exception.type']).must_equal 'ArgumentError' + _(span.events[2].attributes['exception.message']).must_equal 'This job failed' end end end diff --git a/instrumentation/delayed_job/test/opentelemetry/instrumentation/delayed_job_test.rb b/instrumentation/delayed_job/test/opentelemetry/instrumentation/delayed_job_test.rb index 33ad547bd..907122b34 100644 --- a/instrumentation/delayed_job/test/opentelemetry/instrumentation/delayed_job_test.rb +++ b/instrumentation/delayed_job/test/opentelemetry/instrumentation/delayed_job_test.rb @@ -11,6 +11,7 @@ let(:exporter) { EXPORTER } before do + Delayed::Worker.backend.delete_all instrumentation.install exporter.reset end @@ -48,14 +49,6 @@ end describe 'tracing' do - before do - TestHelper.setup_active_record - end - - after do - TestHelper.teardown_active_record - end - it 'before job' do _(exporter.finished_spans.size).must_equal 0 end diff --git a/instrumentation/delayed_job/test/test_helper.rb b/instrumentation/delayed_job/test/test_helper.rb index 76ff7bba1..2f2dd853d 100644 --- a/instrumentation/delayed_job/test/test_helper.rb +++ b/instrumentation/delayed_job/test/test_helper.rb @@ -7,8 +7,13 @@ require 'bundler/setup' Bundler.require(:default, :development, :test) +# These are dependencies that delayed job assumes are already loaded +# We are compensating for that here in this test... that is a smell +# NoMethodError: undefined method `extract_options!' for [#, {}]:Array +# delayed_job-4.1.11/lib/delayed/backend/job_preparer.rb:7:in `initialize'0 +require 'active_support/core_ext/array' + require 'opentelemetry-instrumentation-delayed_job' -require 'active_support/core_ext/kernel/reporting' require 'minitest/autorun' require 'rspec/mocks/minitest_integration' @@ -24,31 +29,8 @@ c.add_span_processor span_processor end -ActiveRecord::Migration.verbose = false - -module TestHelper - extend self - - def setup_active_record - ::ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:') - ::ActiveRecord::Schema.define do - create_table 'delayed_jobs', force: :cascade do |t| - t.integer 'priority', default: 0, null: false - t.integer 'attempts', default: 0, null: false - t.text 'handler', null: false - t.text 'last_error' - t.datetime 'run_at' - t.datetime 'locked_at' - t.datetime 'failed_at' - t.string 'locked_by' - t.string 'queue' - t.datetime 'created_at' - t.datetime 'updated_at' - end - end - end - - def teardown_active_record - ::ActiveRecord::Base.connection.close - end -end +gem_dir = Gem::Specification.find_by_name('delayed_job').gem_dir +require "#{gem_dir}/spec/delayed/serialization/test" +require "#{gem_dir}/spec/delayed/backend/test" + +Delayed::Worker.backend = Delayed::Backend::Test::Job