-
Notifications
You must be signed in to change notification settings - Fork 2k
emit events for each deployment pull step execution #19339
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
Conversation
CodSpeed Performance ReportMerging #19339 will not alter performanceComparing Summary
|
57afdec to
7e35bde
Compare
prefect.flow-run.pull-steps.executed
znicholasbrown
left a comment
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.
yusss
🙏 looking into one potential improvement here but not in a way that'd change the structure of the event |
| deployment=deployment, | ||
| ) | ||
| raise StepExecutionError(f"Encountered error while running {fqn}") from exc | ||
| _emit_pull_steps_event(serialized_steps, failed_step=None, deployment=deployment) |
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.
I thought we'd emit an event for each step. Is there a reason for emitting them all as one event?
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.
what's the value in emitting one for each? it seemed unnecessary to do more than one
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.
the case @zzstoatzz made to me yesterday is that if one step fails, the rest of them aren't going to be executed anyway, so the maximum amount of data a user would need fits on a single event. I'm assuming the compromise there is observability, but pull steps are logged too. Is there a case for say, automating on pull steps outside of failure scenarios?
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.
I doubt there's a use case for automating outside of a failure, but I think it'd be easier to write an automation against a prefect.pull-step.failed event than writing one against a prefect.pull-step.executed event with failed_step populated in the payload.
I think emitting an event for each pull step is overall more flexible with little downside.
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.
@znicholasbrown the new event grammar in the PR body has been updated accordingly
9cc3b3f to
491e42f
Compare
| deployment=deployment, | ||
| ) | ||
| raise StepExecutionError(f"Encountered error while running {fqn}") from exc | ||
| _emit_pull_steps_event(serialized_steps, failed_step=None, deployment=deployment) |
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.
I doubt there's a use case for automating outside of a failure, but I think it'd be easier to write an automation against a prefect.pull-step.failed event than writing one against a prefect.pull-step.executed event with failed_step populated in the payload.
I think emitting an event for each pull step is overall more flexible with little downside.
prefect.flow-run.pull-steps.executed…rovements - Add deployment as related resource in pull-steps.executed events - Add security documentation explaining pre-templating prevents secret leakage - Add test for secret sanitization in event payloads - Add test for deployment in related resources - Modernize type annotations to Python 3.10+ syntax (list, dict, | None) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…ne serialization this commit addresses alex's PR review comments #1-4: 1. keep all inputs including reserved keywords (like 'requires') in event payload 2. use get_events_client() with checkpoint_every=1 directly instead of emit_event to avoid buffering issues (similar to Runner heartbeat pattern) 3. pass flow_run and logger as params instead of relying on context 4. inline serialization logic directly in run_steps loop instead of separate function also converts all event-related tests to use AssertingEventsClient fixture pattern instead of custom fake clients, which required using the events client as an async context manager. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- remove `from __future__ import annotations` from context.py and test file (not needed for 3.10+) - change `Optional[X]` to `X | None` in context.py for modern type hints - quote forward references since no longer using future annotations - add `pytest.MonkeyPatch` type hints to all monkeypatch fixture parameters - remove unnecessary comment about runner heartbeat events (internal monologue) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
when we removed `from __future__ import annotations`, the return type annotation for get_logger became evaluated at runtime, causing a NameError since LoggerAdapter is only imported inside the function. quote the annotation to defer evaluation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…ne serialization This commit addresses PR review comments #1-4 and implements the requested change from comment #5: 1. Keep reserved keywords (like 'requires') in serialized inputs ✅ 2. Use `get_events_client(checkpoint_every=1)` directly instead of `emit_event` to avoid buffering issues ✅ 3. Pass `flow_run` and `logger` as parameters instead of using context ✅ 4. Inline step serialization logic directly in `run_steps` ✅ 5. Emit **one event per step** instead of one aggregate event ✅ Changes: - Removed `_EnvironmentRunContext` class and tests (over-engineered) - Read `flow_run_id` directly from `os.getenv("PREFECT__FLOW_RUN_ID")` - Changed from single aggregate event to individual events per step: - `prefect.flow-run.pull-step.executed` for each successful step - `prefect.flow-run.pull-step.failed` when a step fails - Updated all tests to expect one event per step - Event payload now contains individual step data (index, qualified_name, step_name, id, inputs) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
… to top Small improvements: - Use `enumerate(steps)` instead of manual `step_index` tracking - Move `from uuid import UUID` to top-level imports instead of deferring it 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
6af755b to
3874fc7
Compare
summary
emits events for each deployment pull step execution, enabling the UI to display exact pull step history for flow runs. one event is emitted per step.
changes
event emission
prefect.flow-run.pull-step.executed- step completed successfullyprefect.flow-run.pull-step.failed- step failedprefect.flow-run.<flow_run_id>{ "index": 0, "qualified_name": "prefect.deployments.steps.run_shell_script", "step_name": "run_shell_script", "id": "optional-step-id", "inputs": {...} }PREFECT__FLOW_RUN_IDis set (deployment runs, not local)get_events_client(checkpoint_every=1)to avoid buffering issuessecurity
{{ prefect.blocks.secret.name }})implementation
_EnvironmentRunContextclass (over-engineered)flow_run_iddirectly fromos.getenv("PREFECT__FLOW_RUN_ID")flow_runandloggeras parameters torun_steps()instead of using contextrun_steps()looptesting
context
UI wants immutable record of executed pull steps since deployment configuration may change after flow run scheduling. emitting one event per step provides granular visibility into the execution timeline.