-
-
Notifications
You must be signed in to change notification settings - Fork 494
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
Support serializing ActiveRecord job arguments in global id form #1688
Conversation
e0079ae
to
0783c46
Compare
Codecov Report
@@ Coverage Diff @@
## master #1688 +/- ##
==========================================
+ Coverage 98.56% 98.59% +0.03%
==========================================
Files 136 136
Lines 7782 7808 +26
==========================================
+ Hits 7670 7698 +28
+ Misses 112 110 -2
Continue to review full report at Codecov.
|
end | ||
rescue StandardError | ||
argument | ||
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.
Presuming we have Active Support available here in sentry-rails, we could perhaps simplify the implementation with #deep_transform_values
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.
Good point. I think this code is roughly based on my initial issue wherein I didn't want to assume ActiveSuport, but given this is would be in the -rails
gem, seems fine to assume it's there.
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.
@lewispb Yeah that makes sense. I'll update it 👍
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.
Well I did simplify the implementation to:
def sentry_serialize_arguments(argument)
case argument
when Hash
argument.deep_transform_values { |v| sentry_serialize_arguments(v) }
when Array, Enumerable
argument.map { |v| sentry_serialize_arguments(v) }
when ->(v) { v.respond_to?(:to_global_id) }
argument.to_global_id.to_s
else
argument
end
rescue StandardError
argument
end
But it turns out deep_transform_values
is added after Rails 6.0. And we're still supporting Rails 5.*, so....
I'll check if just transform_value
will work.
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.
Ah ok 😅
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.
@lewispb fyi transform_values
also works 😄
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.
@lewispb fyi transform_values
also works 😄
0783c46
to
e2e5fef
Compare
e2e5fef
to
1045a21
Compare
* master: feat(performance): Sync activerecord and net-http span names (getsentry#1681) Register Sentry's ErrorSubscriber for Rails 7.0+ apps (getsentry#1705) Support serializing ActiveRecord job arguments in global id form (getsentry#1688) release: 5.0.2 Fix report_after_job_retries's decision logic (getsentry#1704) Followup of getsentry#1701 (getsentry#1703) Capture transaction tags (getsentry#1701)
Thanks for @bjeanes's proposal and on the idea and implementation in #1643
Closes #1643