An alternative to the "append_info_to_payload" strategy

[smudge]

This is a solution relating to #94. It started more as a proof-of-concept, but I ended up reworking it a little and writing tests, so it might be useful for you, depending on whether the pain point is actually worth solving.

Essentially, this combines the custom_payload option and the append_info_to_payload method override strategy (see #94) into a single config option that looks like this:

MyApp::Application.configure do
  config.lograge.enabled = true

  # custom_payload must be a proc that returns a hash
  config.lograge.custom_payload = proc do
    {
      host: request.host,
      user_id: current_user.try(:id),
      fwd: request.remote_ip
    }
  end
end

Unfortunately, because of the way that the logging instrumentation uses an ActiveSupport::Notification event to pass the payload through to lograge, I had to perform a bit of magic to get this working. (Notice how request and current_user are magically accessible from within the proc.)

Also, I chose to create a new config (custom_payload) rather than change the behavior of the existing config (custom_options) because the two approaches are not really comparable.

I'd be open to re-implementing this with a bit less magic. Let me know what you think.

[benlovell]

Thanks ❤️ I'll give this a look over the weekend.

[benlovell]

This is an interesting approach and the API feels right but I have some concerns about the 'magic' as you stated and how this would perform in the real-world given it's right there in the critical section. I'll give that a closer look though before making any assumptions.

@pxlpnk any thoughts?

[pxlpnk]

So far I like the idea, I am a bit biased against magic in code especially as this could break every request. Is there any way this could be made more transparent? I see myself hunting a bug in the wrong place somehow. 🎱
Any idea how this could be made a bit more explicit @smudge ?

[benlovell]

@smudge It's been a while but I'm still keen on this approach.

I'd be open to re-implementing this with a bit less magic. Let me know what you think.

Is there still any interest from your part on this? Would be great to land this.

[smudge]

Definitely interested in figuring this out. I was thinking something like this might be a little less magical:

config.lograge.custom_payload do |controller|
  {
    host: controller.request.host,
    user_id: controller.current_user.try(:id),
    fwd: controller.request.remote_ip
  }
end

The block must be executed with the context of the controller, but it's a little more explicit. (Wouldn't need to rely on instance_eval.)

The trick is in passing the custom payload values along with the ActiveSupport::Notification event that lograge listens for. (In the earlier implementation, it adds a new key to the payload called 'custom_payload' and passes the values through that way.) I haven't really thought through how that might be cleaned up. But that's really just an implementation detail. What's most important is making the config API as intuitive as possible.

Here's another alternative (avoiding the use of a proc):

config.lograge.extend_data do |data|
  data[:host] = request.host
  data[:user_id] = current_user.try(:id)
  data[:fwd] = request.remote_ip
end

More like the original implementation, the controller context is implicit to the block. But, at the same time, the way the data is called (using a block instead of a proc) is more clear: by calling extend_data you are extending the data that lograge logs. (I think most Rails developers would be fine with the assumption that the block has access to whatever the controller has access to.)

Any thoughts on these variations? Basically, if you can land on a config API that makes sense and doesn't cause confusion, then the actual implementation is less important. It can be made more or less magical, but it doesn't end up affecting the end-user.

[benlovell]

I'm a fan of the first approach. While it feels a little clumsy having to continually reach into the yielded controller, it seems less magical (although I hate using that term to describe anything Ruby) than it being bound. 👍

[pxlpnk]

I am also a fan of the first approach.

[benlovell]

@smudge Any feels on this still?

[ryanrealscout]

Would love to get this, working now on getting user info into our log lines.

[smudge]

This was needed in order to fix a can't modify frozen String error on the travis builds when installing the rainbow gem. It's unrelated to the rest of my PR. (See rubygems/bundler#5357)

[benlovell]

Yep! Have been hit with this one over and over.

[smudge]

Well, 2 years later, I put another commit together, and the configuration now looks like this:

config.lograge.custom_payload do |controller|
  {
    host: controller.request.host,
    user_id: controller.current_user.try(:id),
    fwd: controller.request.remote_ip
  }
end

I added test coverage for this and also made sure existing tests are all green.

Not 100% satisfied with the underlying implementation, but having an explicit "controller" arg allowed this to move from an underlying instance_eval to just block.call, which is probably a win.

[benlovell]

Thanks for picking this up again! I'm just giving it a quick go over but it looks awesome at first glance.

[troycroz]

Any update on this?

[pxlpnk]

👍