-
Notifications
You must be signed in to change notification settings - Fork 19
Otel: emit plugin operation hooks during replay #496
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -278,6 +278,7 @@ def __init__( | |
| self._replay_status: ReplayStatus = replay_status | ||
| self._replay_status_lock: Lock = Lock() | ||
| self._visited_operations: set[str] = set() | ||
| self._replayed_operation_hooks: set[str] = set() | ||
|
|
||
| @property | ||
| def operations(self) -> dict[str, Operation]: | ||
|
|
@@ -444,11 +445,30 @@ def get_checkpoint_result(self, checkpoint_id: str) -> CheckpointedResult: | |
| """ | ||
| # checking status are deliberately under a lighter non-serialized lock | ||
| with self._operations_lock: | ||
| if checkpoint := self._operations.get(checkpoint_id): | ||
| return CheckpointedResult.create_from_operation(checkpoint) | ||
| checkpoint = self._operations.get(checkpoint_id) | ||
|
|
||
| if checkpoint: | ||
| self._emit_operation_replay_hooks(checkpoint) | ||
| return CheckpointedResult.create_from_operation(checkpoint) | ||
|
|
||
| return CHECKPOINT_NOT_FOUND | ||
|
|
||
| def _emit_operation_replay_hooks(self, operation: Operation) -> None: | ||
| """Emit operation hooks once for each checkpointed operation during replay.""" | ||
| if operation.operation_type is OperationType.EXECUTION: | ||
| return | ||
| if operation.status is OperationStatus.READY: | ||
| return | ||
|
|
||
| with self._replay_status_lock: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is still using the state level replay status, which will be removed in #488, do we want to keep this ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be updated to use the new per context replay status |
||
| if self._replay_status is not ReplayStatus.REPLAY: | ||
| return | ||
| if operation.operation_id in self._replayed_operation_hooks: | ||
| return | ||
| self._replayed_operation_hooks.add(operation.operation_id) | ||
|
Comment on lines
+466
to
+468
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a little confused. What is this _replayed_operation_hooks variable used for? Wouldn't this reset on each invocation?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| self._plugin_executor.on_operation_replay(operation) | ||
|
|
||
| def create_checkpoint( | ||
| self, | ||
| operation_update: OperationUpdate | None = None, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently, is this being executed on every single replay on every invocation?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
called once per operation per invocation if the operation's context is replayed. If the context is completed and the result is cached, its child operations will not be replayed and thus this will not be called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this affect the Otel Plugin behaviour?