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

Support serializing ActiveRecord job arguments in global id form #1688

Merged
merged 2 commits into from
Feb 1, 2022

Conversation

st0012
Copy link
Collaborator

@st0012 st0012 commented Jan 14, 2022

Thanks for @bjeanes's proposal and on the idea and implementation in #1643

Closes #1643

@st0012 st0012 added this to the 5.1.0 milestone Jan 14, 2022
@st0012 st0012 self-assigned this Jan 14, 2022
@st0012 st0012 added this to In progress in 5.x via automation Jan 14, 2022
@st0012 st0012 force-pushed the serialize-active-job-arguments branch from e0079ae to 0783c46 Compare January 22, 2022 12:02
@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2022

Codecov Report

Merging #1688 (1045a21) into master (d6b63bb) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
sentry-rails/lib/sentry/rails/active_job.rb 100.00% <100.00%> (ø)
sentry-rails/spec/sentry/rails/activejob_spec.rb 99.37% <100.00%> (+0.08%) ⬆️
sentry-ruby/lib/sentry/background_worker.rb 100.00% <0.00%> (+5.40%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6b63bb...1045a21. Read the comment docs.

end
rescue StandardError
argument
end
Copy link
Contributor

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

Copy link

@bjeanes bjeanes Jan 25, 2022

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.

Copy link
Collaborator Author

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 👍

Copy link
Collaborator Author

@st0012 st0012 Jan 29, 2022

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok 😅

Copy link
Collaborator Author

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 😄

Copy link
Collaborator Author

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 😄

@st0012 st0012 force-pushed the serialize-active-job-arguments branch from 0783c46 to e2e5fef Compare January 29, 2022 16:29
@st0012 st0012 force-pushed the serialize-active-job-arguments branch from e2e5fef to 1045a21 Compare January 29, 2022 16:56
@st0012 st0012 merged commit 4217838 into master Feb 1, 2022
5.x automation moved this from In progress to Done Feb 1, 2022
@st0012 st0012 deleted the serialize-active-job-arguments branch February 1, 2022 10:06
lewispb added a commit to lewispb/sentry-ruby that referenced this pull request Feb 1, 2022
* 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)
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 this pull request may close these issues.

Serialize ActiveRecord arguments to ActiveJob instances as GlobalIDs
5 participants