Reset execution context in jobs and controllers #54663
Conversation
Currently `ActiveSupport::ExecutionContext#[]` is used to set the `:job` and `:controller` attributes, which does not reset the values once the job or controller have completed execution. This can result in that context leaking, resulting in query logs that are misattributed. This leaky behavior can be seen in production (e.g. performing a job via `perform_now`) but is much more noticeable in the test environment where it's not uncommon to run some combination of jobs/controller actions in a single test. This updates both uses of `ActiveSupport::ExecutionContext#[]` to avoid leaking the context and generating inaccurate query logs.
| Unfortunately I don't think that's exactly the proper fix, because there's some expectation that But of course in test that's a bit different. |
I think for In at least in certain job scenarios, the existing behavior leads to plain incorrect results. I wrote up a quick repro script to highlight that: Both queries occur in the As-is though, I'd argue that the existing behavior is often misleading, if not incorrect. I think there's a strong argument for changing the job behavior given how much more common it would be to run into it leaking query tags.
Either way, I would definitely be interested in what you imagine the proper fix might be because I'd love to fix this behavior upstream vs just in our application(s). |
| For completeness sake, here's another repro script but using controllers and middleware highlighting some of what I mentioned above:
Ultimately, the behavior as-is feels inconsistent, and surprising unless you know about these caveats and implementation details. This data can be so helpful in a number of scenarios (incidents, understanding query patterns, debugging, etc), but only if it's accurate, so I'd really love to help make this data more accurate and actionable. |
Yes, but it's not like you can identify the controller before going through the router, so it's a bit innevitable.
I haven't put much thoughts into it yet, I'm just saying that for the case of controllers at least, it's expected that the context remain. If we want to clear it in test environment, I think it's more the job of the test helpers to do it. For jobs specifically, it might make sense to restore it right away. |
That's fair, I've been thinking on that and I think I still disagree with that being the "correct" behavior (and happy to agree to disagree here) given the reasoning above. At least for query logs, I believe that intent changed at some point since it looks like it was behaving as desired before it was merged with (or into?)
I think jobs are definitely more likely to cause issues with attribution given their usage patterns. I'd be happy to split the controller and job changes out into separate PRs if that's helpful. |
Sure, let's go with that. |
Here's the context: #43598 (comment) |
2 participants
Motivation / Background
At GitHub we're spiking out tooling that identifies queries in CI, highlighting them to the PR author, linting for index usage, etc. While building this functionality we discovered that the query logs attributed a significant number of queries to a common job that's run inline in our test suite. After some investigation, we discovered that the cause was due to
:job(and:controller) context values not being cleared after the job or controller had completed execution.The job in question is commonly run in our test suite and we saw a ~4x reduction in the number of queries attributed to it after a small monkey patch to clear
ActiveSupport::ExecutionContext[:job]post perform.The accuracy of this tag is valuable in this testing context since it's highlighting a lot of potential optimizations we can make to our test suite and avoids potential confusion why when queries are attributed incorrectly. This can also impact production in some circumstances, like mistakenly attributing queries to the wrong source (e.g. running a
perform_nowcould incorrectly begin attributing queries to that job instead its parent job, or queries made in a controller post-perform).Detail
Currently
ActiveSupport::ExecutionContext#[]is used to set the:joband:controllerattributes, which does not reset the values once the job or controller have completed execution. This can result in that context leaking, resulting in query logs that are misattributed.This leaky behavior can be seen in production (e.g. performing a job via
perform_now) but is much more noticeable in the test environment where it's not uncommon to run some combination of jobs/controller actions in a single test.This updates both uses of
ActiveSupport::ExecutionContext#[]to avoid leaking the context and generating inaccurate query logs.Additional information
I wasn't 100% positive if there was a better place to put these tests but thought they'd be valuable as small/cheap regression tests.
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]