[Bugfix] Move ScoutApm instrumentation into an initializer#2442
[Bugfix] Move ScoutApm instrumentation into an initializer#2442tonyta wants to merge 1 commit intoshakacode:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMoved ScoutApm instrumentation from gem-load-time guards into a Rails initializer Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AppServer as Rails App Server
participant NodeRenderer as Node Renderer
participant FS as Filesystem
participant LockService as Bundle Lock
Client->>AppServer: POST /upload-assets (with bundle)
AppServer->>NodeRenderer: stream upload -> writes to uploads/<uuid>/
NodeRenderer->>FS: write to uploads/<uuid>/bundle.tmp
NodeRenderer->>LockService: acquire lock(bundle)
NodeRenderer->>FS: move/copy from uploads/<uuid>/ to bundle-dir
LockService-->>NodeRenderer: release lock(bundle)
Client->>AppServer: POST /render (concurrent)
AppServer->>LockService: acquire lock(bundle)
LockService-->>AppServer: lock granted
AppServer->>FS: read from bundle-dir (safe, isolated)
AppServer->>NodeRenderer: render using bundle
NodeRenderer->>FS: read assets from bundle-dir
AppServer->>LockService: release lock(bundle)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tonyta
force-pushed
the
mv-scout-apm-instrumentation-to-initializer
branch
2 times, most recently
from
788e38b to
0cef7e6
Compare
Greptile SummaryThis PR fixes a load-order bug between ReactOnRails and ScoutApm by moving APM instrumentation from gem load time into a Rails initializer that runs after
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Rails as Rails Boot
participant Scout as ScoutApm Railtie
participant RoR as ReactOnRails Engine
Rails->>Scout: Run scout_apm.start initializer
Scout->>Scout: Initialize ScoutApm agent and config
Rails->>RoR: Run react_on_rails.scout_apm_instrumentation
RoR->>RoR: Check defined?(ScoutApm)
alt ScoutApm is loaded
RoR->>RoR: Instrument Helper methods
RoR->>RoR: Instrument RubyEmbeddedJavaScript methods
else ScoutApm not loaded
RoR->>RoR: Skip instrumentation
end
Last reviewed commit: 0cef7e6 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
tonyta
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
tonyta
force-pushed
the
mv-scout-apm-instrumentation-to-initializer
branch
from
0cef7e6 to
43fe65d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
react_on_rails/spec/react_on_rails/engine_spec.rb (2)
210-228: "When ScoutApm is not defined" context is fragile withouthide_const.These tests rely implicitly on
ScoutApmbeing absent from the test environment. Ifscout_apmis ever added as a test dependency (or is transiently defined in the test process),defined?(ScoutApm)will be truthy and the initializer will run instrumentation, flipping all three assertions from green to red without any obvious cause.Use
hide_constto make the precondition explicit and robust:🛡️ Suggested fix
context "when ScoutApm is not defined" do + before { hide_const("ScoutApm") } + it "does not instrument Helper#react_component" do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/spec/react_on_rails/engine_spec.rb` around lines 210 - 228, The "when ScoutApm is not defined" examples assume ScoutApm is absent; make this explicit by using hide_const("ScoutApm") in that context so the tests are robust; update the context block (surrounding initializer.run and expectations for mock_helper and mock_rb_embedded_js) to call hide_const('ScoutApm') (or equivalent RSpec helper) before running initializer.run to ensure ScoutApm is undefined for the three specs that check react_component, react_component_hash, and exec_server_render_js instrumentation.
170-170: Consider using exact name match in the subject finder.
name.include?("scout_apm")would inadvertently match a second initializer if one were ever added (e.g.,react_on_rails.scout_apm_something_else). A precise match is more defensive:♻️ Suggested fix
- subject(:initializer) { described_class.initializers.find { |i| i.name.include?("scout_apm") } } + subject(:initializer) { described_class.initializers.find { |i| i.name == "react_on_rails.scout_apm_instrumentation" } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/spec/react_on_rails/engine_spec.rb` at line 170, The subject finder uses a substring match which can accidentally select multiple initializers; update the finder that defines subject(:initializer) to locate the initializer by exact name against described_class.initializers (replace the .name.include?("scout_apm") check with an exact equality check against the known initializer name, e.g., .name == "react_on_rails.scout_apm") so only the intended initializer is selected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@react_on_rails/spec/react_on_rails/engine_spec.rb`:
- Around line 210-228: The "when ScoutApm is not defined" examples assume
ScoutApm is absent; make this explicit by using hide_const("ScoutApm") in that
context so the tests are robust; update the context block (surrounding
initializer.run and expectations for mock_helper and mock_rb_embedded_js) to
call hide_const('ScoutApm') (or equivalent RSpec helper) before running
initializer.run to ensure ScoutApm is undefined for the three specs that check
react_component, react_component_hash, and exec_server_render_js
instrumentation.
- Line 170: The subject finder uses a substring match which can accidentally
select multiple initializers; update the finder that defines
subject(:initializer) to locate the initializer by exact name against
described_class.initializers (replace the .name.include?("scout_apm") check with
an exact equality check against the known initializer name, e.g., .name ==
"react_on_rails.scout_apm") so only the intended initializer is selected.
43fe65d to
20a0ba4
Compare
|
Changes since initial push (via amend and force-push):
|
|
@claude review this PR |
|
Claude Code is working… I'll analyze this and get back to you. |
|
@justin808 Claude Code stalled while attempting to pull my branch from It issue seems to be that I can try pushing my feature branch to this repo instead and retrying the Github Action or recreate this PR with the feature branch in this repo instead. Update: I was not able to push my feature branch to You can push the branch to this repo using following: Note: This will allow Claude Code review the same commit but technically the reviewed branch is distinct from the source branch to be merged. |
|
@tonyta Looks good. Can you resolve the changelog conflict, or let me know and I'll create a new branch and do it. |
|
@claude review this PR |
|
Claude finished @justin808's task in 1m 37s —— View job PR Review: Move ScoutApm Instrumentation to InitializerTasks:
This is a clean, well-motivated fix. The root cause analysis is solid and the approach is idiomatic for Rails engines. Here are my findings: SummaryVerdict: Approve with minor notes. The implementation is correct, the tests are comprehensive, and the most important CodeRabbit nitpick ( Implementation (
|
|
@justin808 I've rebased and force-pushed to my source branch. You can verify the rebase was correctly applied with the following: Update: I was able to trigger Github to recognize the new commit be rewriting the commit and force-pushing again. We are ready to merge! Thanks for the review! (previously)
|
tonyta
force-pushed
the
mv-scout-apm-instrumentation-to-initializer
branch
from
e974c8d to
9fcba5a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
react_on_rails/lib/react_on_rails/engine.rb (1)
87-87: Optional: unpin the ScoutApm version reference in the comment.The comment links directly to the
v6.1.0tag. As ScoutApm releases new versions, this link will silently point to old code. Linking to themainbranch (or omitting the tag entirely) would keep the reference evergreen.✏️ Suggested update
- # https://github.com/scoutapp/scout_apm_ruby/blob/v6.1.0/lib/scout_apm.rb#L221 + # https://github.com/scoutapp/scout_apm_ruby/blob/main/lib/scout_apm.rb🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/lib/react_on_rails/engine.rb` at line 87, The comment currently references a version-tagged ScoutApm URL (v6.1.0); update the comment in react_on_rails::engine.rb (the line with the ScoutApm link) to use an evergreen link (e.g., the repository path without the tag or pointing to the main branch) or remove the tag portion so the reference stays current as ScoutApm releases new versions.react_on_rails/spec/react_on_rails/engine_spec.rb (1)
170-170: Optional: prefer an exact name match in the subject finder.The substring match works today but would silently latch onto a different initializer if another
"scout_apm"-named initializer were added. An exact equality check is both more precise and more self-documenting.✏️ Suggested update
- subject(:initializer) { described_class.initializers.find { |i| i.name.include?("scout_apm") } } + subject(:initializer) { described_class.initializers.find { |i| i.name == "react_on_rails.scout_apm_instrumentation" } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/spec/react_on_rails/engine_spec.rb` at line 170, The subject finder uses a substring match on initializer names which can collide; update the finder that locates the initializer (subject(:initializer) { described_class.initializers.find { |i| i.name.include?("scout_apm") } }) to use exact equality on the initializer name (compare i.name == "scout_apm" or equivalent) so it selects the precise initializer by name rather than any name that merely contains that substring.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@react_on_rails/lib/react_on_rails/engine.rb`:
- Line 87: The comment currently references a version-tagged ScoutApm URL
(v6.1.0); update the comment in react_on_rails::engine.rb (the line with the
ScoutApm link) to use an evergreen link (e.g., the repository path without the
tag or pointing to the main branch) or remove the tag portion so the reference
stays current as ScoutApm releases new versions.
In `@react_on_rails/spec/react_on_rails/engine_spec.rb`:
- Line 170: The subject finder uses a substring match on initializer names which
can collide; update the finder that locates the initializer
(subject(:initializer) { described_class.initializers.find { |i|
i.name.include?("scout_apm") } }) to use exact equality on the initializer name
(compare i.name == "scout_apm" or equivalent) so it selects the precise
initializer by name rather than any name that merely contains that substring.
9fcba5a to
3c18116
Compare
|
@justin808 I just rebase again due to the recurring merge conflict on CHANGELOG.md. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
30-36:⚠️ Potential issue | 🟡 MinorScoutApm entry is misplaced in the
#### Prosection.The ScoutApm instrumentation fix lives entirely in the core gem (
react_on_rails/lib/react_on_rails/engine.rb,helper.rb,server_rendering_pool/ruby_embedded_java_script.rb) — not inreact_on_rails_pro/. It should be in a non-Pro#### Fixedsection under### [Unreleased].🔧 Suggested placement
### [Unreleased] Changes since the last non-beta release. +#### Fixed + +- **Fixed ScoutApm instrumentation depending on Gemfile ordering**. ... [PR 2442](https://github.com/shakacode/react_on_rails/pull/2442) by [tonyta](https://github.com/tonyta). + #### Pro ##### Fixed -- **Fixed ScoutApm instrumentation depending on Gemfile ordering**. ... [PR 2442](...) by [tonyta](...). - **Fixed node renderer upload race condition ...** [PR 2456](...) ... - **Fixed node renderer race condition ...** [PR 2464](...) ...Based on learnings, "Pro-only changes go in the
#### Prosection of/CHANGELOG.md; shared changes go in the main section only without duplication."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 30 - 36, Move the ScoutApm instrumentation bullet out of the "#### Pro" section and into the main "#### Fixed" subsection under "### [Unreleased]" (so it appears as a non‑Pro shared change). Update the changelog entry text if needed to remove any "Pro" wording and ensure it references the core files altered (react_on_rails/lib/react_on_rails/engine.rb, helper.rb, and server_rendering_pool/ruby_embedded_java_script.rb) to make clear this fix lives in the core gem rather than react_on_rails_pro.
🧹 Nitpick comments (1)
react_on_rails/spec/react_on_rails/engine_spec.rb (1)
169-175: Consider guarding against anilsubject.
find { |i| i.name.include?("scout_apm") }returnsnilif no matching initializer is found. Failures would then surface asNoMethodError: undefined method 'name' for nilrather than a clear expectation failure. Adding an explicit non-nil assertion improves debuggability.♻️ Proposed fix
subject(:initializer) { described_class.initializers.find { |i| i.name.include?("scout_apm") } } it "defines a named Rails initializer to run after scout_apm.start" do + expect(initializer).not_to be_nil, "Expected to find a scout_apm initializer in ReactOnRails::Engine" expect(initializer.name).to eq "react_on_rails.scout_apm_instrumentation" expect(initializer.after).to eq "scout_apm.start" end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/spec/react_on_rails/engine_spec.rb` around lines 169 - 175, The spec's subject(:initializer) can be nil if no matching initializer is found, causing a NoMethodError; add an explicit non-nil assertion (e.g., expect(initializer).not_to be_nil) immediately after subject(:initializer) is defined and before the existing expectations that access initializer.name and initializer.after so the test fails with a clear assertion message rather than a NoMethodError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 30-36: Move the ScoutApm instrumentation bullet out of the "####
Pro" section and into the main "#### Fixed" subsection under "### [Unreleased]"
(so it appears as a non‑Pro shared change). Update the changelog entry text if
needed to remove any "Pro" wording and ensure it references the core files
altered (react_on_rails/lib/react_on_rails/engine.rb, helper.rb, and
server_rendering_pool/ruby_embedded_java_script.rb) to make clear this fix lives
in the core gem rather than react_on_rails_pro.
---
Nitpick comments:
In `@react_on_rails/spec/react_on_rails/engine_spec.rb`:
- Around line 169-175: The spec's subject(:initializer) can be nil if no
matching initializer is found, causing a NoMethodError; add an explicit non-nil
assertion (e.g., expect(initializer).not_to be_nil) immediately after
subject(:initializer) is defined and before the existing expectations that
access initializer.name and initializer.after so the test fails with a clear
assertion message rather than a NoMethodError.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.mdreact_on_rails/lib/react_on_rails/engine.rbreact_on_rails/lib/react_on_rails/helper.rbreact_on_rails/lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rbreact_on_rails/spec/react_on_rails/engine_spec.rbscript/ci-changes-detector
💤 Files with no reviewable changes (2)
- react_on_rails/lib/react_on_rails/helper.rb
- react_on_rails/lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- react_on_rails/lib/react_on_rails/engine.rb
tonyta
force-pushed
the
mv-scout-apm-instrumentation-to-initializer
branch
from
3c18116 to
1b1ddd2
Compare
edd6330 added ScoutApm instrumentation to methods on the following: - ReactOnRails::Helper - ReactOnRails::ServerRenderingPool::RubyEmbeddedJavaScript Previously, this instrumentation was installed at gem load time using `defined?(ScoutApm)` guards. This caused two problems: 1. Gem ordering dependency: if scout_apm appeared after react_on_rails in the Gemfile, ScoutApm was not yet defined when react_on_rails loaded, so instrumentation was silently skipped. 2. Noisy log output: if scout_apm appeared before react_on_rails, instrumentation was installed before ScoutApm was initialized (via its "scout_apm.start" Railtie initializer), causing INFO-level messages to be written to stdout during boot. This commit moves the instrumentation into ReactOnRails::Engine with an initializer explicitly configured to run after ScoutApm initializes ("scout_apm.start"). This ensures instrumentation is properly installed regardless of gem ordering and only after ScoutApm is fully configured.
tonyta
force-pushed
the
mv-scout-apm-instrumentation-to-initializer
branch
from
1b1ddd2 to
5f59369
Compare
|
@justin808 I appreciate your attention here and hope we can merge this soon as I keep having to rebase to fix merge conflicts on CHANGELOG.md 😅 |
2 participants
Summary
This PR fixes the load-order bug between ReactOnRails and ScoutApm that causes noisy output or prevents ReactOnRails's methods to be properly instrumented. The fix is to move instrumentation from gem load to Rails initializers.
Before:
After:
Details
edd6330 added ScoutApm instrumentation to methods on the following:
Previously, this instrumentation was installed at gem load time using
defined?(ScoutApm)guards. This caused two problems:Gem ordering dependency: if scout_apm appeared after react_on_rails in the Gemfile, ScoutApm was not yet defined when react_on_rails loaded, so instrumentation was silently skipped.
Noisy log output: if scout_apm appeared before react_on_rails, instrumentation was installed before ScoutApm was initialized (via its "scout_apm.start" Railtie initializer), causing INFO-level messages to be written to stdout during boot.
This commit moves the instrumentation into ReactOnRails::Engine with an initializer explicitly configured to run after ScoutApm initializes ("scout_apm.start"). This ensures instrumentation is properly installed regardless of gem ordering and only after ScoutApm is fully configured.
Reproduction
A minimal reproduction of the bug can be found here: https://github.com/tonyta/react-on-rails-scout-apm-repro
It contains all the instructions to recreate the original issue and prove the fix.
Summary by CodeRabbit
Bug Fixes
Tests
Chores