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.

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: remove call to ActiveSupport::Notifications.notifier#synchronize deprecated in Rails 7.2 #707

Merged
merged 6 commits into from
Oct 31, 2023

Conversation

octokatherine
Copy link
Contributor

@octokatherine octokatherine commented Oct 25, 2023

When running Rails 7.2, we get the error

NoMethodError: undefined method `synchronize' for #<ActiveSupport::Notifications::Fanout (1436 patterns)>
...opentelemetry/instrumentation/active_support/span_subscriber.rb:33:in `subscribe'

because the private synchronize API was removed here: rails/rails@bcdeea5.

This span_subscriber code block was a workaround for a bug that was fixed in Rails 7.0, broken again in Rails 7.1, and will be fixed in Rails 7.1.2: rails/rails#49784

I wrapped the workaround code in a conditional based on Rails version, since the code fails in 7.2 but will also be unneeded.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 25, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

subscribers.unshift(
subscribers.delete(subscriber_object)
)
if Gem::Version.new(::Rails.version) < Gem::Version.new('7.2')
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what's best practice, but alternative would be to check if synchronize method is present to make it more like feature detection instead of version detection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that idea, I'll update it

Copy link
Collaborator

@arielvalentin arielvalentin left a comment

Choose a reason for hiding this comment

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

Thanks again @octokatherine I will get a release out a bit later today.

@arielvalentin
Copy link
Collaborator

@robertlaurin @simi merging if there are no objections.

We need some help setting up canaries against Rails edge if you are up for it @octokatherine !!!

subscribers.unshift(
subscribers.delete(subscriber_object)
)
if ::ActiveSupport::Notifications.notifier.respond_to?(:synchronize)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to leave any comments to our future selves that we can remove this block once everything prior to 7.2 is EoL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to add a comment!

@arielvalentin arielvalentin merged commit 828e137 into open-telemetry:main Oct 31, 2023
45 of 46 checks passed
Hareramrai pushed a commit to tools-aoeur/opentelemetry-ruby-contrib that referenced this pull request Nov 2, 2023
* 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 <[email protected]>

* 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 <[email protected]>

* fix: bump gem versions

* fix: bump gems

---------

Co-authored-by: Ariel Valentin <[email protected]>
Co-authored-by: Ariel Valentin <[email protected]>
Co-authored-by: Ariel Valentin <[email protected]>
Co-authored-by: Josef Šimánek <[email protected]>

* 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 <[email protected]>
Co-authored-by: Ariel Valentin <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>
Co-authored-by: Francis Bogsanyi <[email protected]>

* 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] <[email protected]>
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] <[email protected]>
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] <[email protected]>
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] <[email protected]>
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] <[email protected]>
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 <[email protected]>

* 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 <[email protected]>
Co-authored-by: Ariel Valentin <[email protected]>

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Ariel Valentin <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Sam Bostock <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Ariel Valentin <[email protected]>
Co-authored-by: Xuan <[email protected]>
Co-authored-by: Robert Mosolgo <[email protected]>
Co-authored-by: Yohei Kitamura <[email protected]>
Co-authored-by: Ariel Valentin <[email protected]>
Co-authored-by: Josef Šimánek <[email protected]>
Co-authored-by: Jean byroot Boussier <[email protected]>
Co-authored-by: Jean Boussier <[email protected]>
Co-authored-by: Muriel <[email protected]>
Co-authored-by: Francis Bogsanyi <[email protected]>
Co-authored-by: Sander Jans <[email protected]>
Co-authored-by: Katherine Oelsner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants