[Bugfix] Revert to aioprometheus to avoid 307 redirect #4511

robertgshaw2-neuralmagic · 2024-05-01T02:54:09Z

Reverts prometheus logging from prometheus_client back to aioprometheus to avoid 307 Temporary Redirect

At current, we get:

INFO:     Started server process [1620866]
INFO:     Waiting for application startup.
INFO:     Application startup complete.
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
INFO:     127.0.0.1:34444 - "GET /metrics HTTP/1.1" 307 Temporary Redirect
INFO:     127.0.0.1:34444 - "GET /metrics/ HTTP/1.1" 200 OK

Note: this PR breaks some existing functionality

I needed to remove logging cache_info
Some of the metrics exposed by default for prometheus_client are gone

I also tweaked the name of a label (nit from prior PR). We did not publish a release with that label (merged over the weekend), so I think this is okay

BEFORE SUBMITTING, PLEASE READ THE CHECKLIST BELOW AND FILL IN THE DESCRIPTION ABOVE

PR Checklist (Click to Expand)

Thank you for your contribution to vLLM! Before submitting the pull request, please ensure the PR meets the following criteria. This helps vLLM maintain the code quality and improve the efficiency of the review process.

PR Title and Classification

Only specific types of PRs will be reviewed. The PR title is prefixed appropriately to indicate the type of change. Please use one of the following:

[Bugfix] for bug fixes.
[CI/Build] for build or continuous integration improvements.
[Doc] for documentation fixes and improvements.
[Model] for adding a new model or improving an existing model. Model name should appear in the title.
[Frontend] For changes on the vLLM frontend (e.g., OpenAI API server, LLM class, etc.)
[Kernel] for changes affecting CUDA kernels or other compute kernels.
[Core] for changes in the core vLLM logic (e.g., LLMEngine, AsyncLLMEngine, Scheduler, etc.)
[Hardware][Vendor] for hardware-specific changes. Vendor name should appear in the prefix (e.g., [Hardware][AMD]).
[Misc] for PRs that do not fit the above categories. Please use this sparingly.

Note: If the PR spans more than one category, please include all relevant prefixes.

Code Quality

The PR need to meet the following code quality standards:

We adhere to Google Python style guide and Google C++ style guide.
Pass all linter checks. Please use format.sh to format your code.
The code need to be well-documented to ensure future contributors can easily understand the code.
Include sufficient tests to ensure the project to stay correct and robust. This includes both unit tests and integration tests.
Please add documentation to docs/source/ if the PR modifies the user-facing behaviors of vLLM. It helps vLLM user understand and utilize the new features or changes.

Notes for Large Changes

Please keep the changes as concise as possible. For major architectural changes (>500 LOC excluding kernel/data/config/test), we would expect a GitHub issue (RFC) discussing the technical design and justification. Otherwise, we will tag it with rfc-required and might not go through the PR.

What to Expect for the Reviews

The goal of the vLLM team is to be a transparent reviewing machine. We would like to make the review process transparent and efficient and make sure no contributor feel confused or frustrated. However, the vLLM team is small, so we need to prioritize some PRs over others. Here is what you can expect from the review process:

After the PR is submitted, the PR will be assigned to a reviewer. Every reviewer will pick up the PRs based on their expertise and availability.
After the PR is assigned, the reviewer will provide status update every 2-3 days. If the PR is not reviewed within 7 days, please feel free to ping the reviewer or the vLLM team.
After the review, the reviewer will put an action-required label on the PR if there are changes required. The contributor should address the comments and ping the reviewer to re-review the PR.
Please respond to all comments within a reasonable time frame. If a comment isn't clear or you disagree with a suggestion, feel free to ask for clarification or discuss the suggestion.

Thank You

Finally, thank you for taking the time to read these guidelines and for your interest in contributing to vLLM. Your contributions make vLLM a great tool for everyone!

robertgshaw2-neuralmagic · 2024-05-01T02:55:08Z

vllm/engine/metrics.py

 # begin-metrics-definitions
 class Metrics:
    labelname_finish_reason = "finished_reason"

-    def __init__(self, labelnames: List[str], max_model_len: int):
-        # Unregister any existing vLLM collectors


This was a hack to enable our CI to pass before

REGISTRY is a global value. We were touching internal state since our CI runs LLM twice

I just updated the CI to run with --forked which obviate the need for this hack

We had problems using --forked before due to cuda issue (and we removed it). #3631

Seems like metrics test also use the vllm instances, and I am a little concern it will introduce the similar issue again. Why don't we just keep this hack instead?

As long as the test does not import torch, —forked is fine

IIUC, the vllm runner should import torch? https://github.com/vllm-project/vllm/blob/main/tests/metrics/test_metrics.py

robertgshaw2-neuralmagic · 2024-05-01T02:55:35Z

vllm/engine/metrics.py

@@ -15,72 +14,51 @@

 logger = init_logger(__name__)

-disable_created_metrics()


I do not understand what this does / why it was there before, but the equivalent does not seem to exist for aioprometheus

https://github.com/prometheus/client_python/blob/master/prometheus_client/metrics.py#L74

https://prometheus.github.io/client_python/instrumenting/#disabling-_created-metrics

robertgshaw2-neuralmagic · 2024-05-01T02:58:22Z

vllm/engine/metrics.py

-                REGISTRY.unregister(collector)
-
-        # Config Information
-        self.info_cache_config = Info(


This had to be removed since Info does not exist in aioprometheus AFAIK

rkooo567

LGTM. Just one comment related to --forked usage..

rkooo567 · 2024-05-01T05:12:51Z

vllm/engine/metrics.py

 # begin-metrics-definitions
 class Metrics:
    labelname_finish_reason = "finished_reason"

-    def __init__(self, labelnames: List[str], max_model_len: int):
-        # Unregister any existing vLLM collectors


We had problems using --forked before due to cuda issue (and we removed it). #3631

Seems like metrics test also use the vllm instances, and I am a little concern it will introduce the similar issue again. Why don't we just keep this hack instead?

hmellor · 2024-05-01T08:11:53Z

The redirect, while annoying, was technically harmless.

If possible I'd like to keep using prometheus_client as it's still actively maintained. Could we consider alternative approaches before reverting and losing some of the functionality such as the Info metric?

As mentioned in prometheus/client_python#1016, the issue is that prometheus_client mounts a sub-application whereas aioprometheus adds a route for metrics.

One simple solution that works is to change the path_regex from '^/metrics/(?P<path>.*)$' to '^/metrics(?P<path>.*)$':

import re
from starlette.routing import Mount

...

# Add prometheus asgi middleware to route /metrics requests
route = Mount("/metrics", make_asgi_app())
route.path_regex = re.compile('^/metrics(?P<path>.*)$')
app.routes.append(route)

This allows any path that starts with /metrics (including /metrics) to match the metrics app.

hmellor · 2024-05-01T10:08:59Z

(N.B. I've edited my previous comment so it you're only looking at this via email you won't have the latest update)

robertgshaw2-neuralmagic · 2024-05-01T11:38:52Z

@hmellor is there any performance implications to matching a regex?

Forgive me, my understanding of FastAPI is limited

hmellor · 2024-05-01T12:07:54Z

is there any performance implications to matching a regex?

I don't think so.

Forgive me, my understanding of FastAPI is limited

I'll explain my solution a little more, I'm also learning as I go. app.mount calls the following method in starlette (source):

    def mount(
        self, path: str, app: ASGIApp, name: str | None = None
    ) -> None:  # pragma: nocover
        route = Mount(path, app=app, name=name)
        self.routes.append(route)

So, I am bringing the creation and appending of route into api_server.py. No real changes so far.

The Mount class has an attribute path_regex (source) which it generates by calling compile_path (source). When the path is /metrics, the generated path_regex is ^/metrics/(?P<path>.*)$.

This regex is used in Mount.matches (source) to determine if the queried path belongs to this app. Therefore, the URL must contain /metrics/ to match, meaning that /metrics won't match and results in the reported redirect error.

By changing the path_regex to ^/metrics(?P<path>.*)$ we allow /metrics to match, and the redirect error is gone.

In conclusion, this change is only to the value of a regex pattern and does not modify the execution of any functions in fastapi or starlette, so the performance should be unchanged.

robertgshaw2-neuralmagic · 2024-05-01T12:11:05Z

Okay great, this approach works then

robertgshaw2-neuralmagic · 2024-05-01T12:11:20Z

Ill make a PR today

robertgshaw2-neuralmagic added 2 commits May 1, 2024 02:32

fixed /metrics redirect by reverting to aioprometheus

8495919

removed registry

1c73739

robertgshaw2-neuralmagic commented May 1, 2024

View reviewed changes

./format.sh

78b26e0

robertgshaw2-neuralmagic requested review from hmellor, simon-mo and ywang96 May 1, 2024 03:00

robertgshaw2-neuralmagic mentioned this pull request May 1, 2024

v0.4.2 Release Tracker #4505

Closed

simon-mo approved these changes May 1, 2024

View reviewed changes

rkooo567 reviewed May 1, 2024

View reviewed changes

hmellor mentioned this pull request May 1, 2024

Port metrics from aioprometheus to prometheus_client #2730

Merged

robertgshaw2-neuralmagic closed this May 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bugfix] Revert to aioprometheus to avoid 307 redirect #4511

[Bugfix] Revert to aioprometheus to avoid 307 redirect #4511

robertgshaw2-neuralmagic commented May 1, 2024 •

edited

Loading

robertgshaw2-neuralmagic May 1, 2024

rkooo567 May 1, 2024

robertgshaw2-neuralmagic May 1, 2024

rkooo567 May 1, 2024

robertgshaw2-neuralmagic May 1, 2024 •

edited

Loading

robertgshaw2-neuralmagic May 1, 2024

hmellor May 1, 2024

robertgshaw2-neuralmagic May 1, 2024

rkooo567 left a comment

rkooo567 May 1, 2024

hmellor commented May 1, 2024 •

edited

Loading

hmellor commented May 1, 2024

robertgshaw2-neuralmagic commented May 1, 2024

hmellor commented May 1, 2024 •

edited

Loading

robertgshaw2-neuralmagic commented May 1, 2024

robertgshaw2-neuralmagic commented May 1, 2024 •

edited

Loading

		@@ -15,72 +14,51 @@

		logger = init_logger(__name__)

		disable_created_metrics()

[Bugfix] Revert to aioprometheus to avoid 307 redirect #4511

[Bugfix] Revert to aioprometheus to avoid 307 redirect #4511

Conversation

robertgshaw2-neuralmagic commented May 1, 2024 • edited Loading

PR Title and Classification

Code Quality

Notes for Large Changes

What to Expect for the Reviews

Thank You

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertgshaw2-neuralmagic May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkooo567 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmellor commented May 1, 2024 • edited Loading

hmellor commented May 1, 2024

robertgshaw2-neuralmagic commented May 1, 2024

hmellor commented May 1, 2024 • edited Loading

robertgshaw2-neuralmagic commented May 1, 2024

robertgshaw2-neuralmagic commented May 1, 2024 • edited Loading

robertgshaw2-neuralmagic commented May 1, 2024 •

edited

Loading

robertgshaw2-neuralmagic May 1, 2024 •

edited

Loading

hmellor commented May 1, 2024 •

edited

Loading

hmellor commented May 1, 2024 •

edited

Loading

robertgshaw2-neuralmagic commented May 1, 2024 •

edited

Loading