amoghrajesh commented on code in PR #56187: URL: https://github.com/apache/airflow/pull/56187#discussion_r2580122578
########## airflow-core/src/airflow/observability/stats.py: ########## @@ -22,11 +22,11 @@ from collections.abc import Callable Review Comment: Since this file is moved from airflow.stats to airflow.observability.stats, we must add a compatibility shim / deprecation warning for people trying to import from old path. Check how it done in here and do it similarly: https://github.com/apache/airflow/blob/main/airflow-core/src/airflow/__init__.py ########## airflow-core/src/airflow/observability/metrics/__init__.py: ########## @@ -0,0 +1,16 @@ +# Licensed to the Apache Software Foundation (ASF) under one Review Comment: Similarly for the move of metric, we might have to maintain older `metrics` path and add similar deprecation warning into init, you can check this for better example: https://github.com/apache/airflow/blob/main/airflow-core/src/airflow/utils/__init__.py#L23-L58 ########## task-sdk/tests/task_sdk/execution_time/test_sentry.py: ########## @@ -26,8 +26,8 @@ import pytest import uuid6 -from airflow._shared.timezones import timezone from airflow.providers.standard.operators.python import PythonOperator +from airflow.sdk._shared.timezones import timezone Review Comment: Nice catch ########## task-sdk/tests/task_sdk/docs/test_public_api.py: ########## @@ -56,6 +56,7 @@ def test_airflow_sdk_no_unexpected_exports(): "secrets_masker", "configuration", "module_loading", + "observability", Review Comment: This will actually be part of the public API, so we should add a: ```python __lazy_imports: dict[str, str] = { "Stats": ".observability.stats", "Trace": ".observability.trace", } ``` In the `task-sdk/__init__.py` ########## airflow-core/src/airflow/observability/metrics/otel_logger.py: ########## @@ -0,0 +1,57 @@ +# 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 + +from typing import TYPE_CHECKING + +from airflow._shared.observability.metrics import otel_logger +from airflow.configuration import conf + +if TYPE_CHECKING: + from airflow._shared.observability.metrics.otel_logger import SafeOtelLogger + + +def get_otel_logger(cls) -> SafeOtelLogger: + host = conf.get("metrics", "otel_host") # ex: "breeze-otel-collector" + port = conf.getint("metrics", "otel_port") # ex: 4318 + prefix = conf.get("metrics", "otel_prefix") # ex: "airflow" + ssl_active = conf.getboolean("metrics", "otel_ssl_active") + # PeriodicExportingMetricReader will default to an interval of 60000 millis. + conf_interval = conf.getfloat("metrics", "otel_interval_milliseconds", fallback=None) # ex: 30000 + debug = conf.getboolean("metrics", "otel_debugging_on") + service_name = conf.get("metrics", "otel_service") + + metrics_allow_list = conf.get("metrics", "metrics_allow_list", fallback=None) + metrics_block_list = conf.get("metrics", "metrics_block_list", fallback=None) + + stat_name_handler = conf.getimport("metrics", "stat_name_handler") + statsd_influxdb_enabled = conf.getboolean("metrics", "statsd_influxdb_enabled", fallback=False) + + return otel_logger.get_otel_logger( + cls, + host, + port, + prefix, + ssl_active, + conf_interval, + debug, + service_name, + metrics_allow_list, + metrics_block_list, + stat_name_handler, + statsd_influxdb_enabled, + ) Review Comment: I love how clean the initialisation is now ########## airflow-core/src/airflow/observability/metrics/statsd_logger.py: ########## @@ -0,0 +1,76 @@ +# 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 logging +from typing import TYPE_CHECKING + +from airflow._shared.configuration import AirflowConfigException +from airflow._shared.observability.metrics import statsd_logger +from airflow.configuration import conf + +if TYPE_CHECKING: + from airflow._shared.observability.metrics.statsd_logger import SafeStatsdLogger + +log = logging.getLogger(__name__) + + +def get_statsd_logger(cls) -> SafeStatsdLogger: Review Comment: Perfect -- 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]
