feat: add health endpoints - #590
Conversation
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds service health endpoints for hosted agents and introduces a readiness signal path from pub/sub consumer subscriptions up to the agent runner.
Changes:
- Add
/livezand/readyzendpoints toAgentRunner.serve()and implement readiness logic based on runner/agent wiring state. - Extend pub/sub subscription registration to return per-consumer “ready” status functions (in addition to closers), and thread this through runners.
- Add/adjust tests to validate route registration return values and health endpoint behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
dapr_agents/workflow/runners/agent.py |
Mounts health/readiness endpoints and checks readiness conditions. |
dapr_agents/workflow/runners/base.py |
Stores/clears pubsub consumer status functions alongside closers. |
dapr_agents/workflow/utils/registration.py |
Changes route registration APIs to return (closers, status_functions). |
dapr_agents/workflow/utils/subscription.py |
Builds status functions per streaming subscription and returns them with closers. |
tests/workflow/test_message_router.py |
Updates tests for new (closers, status_functions) return shape. |
tests/workflow/test_agent_runner.py |
Adds tests covering /livez and /readyz behavior under different states. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| entry_path: str = "/agent/run", | ||
| status_path: str = "/agent/instances/{instance_id}", | ||
| health_check_path: str = "/livez", | ||
| readiness_check_path: str = "/readyz", |
There was a problem hiding this comment.
Should Dapr Agent app health endpoints point to /readyz rather than /livez since it better aligns with Dapr health checks goals (i.e. "can it accept traffic?")
There was a problem hiding this comment.
You've done the right thing here. In k8s lingo it's /readyz and /livez. We should update the default dapr to deprecate /healtz. Keep as-is in this pr :-)
There was a problem hiding this comment.
Since both app health checks and K8s health checks are opt-in, should there also be a flag to determine if these endpoints should be exposed (similar to expose_entry)?
There was a problem hiding this comment.
Yes that would be great. If you can add both an ENV variable + a setting in the agent execution config.
You can see the agent observability config. We should let the priority be agent execution config > env variable > default
There was a problem hiding this comment.
If i'm not misunderstanding something, most CLI flags/K8s annotations aren't automatically made available as env variables in the app? Should we expect the env variable names to follow the CLI flags/K8s annotations (e.g. ENABLE_APP_HEALTH_CHECK, APP_HEALTH_CHECK_PATH)?
There was a problem hiding this comment.
I'd go with ENABLE_APP_HEALTH_CHECK and ENABLE_APP_READY_CHECK.
You are correct that CLI flags and annotations aren't available as env vars.
In the real deployment model you'd either serve them as env vars on the manifest or you'd mount a configmap as env vars with the details.
We shouldn't let the path be overwritten imo.
When running locally you can just run export ENABLE_APP_HEALTH_CHECK in the terminal for it to be available as env var.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Casper Nielsen <whopsec@protonmail.com>
|
@JeffreyJPZ thank you for this one! Can you move your comments to address the copilot review as it's a little confusing to me which belongs where? |
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
CasperGN
left a comment
There was a problem hiding this comment.
A few comments for changes
| self._default_http_paths.add(health_check_path) | ||
|
|
||
| async def _get_health_status() -> dict[str, str]: | ||
| return {"status": "ok"} |
There was a problem hiding this comment.
As done with the _is_ready() func, can we also include a check for the health endpoint so we know that the agent is actually running healthy?
There was a problem hiding this comment.
Would this be the same as _is_ready excluding checking pub/sub consumers and HTTP routes (and shutdown signals)?
There was a problem hiding this comment.
Hm. I think I was more referring to checking the internals of the agent. The agent could in theory be stuck in a workflow or throwing unrecoverable errors while this would still return ok
There was a problem hiding this comment.
Thats fair. My initial thoughts are on some sort of heartbeat mechanism/watcher thread. I'm also looking at the unrecoverable error path in the subscription code, it looks like the consumer thread just logs the error and exits? Should there be self-healing by recreating consumer threads, or should it just report unhealthy and have K8s restart the container?
Some questions building off of my initial thoughts:
- What if the watcher thread goes down? Can it be reactivated somehow (maybe reminders?)
- How can one distinguish between a really long workflow and a "stuck" workflow, if that even is possible? AFAIK there's no "global" timeout for workflows (which I guess would defeat the point of a workflow), but maybe this can be configured on the agent somehow?
There was a problem hiding this comment.
Regarding the subscription, I think it's fine to report unhealthy and restart. We gotta be careful not to interfere with the expected behavior of retries etc as that comes from the retry policies.
@JoshVanL do you have some input here about how we could probe the wf runtime to infer if it's "running healthy" or not?
There was a problem hiding this comment.
Looks like there's only the wait_for_worker_ready method, but that's probably not enough
| entry_path: str = "/agent/run", | ||
| status_path: str = "/agent/instances/{instance_id}", | ||
| health_check_path: str = "/livez", | ||
| readiness_check_path: str = "/readyz", |
There was a problem hiding this comment.
Yes that would be great. If you can add both an ENV variable + a setting in the agent execution config.
You can see the agent observability config. We should let the priority be agent execution config > env variable > default
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
e42cd3d to
a2be6fb
Compare
|
|
||
| [mypy] | ||
| python_version = 3.10 | ||
| python_version = 3.11 |
There was a problem hiding this comment.
Needed to bump the version since StrEnum is only supported in >=3.11, and some tests rely on str(enum) being equal to the string value which only works in >=3.11. I think it was stale anyway since the pyproject file specifies >=3.11.
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
…-agents into health-endpoints
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
| except RuntimeError as e: | ||
| # Fall through if the agent could not be updated but the value is otherwise valid | ||
| logger.debug(f"Agent {self.name}: {e}") | ||
| applied_value = None |
There was a problem hiding this comment.
Was this behaviour intended from the beginning? Had to split the config update in two which leads to a bit of redundant processing because of this, not sure if there's another way.
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
Signed-off-by: Jeffrey Zhang <jeffreyjpizhang@gmail.com>
|
This pull request has been automatically marked as stale because it has not had activity in the last 60 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
keep-alive |
Description
Changes
/livezand/readyzendpoints when agents are hosted as a service.ToolChoiceenum for consistency.Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR closes: #375
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list:
Note: We expect contributors to open a corresponding documentation PR in the dapr/docs repository. As the implementer, you are the best person to document your work! Implementation PRs will not be merged until the documentation PR is opened and ready for review.
TODO:
03-message-router-workflowexample