Skip to content

Commit

Permalink
Upgrade Sentry SDKs
Browse files Browse the repository at this point in the history
This commit replaces `sentry-raven` with the more modern Sentry SDKs.

Settings related to this change:

- `sentry_disabled`: completely disables Sentry, in case anything goes
  sideways (defaults to `false`)

refs DE-922
flag=none

test plan:
- make sure to check out the correct patchset in the
  `gems/plugins/uuid_provisioner` directory (see the tag below for ref)
- create a new project in Sentry (it may be easiest to set up Sentry to
  run locally; check out `getsentry/onpremise` on GitHub for setup
  details; it's not too onerous. note that you will need to add
  `VIRTUAL_HOST` and `VIRTUAL_PORT` to the `nginx` container in that
  `docker-compose.yml` in order to have your Sentry accessible via Dory
  at `sentry.docker`)
- configure `config/sentry.rb` with your DSN
- restart rails and job servers
- confirm that, by default:
  - any errors raised in controllers appear in the "Issues" section
  - any errors raised in jobs appear in the "Issues" section
  - user context associated with issues includes user IP and global ID
  - job issues contain context from the job under the "INST-JOBS" section
- set `sentry_disabled` setting to true and restart rails/jobs servers
- confirm that the app/jobs start fine and there are no issues
  - also confirm that Sentry is disabled (no errors received)
- remove `config/sentry.yml` and restart rails and jobs servers
- confirm that the app/jobs start fine and there are no issues

[pin-commit-uuid_provisioner=735c2102fb0020ac5aa80734a323bf08322d002d]

Change-Id: Id0fa4b4ee57ab812cd75d21d2e8ab5e21177af1a
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/279454
Reviewed-by: Ben Rinaca <[email protected]>
QA-Review: Isaac Moore <[email protected]>
Product-Review: Isaac Moore <[email protected]>
Tested-by: Service Cloud Jenkins <[email protected]>
  • Loading branch information
rmsy committed Jan 10, 2022
1 parent ff4e0df commit 3e748c1
Show file tree
Hide file tree
Showing 9 changed files with 37 additions and 31 deletions.
6 changes: 4 additions & 2 deletions Gemfile.d/app.rb
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@
gem "ruby_parser", "3.18.1", require: false
gem "icalendar", "2.7.0", require: false
gem "diplomat", "2.5.1", require: false
gem "ims-lti", "2.3.0", require: "ims"
gem "ims-lti", "2.3.1", require: "ims"
gem "inst_access", "0.1.1"
gem "inst_statsd", "2.2.0"
gem "statsd-ruby", "1.4.0", require: false
Expand Down Expand Up @@ -131,7 +131,9 @@
gem "saml2", "3.1.1"
gem "nokogiri-xmlsec-instructure", "0.10.1", require: false
gem "sanitize", "6.0.0", require: false
gem "sentry-raven", "2.13.0", require: false
gem "sentry-ruby", "4.8.1", github: "rmsy/sentry-ruby"
gem "sentry-rails", "4.8.1", github: "rmsy/sentry-ruby"
gem "sentry-inst_jobs", "1.0.0"
gem "simple_oauth", "0.3.1", require: false
gem "twilio-ruby", "5.36.0", require: false
gem "vault", "0.15.0", require: false
Expand Down
2 changes: 1 addition & 1 deletion Gemfile.d/test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
gem "json-schema", "~> 2.8.1"

gem "parallel_tests"
gem "rspecq", github: "kyler-instructure/rspecq", ref: "8e8fef4f564a9880ab99d0012b5a91a597d3f39d"
gem "rspecq", github: "kyler-instructure/rspecq", ref: "b86e1fd21cb9917b76101a8ae4f24949cd94d043"
gem "flakey_spec_catcher", "~> 0.11.2", require: false
gem "factory_bot", "6.1.0", require: false
gem "rspec_junit_formatter", require: false
Expand Down
25 changes: 14 additions & 11 deletions config/initializers/sentry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,24 +19,25 @@

# This initializer is for the Sentry exception tracking system.
#
# "Raven" is the ruby library that is the client to sentry, and it's
# config file would be "config/raven.yml". If that config doesn't exist,
# Sentry's config file would be "config/sentry.yml". If that config doesn't exist,
# nothing happens. If it *does*, we register a callback with Canvas::Errors
# so that every time an exception is reported, we can fire off a sentry
# call to track it and aggregate it for us.
settings = ConfigFile.load("raven")
settings = ConfigFile.load("sentry")

if settings.present?
require "raven/base"
Raven.configure do |config|
config.logger = Rails.logger
config.silence_ready = true
return if Canvas::Plugin.value_to_boolean(Setting.get("sentry_disabled", "false"))

Sentry.init do |config|
config.dsn = settings[:dsn]
config.current_environment = Canvas.environment
config.tags = settings.fetch(:tags, {}).merge("canvas_revision" => Canvas.revision)
config.environment = Canvas.environment
config.release = Canvas.revision
config.sanitize_fields += Rails.application.config.filter_parameters.map(&:to_s)
config.sanitize_credit_cards = false

filter = ActiveSupport::ParameterFilter.new(Rails.application.config.filter_parameters)
config.before_send = lambda do |event, _|
filter.filter(event.to_hash)
end

# this array should only contain exceptions that are intentionally
# thrown to drive client facing behavior. A good example
# are login/auth exceptions. Exceptions that are simply noisy/inconvenient
Expand All @@ -53,6 +54,8 @@
]
end

Sentry.set_tags(settings.fetch(:tags, {}))

Rails.configuration.to_prepare do
Setting.get("ignorable_errors", "").split(",").each do |error|
SentryProxy.register_ignorable_error(error)
Expand Down
File renamed without changes.
4 changes: 2 additions & 2 deletions config/vault_contents.yml.example
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ development:
# inst_access_signature:
# private_key: <private key in base 64>
# encryption_public_key: <public key in base 64 [NOT SAME KEYPAIR AS SIGNATURE!]>
# raven_dsn: garbage-dsn-here
# sentry_dsn: garbage-dsn-here
# twilio_creds:
# account_sid: <sid>
# auth_token: <token>
Expand All @@ -32,4 +32,4 @@ test:
'sts/testaccount/sts/canvas-shards-lookupper-test':
access_key: 'fake-access-key'
secret_key: 'fake-secret-key'
security_token: 'fake-security-token'
security_token: 'fake-security-token'
4 changes: 2 additions & 2 deletions gems/canvas_errors/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ When things go wrong, we want to know about it. When things error out in expect

### Where do we capture exceptions for analysis?

The short answer is that they get sent to [sentry](https://www.sentry.io). Mostly we use the default sentry-raven integration (see config/initializers/sentry.rb in canvas-lms). This means that any time an UNHANDLED exception pops all the way out of the rails process, we’ll tell sentry.
The short answer is that they get sent to [sentry](https://www.sentry.io). Mostly we use the Sentry integration (see config/initializers/sentry.rb in canvas-lms). This means that any time an UNHANDLED exception pops all the way out of the rails process, we’ll tell Sentry.

We also sometimes report errors that we handle if it’s important to report them (because it’s unexpected) but also not explode (because we’re in the middle of doing something important that is continuable). See page view logging in canvas for an example: `app/controllers/application_controller.rb`. Because we register sentry as a callback from Canvas Errors, things that we send there also get sent to Sentry. An error can be captured from anywhere using the capture method:

Expand Down Expand Up @@ -87,4 +87,4 @@ whether or not to take action for a given error based on it's level.
## Running Tests

This gem is tested with rspec. You can use `test.sh` to run it, or
do it yourself with `bundle exec rspec spec`.
do it yourself with `bundle exec rspec spec`.
3 changes: 3 additions & 0 deletions lib/authentication_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,9 @@ def load_user
end

logger.info "[AUTH] final user: #{@current_user&.id}"
if Sentry.initialized? && !Rails.env.test?
Sentry.set_user({ id: @current_user&.global_id, ip_address: request.remote_ip }.compact)
end
@current_user
end
private :load_user
Expand Down
6 changes: 2 additions & 4 deletions lib/sentry_proxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,12 @@
# You should have received a copy of the GNU Affero General Public License along
# with this program. If not, see <http://www.gnu.org/licenses/>.

require "raven/base"

class SentryProxy
def self.capture(exception, data, level = :error)
if exception.is_a?(String) || exception.is_a?(Symbol)
Raven.capture_message(exception.to_s, data) if reportable?(exception.to_s, level)
Sentry.capture_message(exception.to_s, data) if reportable?(exception.to_s, level)
elsif reportable?(exception, level)
Raven.capture_exception(exception, data)
Sentry.capture_exception(exception, data)
end
end

Expand Down
18 changes: 9 additions & 9 deletions spec/lib/sentry_proxy_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,34 +26,34 @@
before { SentryProxy.clear_ignorable_errors }

describe ".capture" do
it "forwards exceptions on to raven" do
it "forwards exceptions on to sentry" do
e = error_klass.new
expect(Raven).to receive(:capture_exception).with(e, data)
expect(Sentry).to receive(:capture_exception).with(e, data)
SentryProxy.capture(e, data)
end

it "passes messages to the capture_message raven method" do
it "passes messages to the capture_message sentry method" do
e = "Some Message"
expect(Raven).to receive(:capture_message).with(e, data)
expect(Sentry).to receive(:capture_message).with(e, data)
SentryProxy.capture(e, data)
end

it "changes symbols to strings because raven chokes otherwise" do
it "changes symbols to strings because sentry chokes otherwise" do
e = :some_exception_type
expect(Raven).to receive(:capture_message).with("some_exception_type", data)
expect(Sentry).to receive(:capture_message).with("some_exception_type", data)
SentryProxy.capture(e, data)
end

it "does not send the message if configured as ignorable" do
SentryProxy.register_ignorable_error(error_klass)
e = error_klass.new
expect(Raven).to_not receive(:capture_exception)
expect(Sentry).to_not receive(:capture_exception)
SentryProxy.capture(e, data)
end

it "does not send to sentry for low-level errors" do
e = error_klass.new
expect(Raven).not_to receive(:capture_exception)
expect(Sentry).not_to receive(:capture_exception)
SentryProxy.capture(e, data, :warn)
end
end
Expand All @@ -73,7 +73,7 @@
it "registers strings to skip capture_message" do
e = "Some Message"
SentryProxy.register_ignorable_error(e)
expect(Raven).to_not receive(:capture_message)
expect(Sentry).to_not receive(:capture_message)
SentryProxy.capture(e, data)
end
end
Expand Down

0 comments on commit 3e748c1

Please sign in to comment.