-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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 http request latency #32839
Conversation
fd015c9
to
ff3fdc5
Compare
@@ -56,9 +56,9 @@ def verify_metrics(do_assert=False): | |||
"serve_deployment_request_counter", | |||
"serve_deployment_replica_starts", | |||
# histogram | |||
"deployment_processing_latency_ms_bucket", | |||
"deployment_processing_latency_ms_count", | |||
"deployment_processing_latency_ms_sum", |
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.
Do you know why this test was passing before if these metrics didn't exist yet?
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.
Sorry I misunderstood, this is a different metric. Still want to know if it was passing or not running though
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.
It is same metric, the reason it can pass before because we check whether the deployment_processing_latency_ms_sum
string is inside the metrics blob.
For this change, I just want to make the test check more restrict.
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.
Ah makes sense!
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.
Looks good to me pending Edward's suggestions, and it would be good to double check that the test is running in CI
e4bff52
to
265bba0
Compare
Signed-off-by: Sihan Wang <[email protected]>
Signed-off-by: Sihan Wang <[email protected]>
265bba0
to
5d89517
Compare
@@ -279,6 +279,9 @@ The following metrics are exposed by Ray Serve: | |||
* error_code | |||
* method | |||
- The number of non-200 HTTP responses returned by each deployment. | |||
* - ``serve_http_request_latency_ms`` [*] | |||
- * endpoint | |||
- The end-to-end latency of HTTP requests (measured from the Serve HTTP proxy). |
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 end-to-end latency of HTTP requests (measured from the Serve HTTP proxy). | |
- A histogram of end-to-end latencies for HTTP requests (measured from the Serve HTTP proxy). |
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 will stick to the original one since it is following our current words convention.
@edoakes ping for merge! |
Add http request latency metrics. Signed-off-by: Jack He <[email protected]>
Add http request latency metrics. Signed-off-by: Edward Oakes <[email protected]>
Add http request latency metrics.
Add http request latency metrics. Signed-off-by: elliottower <[email protected]>
Add http request latency metrics. Signed-off-by: Jack He <[email protected]>
Why are these changes needed?
Add http request latency metrics.
Related issue number
Closes #32711
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.