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

Proxy configuration in version 5+ no longer accepts a URL as a string #1782

Closed
jknipp opened this issue Apr 1, 2022 · 4 comments · Fixed by #1788
Closed

Proxy configuration in version 5+ no longer accepts a URL as a string #1782

jknipp opened this issue Apr 1, 2022 · 4 comments · Fixed by #1788
Assignees
Projects
Milestone

Comments

@jknipp
Copy link
Contributor

jknipp commented Apr 1, 2022

Issue Description

We have recently been migrating Rails 5.2 apps from sentry-raven to sentry-ruby. We have 2 services that are being migrated, one that is on using sentry-raven 4.x (v4.8 to be exact, using Faraday). And another that we tried to upgrade to sentry-ruby v5.0.2. In the app using v4.8, we were able to use the steps in the (https://docs.sentry.io/platforms/ruby/migration/#renamedrelocated) to rename the proxy configuration option, and pass it a URL string as follows:

config.proxy #=> config.transport.proxy
# Example: config.transport.proxy = "http://myproxy.example.com:port"

However, doing the same when moving to v5.x throws an error when a proxy string is provided. This caused a production deployment issue for us because we only use a proxy in our production environment, and were not able to test for this in our development/testing environments.

Reproduction Steps

  1. Configure Sentry Ruby using v5+, setting the transport.proxy option to a URL string based on the migration guide from sentry-raven.
Sentry.init do |config|
 config.transport.proxy = "http://myproxy.example.com:port"
 ...
end
  1. Force triggering of an exception in a Rails controller method, such as Sentry.capture_exception(StandardError.new('This is an exception'))

Expected Behavior

One of the following

  1. The Sentry Ruby HTTPTransport class should be able to handle the proxy as a URL string, and seamlessly convert the parse the URL string, OR
  2. Sentry documentation should be updated, including the migration guide, to indicate the configuration value for config.transport.proxy should be a hash, and a string is NOT acceptable.

Actual Behavior

A no implicit conversion of Symbol into Integer exception is thrown when the proxy is set as a string instead of a hash. See the HTTPTransport class here. Therefore, events are not sent to Sentry.

See log

2022-04-01T15:20:58Z Sending envelope [event] 081d827b4aca47ecb25328d611027179 to Sentry
2022-04-01T15:20:58Z Event sending failed: no implicit conversion of Symbol into Integer
2022-04-01T15:20:58Z exception happened in background worker: String does not have #dig method
2022-04-01T15:20:58Z Sending envelope [transaction] 7193c21c36534fda8e4030d4e9a2dbd9 to Sentry
2022-04-01T15:20:58Z Transaction sending failed: no implicit conversion of Symbol into Integer
2022-04-01T15:20:58Z Unreported Transaction: V1::XXXXXController#create
2022-04-01T15:20:58Z exception happened in background worker: no implicit conversion of Symbol into Integer

Ruby Version

2.6.6

SDK Version

5+

Integration and Its Version

Rails/Sidekiq

Sentry Config

Sentry.init do |config|
  config.transport.proxy = "http://myproxy.example.com:port"
  ...
end
@jknipp
Copy link
Contributor Author

jknipp commented Apr 1, 2022

@st0012 I'm happy to submit a PR if you can let me know what the preference is for fixing this (code or docs updates).

@jknipp
Copy link
Contributor Author

jknipp commented Apr 1, 2022

Actually, I believe this needs to be handled in code using URI rather than via documentation. The workaround is really suboptimal when URI can parse the username and password from the URL string. Having conditional logic embedded in an initializer/configuration setup feels l like an anti-pattern.

My current workaround:

# config/initializers/sentry.rb

proxy = "http://username:password@proxy.thing.com:8080/"

if proxy
  uri = URI(proxy)
  proxy_hash = { uri:  uri, user: uri.user, password: uri.password }
else
  proxy_hash = nil
end

Sentry.init do |config|
  config.transport.proxy = proxy_hash
end

@st0012 st0012 added this to To do in 5.x via automation Apr 1, 2022
@st0012
Copy link
Collaborator

st0012 commented Apr 8, 2022

@jknipp Sorry for the breaking change, I wasn't aware of it. I think the change should be on code, as it used to be supported.

@jknipp
Copy link
Contributor Author

jknipp commented Apr 8, 2022

Hi @st0012, no worries. I'll try to have a PR up for review later today.

jknipp added a commit to jknipp/sentry-ruby that referenced this issue Apr 8, 2022
Fixes getsentry#1782

Update `config.transport.proxy` to allow String and URI values as
previously supported by `sentry-ruby` versions <= 4.8 using Faraday.
jknipp added a commit to jknipp/sentry-ruby that referenced this issue Apr 8, 2022
Fixes getsentry#1782

Update `config.transport.proxy` to allow String and URI values as
previously supported by `sentry-ruby` versions <= 4.8 using Faraday.
jknipp added a commit to jknipp/sentry-ruby that referenced this issue Apr 8, 2022
Fixes getsentry#1782

Update `config.transport.proxy` to allow String and URI values as
previously supported by `sentry-ruby` versions <= 4.8 using Faraday.
5.x automation moved this from To do to Done Apr 8, 2022
st0012 pushed a commit that referenced this issue Apr 8, 2022
Fixes #1782

Update `config.transport.proxy` to allow String and URI values as
previously supported by `sentry-ruby` versions <= 4.8 using Faraday.
@st0012 st0012 added this to the 5.3.0 milestone Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5.x
Done
Development

Successfully merging a pull request may close this issue.

2 participants