Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix!: Drop DelayedJob ActiveRecord in Tests #685

Merged
merged 6 commits into from
Oct 10, 2023
Merged

fix!: Drop DelayedJob ActiveRecord in Tests #685

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a3ff8ed
fix!: Drop DelayedJob ActiveRecord in Tests
arielvalentin Oct 7, 2023
428a0a1
try without loading the emtpy serialization file
arielvalentin Oct 7, 2023
5141f8e
adding explicit delayed_job require
arielvalentin Oct 7, 2023
1a86951
fix: More precise as ext
arielvalentin Oct 7, 2023
b3bf814
Update opentelemetry-instrumentation-delayed_job.gemspec
arielvalentin Oct 8, 2023
038db9d
Merge branch 'main' into fix-delayed-job-tests
arielvalentin Oct 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions instrumentation/delayed_job/Appraisals
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
1 change: 0 additions & 1 deletion ...tation/delayed_job/lib/opentelemetry/instrumentation/delayed_job/plugins/tracer_plugin.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 1 addition & 2 deletions instrumentation/delayed_job/opentelemetry-instrumentation-delayed_job.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
29 changes: 10 additions & 19 deletions .../delayed_job/test/opentelemetry/instrumentation/delayed_job/plugins/tracer_plugin_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -50,8 +49,6 @@ def job_data

after do
OpenTelemetry.propagation = @orig_propagation

TestHelper.teardown_active_record
end

describe 'enqueue callback' do
Expand All @@ -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
Expand Down Expand Up @@ -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'
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
9 changes: 1 addition & 8 deletions instrumentation/delayed_job/test/opentelemetry/instrumentation/delayed_job_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
let(:exporter) { EXPORTER }

before do
Delayed::Worker.backend.delete_all
instrumentation.install
exporter.reset
end
Expand Down Expand Up @@ -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
Expand Down
40 changes: 11 additions & 29 deletions instrumentation/delayed_job/test/test_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 [#<ActiveJobPayload:0x0000000108bf5d48>, {}]:Array
# delayed_job-4.1.11/lib/delayed/backend/job_preparer.rb:7:in `initialize'0
require 'active_support/core_ext/array'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to not just require delayed_job itself here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delayed_job is loaded by bundler therefore it is not necessary to load it twice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if required properly, it would load this https://github.com/collectiveidea/delayed_job/blob/b66bb64437a8606a414a390531ac73108911434e/lib/delayed_job.rb#L11 and that would load https://github.com/collectiveidea/delayed_job/blob/b66bb64437a8606a414a390531ac73108911434e/lib/delayed/backend/job_preparer.rb#L1

🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is very curious isn't it!

Let me try this again and see what I can figure out

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting in that it fails for Rails 7.0 but not for 7.1 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The curious part for me here is that the backtrace clearly states its raised in JobPreparer, which is what requires the file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicitly requiring delayed_job and still get this error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/open-telemetry/opentelemetry-ruby-contrib/actions/runs/6443662571/job/17495853381?pr=685


require 'opentelemetry-instrumentation-delayed_job'
require 'active_support/core_ext/kernel/reporting'

require 'minitest/autorun'
require 'rspec/mocks/minitest_integration'
Expand All @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to load this empty file?

https://github.com/collectiveidea/delayed_job/blob/v4.1.11/spec/delayed/serialization/test.rb

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not be required since I am using the class instead of a symbol. I will remove it and see if tests pass.

https://github.com/collectiveidea/delayed_job/blob/master/lib/delayed/worker.rb#L67

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this line and the tests pass.

arielvalentin marked this conversation as resolved.
Show resolved Hide resolved
require "#{gem_dir}/spec/delayed/backend/test"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO it would make sense to add PR to delayed job to move this file into lib folder since it is useful for testing depending libs. Should I try to open PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be great! Thank you

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏 collectiveidea/delayed_job#1195

Copy link
Collaborator Author

@arielvalentin arielvalentin Oct 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lead time seems like it is about 3-4 months in my experience collectiveidea/delayed_job#1169

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely not blocker from my side


Delayed::Worker.backend = Delayed::Backend::Test::Job
Loading