CallbackRegistrar: add correlation hook with master record #32
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a new hook registration method register_correlation_hook_with_master_record that allows correlation hooks to access the master record during snapshot creation. The change maintains backward compatibility by keeping the existing register_correlation_hook method and wrapping it internally to ignore the master record parameter.
Changes:
- Added
register_correlation_hook_with_master_recordmethod inCallbackRegistrarfor hooks needing access to master records - Modified internal snapshot hook infrastructure to pass master records to all correlation hooks
- Updated test fixtures to accommodate the new hook signature with an additional master record parameter
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| dp3/common/callback_registrar.py | Added new register_correlation_hook_with_master_record method and modified existing register_correlation_hook to wrap hooks with lambda that ignores master record |
| dp3/snapshots/snapshot_hooks.py | Updated SnapshotCorrelationHookContainer.register() and run() methods to accept and pass master records to hooks, added optional hook_name parameter |
| dp3/snapshots/snapshooter.py | Modified register_correlation_hook() to accept master records and optional hook name, updated make_snapshot() and make_linkless_snapshot() to collect and pass master records |
| tests/test_common/test_snapshots.py | Updated test hook signatures and test calls to include master record parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Adds `register_correlation_hook_with_master_record` hook - identical to existing `register_correlation_hook`, but with an additional master record parameter sent to the callback. Some modules need access to the master record and since it's already loaded during correlation process (snapshot creation), providing it doesn't incur any additional overhead. One such example is the new TS last activity module in the NERD2 system. For backwards compatibility, the new hook type was created instead of modifying the `register_correlation_hook`. Internally, they both share the same implementation.
dbnk0
force-pushed
the
correlation_hook_with_master_record
branch
from
33bb0d3 to
55bebab
Compare
Contributor
Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: Dávid Benko <[email protected]>
Test added by Copilot wasn't working. The issue was a conflict between the new and existing correlation hooks. Solved by adding new separate attributes.
Adds information about `register_correlation_hook_with_master_record` hook to the Modules section.
Contributor
Co-authored-by: Copilot <[email protected]>
xsedla1o
force-pushed
the
correlation_hook_with_master_record
branch
from
9cf3431 to
738790b
Compare
xsedla1o
requested changes
Jan 14, 2026
Collaborator
xsedla1o
left a comment
xsedla1o
left a comment
There was a problem hiding this comment.
Good changes, just a tiny suggestion to improve.
Replaces `lambda` with `functools.wraps` in `CallbackRegistrar`, making `hook_name` argument in `SnapshotCorrelationHookContainer.register` redundant.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds
register_correlation_hook_with_master_recordhook - identical to existingregister_correlation_hook, but with an additional master record parameter sent to the callback. Some modules need access to the master record and since it's already loaded during correlation process (snapshot creation), providing it doesn't incur any additional overhead. One such example is the new TS last activity module in the NERD2 system.For backwards compatibility, the new hook type was created instead of modifying the
register_correlation_hook. Internally, they both share the same implementation.