fix!: Drop DelayedJob ActiveRecord in Tests #685

arielvalentin · 2023-10-07T16:45:01Z

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.

See: #686

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.

simi · 2023-10-07T19:56:40Z

instrumentation/delayed_job/test/test_helper.rb

-  end
-end
+gem_dir = Gem::Specification.find_by_name('delayed_job').gem_dir
+require "#{gem_dir}/spec/delayed/serialization/test"


Any reason to load this empty file?

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

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

I removed this line and the tests pass.

simi · 2023-10-07T19:57:47Z

instrumentation/delayed_job/test/test_helper.rb

-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"


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?

That would be great! Thank you

🙏 collectiveidea/delayed_job#1195

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

definitely not blocker from my side

simi · 2023-10-07T19:59:26Z

instrumentation/delayed_job/test/test_helper.rb

+# 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'


Any reason to not just require delayed_job itself here?

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

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

🤔

That is very curious isn't it!

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

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

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

Explicitly requiring delayed_job and still get this error.

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

instrumentation/delayed_job/test/test_helper.rb

instrumentation/delayed_job/opentelemetry-instrumentation-delayed_job.gemspec

@simi

* chore: use stable toys version * chore: use stable version of toys release code * release: Release 2 gems (open-telemetry#675) * fix: Fix "uses cases" typo in `CONTRIBUTING.md` (open-telemetry#679) * chore(deps-dev): update webmock requirement from ~> 3.18.1 to ~> 3.19.1 in /resources/azure (open-telemetry#653) * chore(deps-dev): update webmock requirement from ~> 3.18.1 to ~> 3.19.1 in /resources/google_cloud_platform (open-telemetry#652) * fix: Remove dependence on activesupport (open-telemetry#687) Instrumentations must not use transitive dependencies used by the library. In this case, relying on the gruf library to load specific ActiveSupport extensions leaves the instrumentation vulnerable to bugs. For this reason I have changed the code to use Enumerable methods instead of ActiveSupport extensions. See open-telemetry#686 * fix!: Drop DelayedJob ActiveRecord in Tests (open-telemetry#685) * fix: Add Rails 7.1 compatability (open-telemetry#684) * chore: Add tests for Rails 7.1 * fix: Rails 7.1 incompatabilities * feat!: obfuscation for mysql2, dalli and postgresql as default option for db_statement (open-telemetry#682) * feat!: fuscation for mysql2, dalli and pg * feat!: update readme * feat!: set db.statement option to obfuscate by default for mysql2, pg and dalli Co-authored-by: Ariel Valentin <arielvalentin@users.noreply.github.com> * ci: Update test reset code for GraphQL-Ruby 2.1.3 (open-telemetry#691) Fixes open-telemetry#689 * fix: Omit `nil` `net.peer.name` attributes (open-telemetry#693) * ci: upgrade to latest stable version of toys (open-telemetry#694) Fixes errors with incompatible versions defined in the configs: https://github.com/open-telemetry/opentelemetry-ruby-contrib/actions/runs/6534421626/job/17741560442#step:5:10 * release: Release 12 gems (open-telemetry#695) * release: Release 12 gems * opentelemetry-instrumentation-gruf 0.1.1 (was 0.1.0) * opentelemetry-instrumentation-active_support 0.4.3 (was 0.4.2) * opentelemetry-instrumentation-action_view 0.6.1 (was 0.6.0) * opentelemetry-instrumentation-action_pack 0.7.1 (was 0.7.0) * opentelemetry-instrumentation-active_job 0.6.1 (was 0.6.0) * opentelemetry-instrumentation-active_record 0.6.3 (was 0.6.2) * opentelemetry-instrumentation-dalli 0.25.0 (was 0.24.2) * opentelemetry-instrumentation-delayed_job 0.22.0 (was 0.21.0) * opentelemetry-instrumentation-faraday 0.23.3 (was 0.23.2) * opentelemetry-instrumentation-mysql2 0.25.0 (was 0.24.3) * opentelemetry-instrumentation-pg 0.26.0 (was 0.25.3) * opentelemetry-instrumentation-rails 0.28.1 (was 0.28.0) * ci: Trigger builds * fix: Apply suggestions from code review Co-authored-by: Josef Šimánek <josef.simanek@gmail.com> * fix: bump gem versions * fix: bump gems --------- Co-authored-by: Ariel Valentin <ariel@arielvalentin.com> Co-authored-by: Ariel Valentin <arielvalentin@github.com> Co-authored-by: Ariel Valentin <arielvalentin@users.noreply.github.com> Co-authored-by: Josef Šimánek <josef.simanek@gmail.com> * chore: Update CODEOWNERS (open-telemetry#692) * chore: Update CODEOWNERS Add @simi and @kaylareopelle and approvers to the list * release: Release opentelemetry-instrumentation-all 0.51.0 (was 0.50.1) (open-telemetry#699) * release: Release opentelemetry-instrumentation-all 0.51.0 (was 0.50.1) * docs: Update all gem Changelog --------- Co-authored-by: Ariel Valentin <ariel@arielvalentin.com> Co-authored-by: Ariel Valentin <arielvalentin@users.noreply.github.com> * feat(trilogy): instrument connect and ping (open-telemetry#704) These can be just as slow as a query if not slower. Co-authored-by: Jean Boussier <jean.boussier@gmail.com> * fix: Remove dependency on ActiveSupport core extensions from Grape instrumentation (open-telemetry#706) * release: Release opentelemetry-instrumentation-trilogy 0.57.0 (was 0.56.3) (open-telemetry#708) * release: Release opentelemetry-instrumentation-trilogy 0.57.0 (was 0.56.3) * Empty commit * Release instrumentation-all as well. --------- Co-authored-by: Ariel Valentin <ariel@arielvalentin.com> Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com> * chore(deps): update rubocop requirement from ~> 1.56.2 to ~> 1.57.1 (open-telemetry#702) Updates the requirements on [rubocop](https://github.com/rubocop/rubocop) to permit the latest version. - [Release notes](https://github.com/rubocop/rubocop/releases) - [Changelog](https://github.com/rubocop/rubocop/blob/master/CHANGELOG.md) - [Commits](rubocop/rubocop@v1.56.2...v1.57.1) --- updated-dependencies: - dependency-name: rubocop dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): update rubocop requirement from ~> 1.56.1 to ~> 1.57.1 in /instrumentation/base (open-telemetry#701) chore(deps-dev): update rubocop requirement from ~> 1.56.1 to ~> 1.57.1 Updates the requirements on [rubocop](https://github.com/rubocop/rubocop) to permit the latest version. - [Release notes](https://github.com/rubocop/rubocop/releases) - [Changelog](https://github.com/rubocop/rubocop/blob/master/CHANGELOG.md) - [Commits](rubocop/rubocop@v1.56.1...v1.57.1) --- updated-dependencies: - dependency-name: rubocop dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): update rubocop requirement from ~> 1.56.1 to ~> 1.57.1 in /resource_detectors (open-telemetry#700) chore(deps-dev): update rubocop requirement from ~> 1.56.1 to ~> 1.57.1 Updates the requirements on [rubocop](https://github.com/rubocop/rubocop) to permit the latest version. - [Release notes](https://github.com/rubocop/rubocop/releases) - [Changelog](https://github.com/rubocop/rubocop/blob/master/CHANGELOG.md) - [Commits](rubocop/rubocop@v1.56.1...v1.57.1) --- updated-dependencies: - dependency-name: rubocop dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): update rubocop requirement from ~> 1.56.1 to ~> 1.57.1 in /propagator/ottrace (open-telemetry#698) chore(deps-dev): update rubocop requirement from ~> 1.56.1 to ~> 1.57.1 Updates the requirements on [rubocop](https://github.com/rubocop/rubocop) to permit the latest version. - [Release notes](https://github.com/rubocop/rubocop/releases) - [Changelog](https://github.com/rubocop/rubocop/blob/master/CHANGELOG.md) - [Commits](rubocop/rubocop@v1.56.1...v1.57.1) --- updated-dependencies: - dependency-name: rubocop dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): update rubocop requirement from ~> 1.56.1 to ~> 1.57.1 in /propagator/xray (open-telemetry#697) chore(deps-dev): update rubocop requirement from ~> 1.56.1 to ~> 1.57.1 Updates the requirements on [rubocop](https://github.com/rubocop/rubocop) to permit the latest version. - [Release notes](https://github.com/rubocop/rubocop/releases) - [Changelog](https://github.com/rubocop/rubocop/blob/master/CHANGELOG.md) - [Commits](rubocop/rubocop@v1.56.1...v1.57.1) --- updated-dependencies: - dependency-name: rubocop dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Remove opentelemetry-resource_detectors all-in-one gem (open-telemetry#659) * fix: remove call to ActiveSupport::Notifications.notifier#synchronize deprecated in Rails 7.2 (open-telemetry#707) * wrap call to depcrecated private API behind version conditional * convert Rails version string to Gem::Version * fix module access? * check for synchronize method instead of Rails version * add comment --------- Co-authored-by: Ariel Valentin <arielvalentin@users.noreply.github.com> * chore: Fix linter issue * release: Release 2 gems (open-telemetry#710) * release: Release 2 gems * opentelemetry-instrumentation-grape 0.1.5 (was 0.1.4) * opentelemetry-instrumentation-active_support 0.4.4 (was 0.4.3) * ci: Force --------- Co-authored-by: Ariel Valentin <ariel@arielvalentin.com> Co-authored-by: Ariel Valentin <arielvalentin@github.com> --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Ariel Valentin <arielvalentin@github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Sam Bostock <sambostock@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ariel Valentin <arielvalentin@users.noreply.github.com> Co-authored-by: Xuan <112967240+xuan-cao-swi@users.noreply.github.com> Co-authored-by: Robert Mosolgo <rdmosolgo@gmail.com> Co-authored-by: Yohei Kitamura <3087402+yoheyk@users.noreply.github.com> Co-authored-by: Ariel Valentin <ariel@arielvalentin.com> Co-authored-by: Josef Šimánek <josef.simanek@gmail.com> Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com> Co-authored-by: Jean Boussier <jean.boussier@gmail.com> Co-authored-by: Muriel <m.picone.farias@catawiki.nl> Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com> Co-authored-by: Sander Jans <scbjans@gmail.com> Co-authored-by: Katherine Oelsner <49968061+octokatherine@users.noreply.github.com>

arielvalentin requested review from fbogsany, mwear, robertlaurin, dazuma, ericmustin, ahayworth, plantfansam and robbkidd as code owners October 7, 2023 16:45

arielvalentin mentioned this pull request Oct 7, 2023

fix: Add Rails 7.1 compatability #684

Merged

simi reviewed Oct 7, 2023

View reviewed changes

arielvalentin commented Oct 7, 2023

View reviewed changes

instrumentation/delayed_job/test/test_helper.rb Outdated Show resolved Hide resolved

try without loading the emtpy serialization file

428a0a1

arielvalentin requested a review from simi October 7, 2023 21:38

arielvalentin added 2 commits October 7, 2023 16:54

adding explicit delayed_job require

5141f8e

fix: More precise as ext

1a86951

arielvalentin enabled auto-merge (squash) October 8, 2023 20:10

arielvalentin commented Oct 8, 2023

View reviewed changes

instrumentation/delayed_job/opentelemetry-instrumentation-delayed_job.gemspec Outdated Show resolved Hide resolved

Update opentelemetry-instrumentation-delayed_job.gemspec

b3bf814

mwear approved these changes Oct 10, 2023

View reviewed changes

Merge branch 'main' into fix-delayed-job-tests

038db9d

arielvalentin merged commit fc5a75f into open-telemetry:main Oct 10, 2023
4 checks passed

arielvalentin deleted the fix-delayed-job-tests branch October 10, 2023 18:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!: Drop DelayedJob ActiveRecord in Tests #685

fix!: Drop DelayedJob ActiveRecord in Tests #685

arielvalentin commented Oct 7, 2023 •

edited

simi Oct 7, 2023

arielvalentin Oct 7, 2023

arielvalentin Oct 7, 2023

simi Oct 7, 2023

arielvalentin Oct 7, 2023

simi Oct 8, 2023

arielvalentin Oct 9, 2023 •

edited

simi Oct 9, 2023

simi Oct 7, 2023

arielvalentin Oct 7, 2023

simi Oct 7, 2023

arielvalentin Oct 7, 2023

arielvalentin Oct 7, 2023

arielvalentin Oct 7, 2023

arielvalentin Oct 7, 2023

arielvalentin Oct 7, 2023

fix!: Drop DelayedJob ActiveRecord in Tests #685

fix!: Drop DelayedJob ActiveRecord in Tests #685

Conversation

arielvalentin commented Oct 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielvalentin Oct 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielvalentin commented Oct 7, 2023 •

edited

arielvalentin Oct 9, 2023 •

edited