-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
[serve] Add component logger + basic access logging #23558
Conversation
@simon-mo I probably need to polish this up a bit, but PTAL and leave feedback on the high-level approach. |
High level looks good. Can we clarify the intended usage now for:
|
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 output in PR summary looks awesome
@jiaodong my working definition of "component" here is a physical process, so all logs from controller/proxy/deployment replicas will have their component listed. We could consider including the file name / line number if we want to narrow down where the logs come from in each process. |
@simon-mo @jiaodong to clarify the logging behavior (and I will add this to the documentation):
|
@simon-mo @jiaodong I added the
|
674ab1f
to
9fc29ca
Compare
Why are these changes needed?
Adds a "component logger" to standardize logging across the HTTP proxy, controller, and deployment replicas. This logger has the format:
<level> <timestamp> <component> <component_id> - <message>
component
will be one of {controller, http_proxy, <deployment_name>} andcomponent_id
is the {pid, node_ip, replica_id} for each of these, respectively.For the HTTP proxy and replicas, it also adds a default access log with a message of the form:
<level> <timestamp> <component> <component_id> - <method> <route> <status> <latency_ms>
The format across HTTP and handle calls is standard, so a single log parser can be used to ingest both. For ServeHandle calls, the method will be
HANDLE
, the route will be the actor method, and the status will beOK
orERROR
.One more drive-by small change: this PR removes the status printing in
serve run
because it interleaves with the log messages and is a pretty bad UX. We could consider still monitoring the statuses and printing something if they enterUNHEALTHY
, but the controller should log that anyways.Example
Code:
Logs for a single request:
Related issue number
Checks
scripts/format.sh
to lint the changes in this PR.