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 dependency on ActiveSupport core extensions from Grape instrumentation #706

Conversation

muripic
Copy link
Contributor

@muripic muripic commented Oct 23, 2023

Addresses #705

I checked if we are using any other core extensions apart from blank? using this as a reference: https://guides.rubyonrails.org/active_support_core_extensions.html

Also, instead of replacing directly with empty?, I added an extra check for nil?. We shouldn't get nil values here, but I added it as a safeguard in case there are any changes in the Grape gem that change this behaviour.

Another case that was covered by blank? but not by empty? is pure whitespace strings, but I decided not to add this since it would be very unlikely to have path with whitespace.

@@ -96,7 +96,7 @@ def path(endpoint)
version = endpoint.routes.first.options[:version] || ''
prefix = endpoint.routes.first.options[:prefix]&.to_s || ''
parts = [prefix, version] + namespace.split('/') + endpoint.options[:path]
parts.reject { |p| p.blank? || p.eql?('/') }.join('/').prepend('/')
parts.reject { |p| p.nil? || p.empty? || p.eql?('/') }.join('/').prepend('/')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would safe navigation work?

Suggested change
parts.reject { |p| p.nil? || p.empty? || p.eql?('/') }.join('/').prepend('/')
parts.reject { |p| p&.empty? || p.eql?('/') }.join('/').prepend('/')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, no, because p&.empty? returns nil if p is nil, which evaluates to false as a boolean.

3.1.3 :001 > p = nil
 => nil
3.1.3 :002 > p&.empty?
 => nil
3.1.3 :003 > p.nil? || p.empty? || p.eql?('/')
 => true
3.1.3 :004 > p&.empty? || p.eql?('/')
 => false

I guess there are refactor alternatives, but even though it is a bit verbose I thought it was quite readable like this. But of course I'm open to other suggestions 😃

@arielvalentin arielvalentin merged commit c5f5c58 into open-telemetry:main Oct 23, 2023
47 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.

2 participants