Conversation
jeffwidman
left a comment
There was a problem hiding this comment.
This was a quick spike I started on last night for fun, but it's not top priority so annotating the changes in case I don't get back to it for a bit.
There was a problem hiding this comment.
I'm unclear if we actually need this given we already require "sentry-ruby" in fetch_files.rb/update_files.rb.
updater/lib/dependabot/sentry.rb
Outdated
There was a problem hiding this comment.
This should be quick if I can get into the debugger to see the payload, but for some reason when I drop a debugger statement here, isn't firing during unit tests... Maybe they've mocked the actual Sentry call so the caller of this (before_send()) never fires?
updater/lib/dependabot/setup.rb
Outdated
There was a problem hiding this comment.
TODO: still need to research if this should be replaced with anything, or simply deleted...
updater/lib/dependabot/setup.rb
Outdated
There was a problem hiding this comment.
TODO: need to get this to fire so I can debug this section of code, and see what the payloads look like...
updater/lib/dependabot/setup.rb
Outdated
There was a problem hiding this comment.
Ruby n00b question: What does this construct do?
I see inline assignment, does it also check if hint[:exception] is nil or does it check if it's false?
There was a problem hiding this comment.
It checks whether hint[:exception] is either nil or false, that's usually called "falsy".
updater/lib/dependabot/setup.rb
Outdated
There was a problem hiding this comment.
I'll need to look into this... at a minimum, I'll probably want to rename this to sentry_context
There was a problem hiding this comment.
If I can figure out how to get access to the debugger this should be straightforward...
89a7bad to
33bcc86
Compare
updater/lib/dependabot/sentry.rb
Outdated
There was a problem hiding this comment.
Opened upstream feature / pull requests:
There was a problem hiding this comment.
It's merged, so I updated the PR, but can't resolve this TODO until it lands it an actual release and that release gets pulled into this PR...
33bcc86 to
02a4ba9
Compare
The Sentry team deprecated `sentry-raven` nearly three years ago: * getsentry/sentry-ruby#1125 So this migrates to their replacement `sentry-ruby` gem. Beyond the namespace changes, the major logical changes are: 1. Events now default to sending asynchronously using a thread pool whereas raven used to send them synchronously. 2. The `raven_context` attached to exceptions is no longer automatically picked up. It must be explicitly wired in. 3. Processor classes were removed. This didn't matter much to us as we had only a single `ExceptionSanitizer`, which is easily wired into the `before_send()` interface. 4. The `config.logger` param was removed.
02a4ba9 to
c4070f4
Compare
| @@ -1,29 +1,29 @@ | |||
| # frozen_string_literal: true | |||
There was a problem hiding this comment.
Since this file is no longer using RavenProcessor, I should actually further move out the sentry-specific bits to the caller and keep this as a generic exception message processing class.
That will also make testing it a bit easier since I can test solely against exception strings.
This also means the looping through multiple exceptions should get moved to the caller, but I already have to do that anyway because of the other processing we need to do in before_send, like applying raven_context.
|
Closing in favor of @JamieMagee 's great work in #8818 . My work wasn't entirely in vain, as we're benefiting from getsentry/sentry-ruby#2072 which I discovered while working on this draft PR. |
The Sentry team deprecated
sentry-ravennearly three years ago:So this migrates to their replacement
sentry-rubygem.Beyond the namespace changes, the major logical changes are:
raven_contextattached to exceptions is no longer automatically picked up. It must be explicitly wired in.ExceptionSanitizer, which is easily wired into thebefore_send()interface.config.loggerparam was removed.