-
Notifications
You must be signed in to change notification settings - Fork 6.9k
[serve] metrics route filters #57604
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
base: master
Are you sure you want to change the base?
Conversation
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.
Code Review
This pull request adds filters to exclude metrics from health check (/-/healthz
) and routes (/-/routes
) endpoints in test utility functions. This ensures that tests for application-level metrics are not affected by these internal endpoints. The changes are correct and achieve the stated goal. I've provided a couple of suggestions to improve code quality by reducing duplication and improving readability.
Signed-off-by: omkar <omkar@anyscale.com>
from ray.serve.generated import serve_pb2, serve_pb2_grpc | ||
from ray.util.state import list_actors | ||
|
||
ALLOWED_ROUTES = ("/-/healthz", "/-/routes") |
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 think EXCLUDED_ROUTES
is more fitting. Also do we need to include /-/routes
?
if any(f'route="{route}"' in line for route in ALLOWED_ROUTES): | ||
continue |
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.
can simplify the expression if we're just checking for /-/healthz
Why are these changes needed?
For tests, currently health check and
routes
endpoint requests are counted. This PR adds a filter to remove these endpoints from metrics aggregation to ensure only valid app level api requests are counted.Related issue number
Checks
git commit -s
) in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.