ferruzzi commented on code in PR #56150:
URL: https://github.com/apache/airflow/pull/56150#discussion_r2770334001
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -387,28 +437,32 @@ def get_otel_logger(
stat_name_handler: Callable[[str], str] | None = None,
statsd_influxdb_enabled: bool = False,
) -> SafeOtelLogger:
- effective_service_name: str = service_name or "airflow"
+ otel_env_config = load_metrics_env_config()
+
+ effective_service_name: str = otel_env_config.service_name or service_name
Review Comment:
No more falling back on "airflow"?
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -18,18 +18,21 @@
import datetime
import logging
-import os
import random
import warnings
from collections.abc import Callable
from typing import TYPE_CHECKING
from opentelemetry import metrics
-from opentelemetry.exporter.otlp.proto.http.metric_exporter import
OTLPMetricExporter
from opentelemetry.sdk.metrics import MeterProvider
-from opentelemetry.sdk.metrics._internal.export import ConsoleMetricExporter,
PeriodicExportingMetricReader
+from opentelemetry.sdk.metrics._internal.export import (
+ ConsoleMetricExporter,
+ MetricExporter,
+ PeriodicExportingMetricReader,
+)
from opentelemetry.sdk.resources import SERVICE_NAME, Resource
+from ..otel_env_config import OtelEnvConfig, load_metrics_env_config
from .protocols import Timer
from .validators import (
Review Comment:
I see that this is existing code, but we don't generally use relative
imports in Airflow. Is there a reason we're using it here?
##########
airflow-core/src/airflow/observability/metrics/otel_logger.py:
##########
@@ -26,17 +26,28 @@
def get_otel_logger() -> SafeOtelLogger:
+ # The config values have been deprecated and therefore,
+ # if the user hasn't added them to the config, the default values won't be
used.
+ # A fallback is needed to avoid an exception.
Review Comment:
Non-blocking suggestion for here and the get_tracer below: Can you add a
log message here to the effect of "if any of these config values exist, use
them but log a warning that they are deprecated"?
##########
shared/observability/src/airflow_shared/observability/traces/otel_tracer.py:
##########
@@ -142,9 +146,9 @@ def start_span(
links=None,
start_time=None,
):
- """Start a span; if service_name is not given, otel_service is used."""
- if component is None:
- component = self.otel_service
+ """Start a span."""
+ # Common practice is to use the module name.
+ component = component or __name__
Review Comment:
This will break existing user dashboards, right? Do we need to handle this
as a breaking change and do a deprecation process?
##########
shared/observability/src/airflow_shared/observability/traces/otel_tracer.py:
##########
@@ -339,18 +343,54 @@ def get_otel_tracer(
otel_service: str | None = None,
debug: bool = False,
) -> OtelTrace:
- """Get OTEL tracer from airflow configuration."""
+ """Get OTEL tracer from the regular OTel env variables or the airflow
configuration."""
+ otel_env_config = load_traces_env_config()
+
tag_string = cls.get_constant_tags()
protocol = "https" if ssl_active else "http"
- # Allow transparent support for standard OpenTelemetry SDK environment
variables.
- #
https://opentelemetry.io/docs/specs/otel/protocol/exporter/#configuration-options
- endpoint = os.environ.get("OTEL_EXPORTER_OTLP_TRACES_ENDPOINT",
f"{protocol}://{host}:{port}/v1/traces")
+ # According to the OpenTelemetry Spec, specific config options like
'OTEL_EXPORTER_OTLP_TRACES_ENDPOINT'
+ # take precedence over generic ones like 'OTEL_EXPORTER_OTLP_ENDPOINT'.
+ env_exporter_protocol = (
+ otel_env_config.type_specific_exporter_protocol or
otel_env_config.exporter_protocol
+ )
+ env_endpoint = otel_env_config.type_specific_endpoint or
otel_env_config.base_endpoint
+
+ # If the protocol env var isn't set, then it will be None,
+ # and it will default to an http/protobuf exporter.
+ if env_endpoint and env_exporter_protocol == "grpc":
Review Comment:
Lots of shared code with get_metric_exporter. Consider a shared helper
method? Something like `parse_otel_config` in your new `otel_env_config.py`??
##########
task-sdk/src/airflow/sdk/observability/traces/otel_tracer.py:
##########
@@ -27,14 +27,21 @@
def get_otel_tracer(cls, use_simple_processor: bool = False) -> OtelTrace:
+ # The config values have been deprecated and therefore,
+ # if the user hasn't added them to the config, the default values won't be
used.
+ # A fallback is needed to avoid an exception.
Review Comment:
Same as above. Should we have a warning if any of the deprecated values are
used here?
--
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]
