-
-
Notifications
You must be signed in to change notification settings - Fork 520
Strip ActiveJob keys to prevent recursion #386
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,35 @@ def call(_worker, msg, _queue) | |
| end | ||
| else | ||
| Sidekiq.configure_server do |config| | ||
| config.error_handlers << Proc.new {|ex,context| Raven.capture_exception(ex, :extra => {:sidekiq => context}) } | ||
| config.error_handlers << Proc.new { |ex, context| | ||
| Raven.capture_exception(ex, :extra => { | ||
| :sidekiq => filter_context(context), | ||
| }) | ||
| } | ||
| end | ||
| end | ||
|
|
||
| def filter_context(context) | ||
| case context | ||
| when Array | ||
| context.map { |arg| filter_context(arg) } | ||
| when Hash | ||
| Hash[context.map { |key, value| filter_context_hash(key, value) }] | ||
| else | ||
| context | ||
| end | ||
| end | ||
|
|
||
| def filter_context_hash(key, value) | ||
| # Strip any `_aj` prefixes from keys. | ||
| # These keys come from an internal serialized object from ActiveJob. | ||
| # Internally, there are a subset of keys that ActiveJob references, but | ||
| # these are declared as private, and I don't think it's wise | ||
| # to keep chasing what this list is. But they all use a common prefix, so | ||
| # we want to strip this becuase ActiveJob will complain. | ||
| # e.g.: _aj_globalid -> _globalid | ||
| if key[0..3] == '_aj_' | ||
| key = key[3..-1] | ||
| end | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't it be better to remove the keys entirely? If the ActiveJob job to send this event to Sentry fails, the metadata will be overridden if it's re-queued, so at the very least it's not reliable data.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't sure if it'd be safe to strip them entirely or not. That was my initial thought, but decided it'd probably be better to err on the side of preserving as much data as possible.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @seanlinsley's right, this is internal private state that's likely wrong/unreliable anyway. We should just drop them. |
||
| [key, filter_context(value)] | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we can access the
RESERVED_KEYSconstant directly here? (I think that's what it's called)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'll dedup the knowledge of what a "forbidden" key is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to, but it appears to be private.
https://github.com/rails/rails/blob/master/activejob/lib/active_job/arguments.rb#L130-L135
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I err'd on the side of being liberal and catching
_aj_prefix rather than duping the list in here and worrying about maintaining that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose the only truly maintainable solution in the long term would be to
rescueSerializationErrors. Hm.