Skip to content

Conversation

ok-scale
Copy link
Contributor

@ok-scale ok-scale commented Oct 9, 2025

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

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run pre-commit jobs to lint the changes in this PR. (pre-commit setup)
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@ok-scale ok-scale requested a review from a team as a code owner October 9, 2025 17:17
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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>
@ray-gardener ray-gardener bot added serve Ray Serve Related Issue observability Issues related to the Ray Dashboard, Logging, Metrics, Tracing, and/or Profiling labels Oct 9, 2025
@ok-scale ok-scale added the go add ONLY when ready to merge, run all tests label Oct 9, 2025
from ray.serve.generated import serve_pb2, serve_pb2_grpc
from ray.util.state import list_actors

ALLOWED_ROUTES = ("/-/healthz", "/-/routes")
Copy link
Contributor

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?

Comment on lines +719 to +720
if any(f'route="{route}"' in line for route in ALLOWED_ROUTES):
continue
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go add ONLY when ready to merge, run all tests observability Issues related to the Ray Dashboard, Logging, Metrics, Tracing, and/or Profiling serve Ray Serve Related Issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants