jason810496 commented on code in PR #56150: URL: https://github.com/apache/airflow/pull/56150#discussion_r2384026572
########## airflow-core/src/airflow/utils/otel_config.py: ########## @@ -0,0 +1,181 @@ +# +# 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 Review Comment: ```suggestion import structlog ``` ########## airflow-core/src/airflow/utils/otel_config.py: ########## @@ -0,0 +1,181 @@ +# +# 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 +import os +from dataclasses import dataclass +from enum import Enum +from functools import lru_cache + +log = logging.getLogger(__name__) + + +def _parse_kv_str_to_dict(str_var: str) -> dict[str, str]: Review Comment: Is it possible to reuse the util here ? https://github.com/apache/airflow/blob/681b9e9d4eed41f0806ffaf05e84edc7a679fc08/airflow-core/src/airflow/traces/utils.py#L94-L95 ########## airflow-core/src/airflow/utils/otel_config.py: ########## @@ -0,0 +1,181 @@ +# +# 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 +import os +from dataclasses import dataclass +from enum import Enum +from functools import lru_cache + +log = logging.getLogger(__name__) + + +def _parse_kv_str_to_dict(str_var: str) -> dict[str, str]: + """ + Convert a string of key-value pairs to a dictionary. + + Environment variables like 'OTEL_RESOURCE_ATTRIBUTES' or 'OTEL_EXPORTER_OTLP_HEADERS' + accept values with the format "key1=value1,key2=value2,..." + """ + configs = {} + if str_var: + for pair in str_var.split(","): + if "=" in pair: + k, v = pair.split("=", 1) + configs[k.strip()] = v.strip() + return configs + + +class OtelDataType(str, Enum): + """Enum with the different telemetry data types.""" + + TRACES = "traces" + METRICS = "metrics" + LOGS = "logs" + + +@dataclass(frozen=True) +class OtelConfig: + """Immutable class for holding and validating OTel config environment variables.""" + + data_type: OtelDataType # traces | metrics + endpoint: str # url + protocol: str # "grpc" or "http/protobuf" + exporter: str # OTEL_TRACES_EXPORTER | OTEL_METRICS_EXPORTER + service_name: str # default "Airflow" + headers_kv_str: str + headers: dict[str, str] + resource_attributes_kv_str: str + resource_attributes: dict[str, str] + interval_ms: int + + def __post_init__(self): + """Validate the environment variables where necessary.""" + endpoint_type_specific = ( + "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT" + if self.data_type == OtelDataType.TRACES + else "OTEL_EXPORTER_OTLP_METRICS_ENDPOINT" + ) + + if not self.endpoint: + raise OSError( Review Comment: Would `ValueError` be better ? ```suggestion raise ValueError( ``` ########## airflow-core/src/airflow/utils/otel_config.py: ########## @@ -0,0 +1,181 @@ +# Review Comment: Please avoid adding new module under `airflow.utils` as we are trying to cleanup all the `utils` https://github.com/apache/airflow/issues/51671. Perhaps it would be better to use `airflow.otel`, or alternatively, either `airflow.metrics` or `airflow.traces`. ########## airflow-core/tests/unit/core/test_otel_tracer.py: ########## @@ -42,61 +41,67 @@ def name(): class TestOtelTrace: def test_get_otel_tracer_from_trace_metaclass(self): """Test that `Trace.some_method()`, uses an `OtelTrace` instance when otel is configured.""" - conf.set("traces", "otel_on", "True") - conf.set("traces", "otel_debugging_on", "True") - - tracer = otel_tracer.get_otel_tracer(Trace) - assert tracer.use_simple_processor is False + with env_vars( + { + "AIRFLOW__TRACES__OTEL_ON": "True", + "OTEL_EXPORTER_OTLP_ENDPOINT": "http://localhost:4318";, + "OTEL_TRACES_EXPORTER": "console", + } + ): + tracer = otel_tracer.get_otel_tracer(Trace) + assert tracer.use_simple_processor is False - assert isinstance(Trace.factory(), EmptyTrace) + assert isinstance(Trace.factory(), EmptyTrace) - Trace.configure_factory() - assert isinstance(Trace.factory(), OtelTrace) + Trace.configure_factory() + assert isinstance(Trace.factory(), OtelTrace) - task_tracer = otel_tracer.get_otel_tracer_for_task(Trace) - assert task_tracer.use_simple_processor is True + task_tracer = otel_tracer.get_otel_tracer_for_task(Trace) + assert task_tracer.use_simple_processor is True - task_tracer.get_otel_tracer_provider() - assert task_tracer.use_simple_processor is True + task_tracer.get_otel_tracer_provider() + assert task_tracer.use_simple_processor is True def test_debug_trace_metaclass(self): """Test that `DebugTrace.some_method()`, uses the correct instance when the debug_traces flag is configured.""" - conf.set("traces", "otel_on", "True") - conf.set("traces", "otel_debug_traces_on", "False") - - assert DebugTrace.check_debug_traces_flag is True - - # Factory hasn't been configured, it defaults to EmptyTrace. - assert not isinstance(DebugTrace.factory(), OtelTrace) - assert isinstance(DebugTrace.factory(), EmptyTrace) - - DebugTrace.configure_factory() - # Factory has been configured, it should still be EmptyTrace. - assert not isinstance(DebugTrace.factory(), OtelTrace) - assert isinstance(DebugTrace.factory(), EmptyTrace) + with env_vars( + { + "AIRFLOW__TRACES__OTEL_ON": "True", + "OTEL_EXPORTER_OTLP_ENDPOINT": "http://localhost:4318";, + "OTEL_TRACES_EXPORTER": "otlp", + } + ): Review Comment: Same here. ########## airflow-core/tests/unit/core/test_otel_tracer.py: ########## @@ -42,61 +41,67 @@ def name(): class TestOtelTrace: def test_get_otel_tracer_from_trace_metaclass(self): """Test that `Trace.some_method()`, uses an `OtelTrace` instance when otel is configured.""" - conf.set("traces", "otel_on", "True") - conf.set("traces", "otel_debugging_on", "True") - - tracer = otel_tracer.get_otel_tracer(Trace) - assert tracer.use_simple_processor is False + with env_vars( + { + "AIRFLOW__TRACES__OTEL_ON": "True", + "OTEL_EXPORTER_OTLP_ENDPOINT": "http://localhost:4318";, + "OTEL_TRACES_EXPORTER": "console", + } + ): Review Comment: How about using `@conf_vars` decorator at test method level then we could leave rest of the assert code as is. -- 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]
