shadabahmed / logstasher

Awesome rails logs
MIT License
817 stars 136 forks source link

Use ActiveJob’s serialiser to serialise AJ arguments #153

Closed dv closed 3 years ago

dv commented 4 years ago

Summary: This solves the issue where keyword arguments are not GlobalID’d.

In this line: https://github.com/shadabahmed/logstasher/blob/08b6db91d5fbec7d584b1e7325c71b541b556054/lib/logstasher/active_job/log_subscriber.rb#L87

LogStasher is manually converting the arguments that respond to to_global_id to GlobalID strings. However, this code ignores keyword arguments, which are passed in as a Hash.

Example:

user = User.first
Job.perform_later(author: user)

This would result in a log entry that uses the output of user.to_json instead of user.to_global_id.to_s, causing all properties of user to be included in the log entry. This in contrast to the following which is supported by Logstasher:

user = User.first
Job.perform_later(user)

That would hit the to_global_id.to_s call in args_info and result in the GlobalID string of user to be used instead of the full to_json.

We could leverage ActiveJob itself to not reinvent the wheel and use

def args_info(job)
  ActiveJob::Arguments.serialize(arguments)
end

The downside to this is that it changes the output of GID arguments from this:

"gid://report/User/9231491"

to this:

{"_aj_globalid"=>"gid://report/User/9231491"}

I'm not sure if that's wanted.

shadabahmed commented 3 years ago

Test are failing. Will merge manually and revert this change