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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## Unreleased

### Features

- Support serializing ActiveRecord job arguments in global id form [#1688](https://github.com/getsentry/sentry-ruby/pull/1688)

## 5.0.2

- Respect port info provided in user's DSN [#1702](https://github.com/getsentry/sentry-ruby/pull/1702)
Expand Down
17 changes: 15 additions & 2 deletions sentry-rails/lib/sentry/rails/active_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ def capture_and_reraise_with_sentry(scope, &block)
extra: sentry_context,
tags: {
job_id: job_id,
provider_job_id:provider_job_id
provider_job_id: provider_job_id
}
)
raise e
Expand All @@ -57,13 +57,26 @@ def already_supported_by_sentry_integration?
def sentry_context
{
active_job: self.class.name,
arguments: arguments,
arguments: sentry_serialize_arguments(arguments),
scheduled_at: scheduled_at,
job_id: job_id,
provider_job_id: provider_job_id,
locale: locale
}
end

def sentry_serialize_arguments(argument)
case argument
when Hash
argument.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 rescue argument
else
argument
end
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 😄

end
end
end
52 changes: 52 additions & 0 deletions sentry-rails/spec/sentry/rails/activejob_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ def perform
end
end

class JobWithArgument < ActiveJob::Base
def perform(*args, integer:, post:, **options)
raise "foo"
end
end

class QueryPostJob < ActiveJob::Base
self.logger = nil

Expand Down Expand Up @@ -76,6 +82,52 @@ def rescue_callback(error)
expect(NormalJob.perform_now).to eq("foo")
end

describe "ActiveJob arguments serialization" do
it "serializes ActiveRecord arguments in globalid form" do
post = Post.create!
post2 = Post.create!

expect do
JobWithArgument.perform_now("foo", { bar: Sentry }, integer: 1, post: post, nested: { another_level: { post: post2 } })
end.to raise_error(RuntimeError)

event = transport.events.last.to_json_compatible
expect(event.dig("extra", "arguments")).to eq(
[
"foo",
{ "bar" => "Sentry" },
{
"integer" => 1,
"post" => "gid://rails-test-app/Post/#{post.id}",
"nested" => { "another_level" => { "post" => "gid://rails-test-app/Post/#{post2.id}" } }
}
]
)
end

it "handles problematic globalid conversion gracefully" do
post = Post.create!

def post.to_global_id
raise
end

expect do
JobWithArgument.perform_now(integer: 1, post: post)
end.to raise_error(RuntimeError)

event = transport.events.last.to_json_compatible
expect(event.dig("extra", "arguments")).to eq(
[
{
"integer" => 1,
"post" => post.to_s,
}
]
)
end
end

it "adds useful context to extra" do
expect { FailedJob.perform_now }.to raise_error(FailedJob::TestError)

Expand Down