ashb commented on code in PR #64523: URL: https://github.com/apache/airflow/pull/64523#discussion_r4156261070
########## airflow-core/src/airflow/api_fastapi/common/http_metrics.py: ########## @@ -0,0 +1,132 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""HTTP API metrics middleware.""" + +from __future__ import annotations + +import time +from typing import TYPE_CHECKING + +import structlog + +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.metrics") + +_API_METRICS_PATH_PREFIXES = ("/api/v2", "/ui") Review Comment: Big thing missing here is `/api/execution` for the TaskSDK execution API ########## airflow-core/tests/unit/api_fastapi/common/test_http_metrics.py: ########## @@ -0,0 +1,279 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import asyncio +from unittest import mock + +import pytest +import structlog.testing +from fastapi import FastAPI +from starlette.responses import PlainTextResponse +from starlette.testclient import TestClient + +from airflow.api_fastapi.common.http_metrics import ( + HttpMetricsMiddleware, + _emit_api_metrics, + _get_status_family, +) + + +def _make_app() -> FastAPI: Review Comment: This should be a test fixture, and we only need to create it once I think: ```suggestion @pytest.fixture(scope="module") def test_app() -> FastAPI: ``` ########## airflow-core/tests/unit/api_fastapi/common/test_http_metrics.py: ########## @@ -0,0 +1,279 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import asyncio +from unittest import mock + +import pytest +import structlog.testing +from fastapi import FastAPI +from starlette.responses import PlainTextResponse +from starlette.testclient import TestClient + +from airflow.api_fastapi.common.http_metrics import ( + HttpMetricsMiddleware, + _emit_api_metrics, + _get_status_family, +) + + +def _make_app() -> FastAPI: Review Comment: And then we should also have ``` @pytest.fixture def test_client(test_app) -> TestClient: return TestClient(test_app, raise_server_exceptions=False) ``` I think that'll all work. ########## shared/observability/src/airflow_shared/observability/metrics/metrics_template.yaml: ########## @@ -690,9 +690,22 @@ metrics: legacy_name: "-" name_variables: ["status"] + - name: "http_requests_total" + description: "Number of completed REST API requests, tagged by method, route, and status_family." Review Comment: So my only other comment here, is if someone is still using a statsd that doesn't support tagging, this this metric doesn't break out by status, op, template or anything else. Without those tags, is it still worthwhile emitting those metrics? -- 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]
