henry3260 commented on code in PR #64523:
URL: https://github.com/apache/airflow/pull/64523#discussion_r3068310543
##########
airflow-core/src/airflow/api_fastapi/common/http_access_log.py:
##########
@@ -23,13 +23,87 @@
from typing import TYPE_CHECKING
import structlog
+from starlette.routing import Match
+
+from airflow._shared.observability.metrics.stats import Stats
if TYPE_CHECKING:
from starlette.types import ASGIApp, Message, Receive, Scope, Send
logger = structlog.get_logger(logger_name="http.access")
_HEALTH_PATHS = frozenset(["/api/v2/monitor/health"])
+_API_PATH_PREFIX_TO_SURFACE = (
+ ("/api/v2", "public"),
+ ("/ui", "ui"),
+)
Review Comment:
> Reference in new issue
>
> ### Reference in new issue
> Repository
> Title
>
> Body
Yes, this is intentional. `/execution` is a mounted sub-application, unlike
`/api/v2` and `/ui`, which are part of the core app routing.
Because of that, simply adding `/execution` to `_API_PATH_PREFIX_TO_SURFACE`
would make its metrics coverage depend on how the app is started. In practice,
`/execution` would only be covered when it is mounted under the root app that
also has `HttpAccessLogMiddleware`, rather than being explicitly instrumented
by the execution API itself.
So I left it out on purpose for now, to keep the scope explicit and avoid
introducing coverage that is deployment-mode dependent.
##########
airflow-core/src/airflow/api_fastapi/common/http_access_log.py:
##########
@@ -23,13 +23,87 @@
from typing import TYPE_CHECKING
import structlog
+from starlette.routing import Match
+
+from airflow._shared.observability.metrics.stats import Stats
if TYPE_CHECKING:
from starlette.types import ASGIApp, Message, Receive, Scope, Send
logger = structlog.get_logger(logger_name="http.access")
_HEALTH_PATHS = frozenset(["/api/v2/monitor/health"])
+_API_PATH_PREFIX_TO_SURFACE = (
+ ("/api/v2", "public"),
+ ("/ui", "ui"),
+)
Review Comment:
Yes, this is intentional. `/execution` is a mounted sub-application, unlike
`/api/v2` and `/ui`, which are part of the core app routing.
Because of that, simply adding `/execution` to `_API_PATH_PREFIX_TO_SURFACE`
would make its metrics coverage depend on how the app is started. In practice,
`/execution` would only be covered when it is mounted under the root app that
also has `HttpAccessLogMiddleware`, rather than being explicitly instrumented
by the execution API itself.
So I left it out on purpose for now, to keep the scope explicit and avoid
introducing coverage that is deployment-mode dependent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
