Add db_runtime instrumentation for ActiveGraph - #1741
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds ChangesDatabase runtime tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Neo4jBolt
participant RuntimeRegistry
participant RailsController
participant ActionPayload
Neo4jBolt->>RuntimeRegistry: report Bolt request duration
RailsController->>RuntimeRegistry: reset before process_action
RailsController->>RailsController: execute action
RailsController->>RuntimeRegistry: reset accumulated runtime
RuntimeRegistry->>ActionPayload: provide db_runtime
RailsController->>ActionPayload: append db_runtime
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
spec/unit/railties/controller_runtime_spec.rb (1)
31-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest that it adds to existing
db_runtime.If you update
append_info_to_payloadto accumulatedb_runtime(so as not to overwrite metrics from other ORMs), consider updating this test or adding another context to verify the accumulation.💡 Proposed test addition
payload = {} controller.send(:append_info_to_payload, payload) expect(payload[:db_runtime]).to eq(250.0) end + + it 'adds to existing database runtime in the payload' do + ActiveGraph::RuntimeRegistry.call('neo4j.core.bolt.request', 1.0, 1.25, 'id', {}) + payload = { db_runtime: 100.0 } + + controller.send(:append_info_to_payload, payload) + + expect(payload[:db_runtime]).to eq(350.0) + end end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/unit/railties/controller_runtime_spec.rb` around lines 31 - 38, Update the test around append_info_to_payload to initialize payload[:db_runtime] with an existing runtime value, invoke the method, and assert that the result includes both the existing value and the newly recorded 250.0 runtime. Preserve the current assertion for an empty payload as a separate case if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/active_graph/railties/controller_runtime.rb`:
- Around line 15-19: Update append_info_to_payload so
ActiveGraph::RuntimeRegistry.reset is added to any existing payload[:db_runtime]
value rather than replacing it, preserving accumulated runtime from ActiveRecord
or other ORM/ODM integrations.
---
Nitpick comments:
In `@spec/unit/railties/controller_runtime_spec.rb`:
- Around line 31-38: Update the test around append_info_to_payload to initialize
payload[:db_runtime] with an existing runtime value, invoke the method, and
assert that the result includes both the existing value and the newly recorded
250.0 runtime. Preserve the current assertion for an empty payload as a separate
case if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b5690518-79b6-4533-901e-2d32b2222247
📒 Files selected for processing (5)
lib/active_graph/railtie.rblib/active_graph/railties/controller_runtime.rblib/active_graph/runtime_registry.rbspec/unit/railties/controller_runtime_spec.rbspec/unit/runtime_registry_spec.rb
c28c7f9 to
1e70270
Compare
Fixes #1291
This pull introduces/changes:
ActiveSupport::NotificationsActiveGraph::RuntimeRegistryto accumulate Neo4j Bolt request runtimepayload[:db_runtime]for Rails controller actionsActiveSupport.on_load(:action_controller)Summary
Adds ActiveGraph database runtime instrumentation to expose
db_runtimein Rails controller payloads.This brings ActiveGraph closer to ActiveRecord's database runtime instrumentation behavior.
Changes
ActiveGraph::RuntimeRegistryto accumulate Neo4j Bolt request runtime.neo4j.core.bolt.requestusingActiveSupport::Notifications.monotonic_subscribe.ActiveSupport.on_load(:action_controller).payload[:db_runtime].Testing
Local test execution is currently pending Ruby/Bundler setup on the development environment.
Summary by CodeRabbit
db_runtime), including support for incrementing when a value is already present.