Fix around_action hook for nested actions

[alitsiya]

We need an ability to trace timing for specific actions inside organizers. There is a around_action hook. But there is an issue with around_action that it's not working for nested Actions that are invoked via proc (example is reduce_if). This is an attempt to fix the issue with around_action. See a spec added in spec/acceptance/around_each_spec.rb:

expect(result[:logger].logs).to eq(
      [
        { :action => TestDoubles::AddsOneAction, :before => 1, :after => 2 },
        { :action => TestDoubles::AddsTwoAction, :before => 2, :after => 4 },
        { :action => TestDoubles::AddsThreeAction, :before => 4, :after => 7 }
      ]
    )

Where actual result is

expect(result[:logger].logs).to eq(
      [
        { :action => TestDoubles::AddsOneAction, :before => 1, :after => 2 },
        { :action => TestDoubles::AddsThreeAction, :before => 2, :after => 7 }
      ]
    )

So it skips entirely nested actions.

This PR is an attempt to fix issue with around_action.

Verified fix using forked version: before:

after:

NOTE: The solution adds unfortunate side effect of having _around_actions key in a context.

[adomokos]

Thank you for the draft PR! One thing that I regretted with LightService was introducing its only dependency: active_support. Some might say active_support should be part of the language. Yeah, sure, but still, it's a dependency that I wanted to avoid.

Can you just use one of the before_action/after_action hooks to send these messages to Datadog? We added those "hooks" so people can extend it with anything they want.

I still have the dream of yanking active_support out of the library, but maybe it's too far-fetched. I am curious to hear what other ppl think.

[adomokos]

Also, around_action is not reliable when actions are nested in reduce_if context (added a spec that shows this case)

Should we fix reduce_if then?

[alitsiya]

We looked at a potential solution for around_action with nested actions and here is what we have to far https://github.com/adomokos/light-service/compare/main...jaredsmithse:jps-around-each-options (also copied to this PR).

[adomokos]

Thank you, I'll take a look at this over the weekend.

[adomokos]

I did take a look. It's a bit hard for me to understand the purpose of this PR.

Your first sentence in the first comment might provide some clue:

Add ability to do instrumentation around actions via ActiveSupport::Notifications without adding unnecessary latency.

How big is this latency? How much better the code is going to get with this PR?

In case you want to use AS Notification, I am good with "open for extension and closed for modification"-kinda change, providing hooks where you can "plug in" your own, custom code.

Can you please create an acceptance test that demonstrates what this PR will add to LS? I am happy to merge it in:

• if it adds value
• fixes bugs
• makes the code cleaner

~ which I think it will do.

Thank you so much and let's get this PR rollin'!

[alitsiya]

Hi @adomokos , thanks for looking into this and sorry for not modifying the description after second iteration. I'm not necessary advocating for adding modifications for a 'hook up', since there is an around_action. It's an attempt to fix a bug where nested around_actions are not invoked. So I updated the description now from ActiveSupport::Notifications to a bug fix.

I did take a look. It's a bit hard for me to understand the purpose of this PR.

Your first sentence in the first comment might provide some clue:

Add ability to do instrumentation around actions via ActiveSupport::Notifications without adding unnecessary latency.

How big is this latency? How much better the code is going to get with this PR?

It's a pure guess. I haven't measured the impact.

In case you want to use AS Notification, I am good with "open for extension and closed for modification"-kinda change, providing hooks where you can "plug in" your own, custom code.

Can you please create an acceptance test that demonstrates what this PR will add to LS? I am happy to merge it in:

• if it adds value
• fixes bugs
• makes the code cleaner

~ which I think it will do.

Thank you so much and let's get this PR rollin'!

[adomokos]

Thank you for your work/time investment into fixing this! I am looking forward to seeing this PR completed!

[adomokos]

Thank you, I'll take a look at this shortly.

[alitsiya]

Thanks! Just tested the change, attached results to the description.

[adomokos]

@alitsiya - can you please get the CI build green? It seems rubocop needs a bit of TLC.

[adomokos]

@alitsiya - what do you think about this?

It passes all the tests, but Rubocop is still failing.

We have current_action as an attribute on the Context, around_actions is similar to that. Is this the best solution? I don't think so, but I'd prefer this over having a special key in the Context that the developer (the library user) is exposed to.

In my solution I am not a fan of this line:

if context.instance_of?(Context) and context.around_actions.respond_to?(:call)

If you have ideas to make this more elegant, I am all for that.

Let me know what you think.

[alitsiya]

This looks great! Thanks for the help! Cherry-picked your suggestions!

[adomokos]

@alitsiya - this is ready to be merge. Can you please fix rubocop? Thank you!

[adomokos]

Thank you for working on this @alitsiya! 💯