You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "dsuhinin (via GitHub)" <gi...@apache.org> on 2026/06/11 15:14:59 UTC
[PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
dsuhinin opened a new pull request, #68393:
URL: https://github.com/apache/airflow/pull/68393
Mirror `airflow_shared.observability.traces.configure_otel`'s shape in the metrics module so both halves of the OTel config story have one entry point, one conf-driven setup function, and the same backcompat-bridging contract.
## What changes
- **New**: `airflow_shared.observability.metrics.configure_otel(conf)` — a single conf-driven entry point that bridges deprecated Airflow configs (`metrics.otel_host` / `otel_port` / `otel_ssl_active` / `otel_service` / `otel_interval_milliseconds`) into the standard OTel env vars, loads the exporter via
the SDK's entry-point mechanism, installs the global `MeterProvider`, and returns a `SafeOtelLogger` for the Stats factory.
- **Removed**: the 11-parameter `get_otel_logger(host=, port=, ssl_active=, prefix=, conf_interval=, debug=, ...)` function and the helper `get_otel_data_exporter` that duplicated the OTel SDK's own exporter dispatch.
- **Removed**: the 26-line shim file in `airflow-core` and the near-identical one in `task-sdk`. Both `stats_utils.py` files now wire directly to `configure_otel`.
- **Removed**: `otel_env_config.py` (entirely unused after the bridging consolidated into `_get_backcompat_config`).
OTel environment variables take precedence over the deprecated Airflow configs in every code path, matching the rule already used in `traces/configure_otel`. IPv6 host bracketing in the bridged URL is preserved via `_format_url_host`. Fork-safety reset (#64690) and the `ExponentialBucketHistogram` view
are preserved verbatim.
## Why
Before: 11-parameter shared function + 4-parameter exporter helper + a 26-line shim duplicated across two distributions + an unused env-config dataclass module. Net ~700 lines for what is, conceptually, "read conf, bridge to env, install provider."
After: one `configure_otel(conf)` function that follows the same shape as the existing traces module. ~225 fewer lines net. New developers who learn one module understand the other immediately.
## Verified
- Unit tests: 41/41 pass (8 parametrize cases for the new bridging contract; protocol-dispatch sub-tests dropped because they exercised SDK internals, not our code).
- End-to-end: metrics emitted via the deprecated `AIRFLOW__METRICS__OTEL_*` configs (no env vars set) arrived at a real OTel collector — proving the bridging works end-to-end through the OTel SDK pipeline.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #68393:
URL: https://github.com/apache/airflow/pull/68393#issuecomment-5093138011
I think this one - should really carefully test all the scenarios and components (the lifespan case from fastapi is I think an example where it would likely break things.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3659066953
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -435,70 +437,130 @@ def atexit_register_metrics_flush():
atexit.register(flush_otel_metrics)
-def get_otel_logger(
+def _get_backcompat_config(
*,
- host: str | None = None,
- port: int | None = None,
- prefix: str | None = None,
- ssl_active: bool = False,
- conf_interval: float | None = None,
- debug: bool = False,
- service_name: str | None = None,
- metrics_allow_list: str | None = None,
- metrics_block_list: str | None = None,
- stat_name_handler: Callable[[str], str] | None = None,
- statsd_influxdb_enabled: bool = False,
-) -> SafeOtelLogger:
+ host: str | None,
+ port: str | None,
+ ssl_active: bool,
+ service: str | None,
+ interval_ms: str | None,
+) -> tuple[str | None, float | None, Resource | None]:
+ resource = None
+ if service and not os.environ.get("OTEL_SERVICE_NAME") and not os.environ.get("OTEL_RESOURCE_ATTRIBUTES"):
+ resource = Resource.create(attributes={SERVICE_NAME: service})
+
+ endpoint = None
+ if (
+ host
+ and port
+ and not os.environ.get("OTEL_EXPORTER_OTLP_ENDPOINT")
+ and not os.environ.get("OTEL_EXPORTER_OTLP_METRICS_ENDPOINT")
+ ):
+ scheme = "https" if ssl_active else "http"
+ endpoint = f"{scheme}://{_format_url_host(host)}:{port}/v1/metrics"
+
+ parsed_interval_ms: float | None = None
+ if interval_ms and not os.environ.get("OTEL_METRIC_EXPORT_INTERVAL"):
+ try:
+ parsed_interval_ms = float(interval_ms)
+ except (TypeError, ValueError):
+ log.warning("Invalid metrics.otel_interval_milliseconds value: %r; ignoring.", interval_ms)
+
+ return endpoint, parsed_interval_ms, resource
+
+
+def _load_exporter_from_env() -> MetricExporter:
"""
- Build and return a :class:`SafeOtelLogger` backed by a configured :class:`MeterProvider`.
+ Pick a metric exporter per the OTel SDK environment-variable spec.
- Histogram instruments (used for ``timing()`` / ``timer()`` metrics) are aggregated with
- :class:`~opentelemetry.sdk.metrics.view.ExponentialBucketHistogramAggregation`
- so that bucket boundaries adapt automatically to the observed data range. This avoids
- the need to hand-tune explicit bucket boundaries for metrics that span very different
- scales (milliseconds to hours).
+ ``OTEL_METRICS_EXPORTER`` selects the backend (``otlp`` default; ``console``
+ for debugging; ``prometheus`` or custom values are looked up via entry
+ points). For ``otlp``, ``OTEL_EXPORTER_OTLP_METRICS_PROTOCOL`` then
+ ``OTEL_EXPORTER_OTLP_PROTOCOL`` selects the transport: ``http/protobuf``
+ (default) or ``grpc``.
+
+ See:
+ https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#exporter-selection
+ https://opentelemetry.io/docs/specs/otel/protocol/exporter/#specify-protocol
"""
- otel_env_config = load_metrics_env_config()
+ exporter_name = os.environ.get("OTEL_METRICS_EXPORTER", "otlp")
+ if exporter_name == "otlp":
+ protocol = _resolve_otlp_protocol("OTEL_EXPORTER_OTLP_METRICS_PROTOCOL")
+ if protocol == "http/protobuf":
+ from opentelemetry.exporter.otlp.proto.http.metric_exporter import OTLPMetricExporter
+
+ return OTLPMetricExporter()
+ if protocol == "grpc":
+ from opentelemetry.exporter.otlp.proto.grpc.metric_exporter import ( # type: ignore[assignment]
+ OTLPMetricExporter,
+ )
- effective_service_name: str = otel_env_config.service_name or service_name or "airflow"
- effective_prefix: str = prefix or DEFAULT_METRIC_NAME_PREFIX
- resource = Resource.create(attributes={SERVICE_NAME: effective_service_name})
+ return OTLPMetricExporter()
+ raise ValueError(f"Unsupported OTLP protocol {protocol!r}; expected 'grpc' or 'http/protobuf'.")
+ eps = entry_points(group="opentelemetry_metrics_exporter", name=exporter_name)
+ ep = next(iter(eps), None)
+ if ep is None:
+ raise RuntimeError(
+ f"No metric exporter found for OTEL_METRICS_EXPORTER={exporter_name!r}. "
+ f"Available: {[e.name for e in entry_points(group='opentelemetry_metrics_exporter')]}"
+ )
+ return ep.load()()
- # https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#periodic-exporting-metricreader
- interval = otel_env_config.interval_ms or conf_interval
- metric_exporter = get_otel_data_exporter(
- otel_env_config=otel_env_config,
+def configure_otel(
+ *,
+ host: str | None = None,
+ port: str | None = None,
+ ssl_active: bool = False,
+ service: str | None = None,
+ interval_ms: str | None = None,
+ debug: bool = False,
+ prefix: str = DEFAULT_METRIC_NAME_PREFIX,
+ allow_list: str | None = None,
+ block_list: str | None = None,
+ stat_name_handler: Callable[[str], str] | None = None,
+ statsd_influxdb_enabled: bool = False,
+) -> SafeOtelLogger:
+ backcompat_endpoint, backcompat_interval_ms, resource = _get_backcompat_config(
host=host,
port=port,
ssl_active=ssl_active,
+ service=service,
+ interval_ms=interval_ms,
)
- readers = [
+ if backcompat_endpoint:
+ os.environ["OTEL_EXPORTER_OTLP_METRICS_ENDPOINT"] = backcompat_endpoint
Review Comment:
Done — added the same style of comment the traces side carries, noting the writes are the deliberate backcompat bridge mirroring traces/configure_otel.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3420654673
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -435,70 +437,134 @@ def atexit_register_metrics_flush():
atexit.register(flush_otel_metrics)
-def get_otel_logger(
+def _get_backcompat_config(
*,
- host: str | None = None,
- port: int | None = None,
- prefix: str | None = None,
- ssl_active: bool = False,
- conf_interval: float | None = None,
- debug: bool = False,
- service_name: str | None = None,
- metrics_allow_list: str | None = None,
- metrics_block_list: str | None = None,
- stat_name_handler: Callable[[str], str] | None = None,
- statsd_influxdb_enabled: bool = False,
-) -> SafeOtelLogger:
+ host: str | None,
+ port: str | None,
+ ssl_active: bool,
+ service: str | None,
+ interval_ms: str | None,
+) -> tuple[str | None, float | None, Resource | None]:
+ resource = None
+ if service and not os.environ.get("OTEL_SERVICE_NAME") and not os.environ.get("OTEL_RESOURCE_ATTRIBUTES"):
+ resource = Resource.create(attributes={SERVICE_NAME: service})
+
+ endpoint = None
+ if (
+ host
+ and port
+ and not os.environ.get("OTEL_EXPORTER_OTLP_ENDPOINT")
+ and not os.environ.get("OTEL_EXPORTER_OTLP_METRICS_ENDPOINT")
+ ):
+ scheme = "https" if ssl_active else "http"
+ endpoint = f"{scheme}://{_format_url_host(host)}:{port}/v1/metrics"
+
+ parsed_interval_ms: float | None = None
+ if interval_ms and not os.environ.get("OTEL_METRIC_EXPORT_INTERVAL"):
+ try:
+ parsed_interval_ms = float(interval_ms)
+ except (TypeError, ValueError):
+ log.warning("Invalid metrics.otel_interval_milliseconds value: %r; ignoring.", interval_ms)
+
+ return endpoint, parsed_interval_ms, resource
+
+
+def _load_exporter_from_env() -> MetricExporter:
"""
- Build and return a :class:`SafeOtelLogger` backed by a configured :class:`MeterProvider`.
+ Pick a metric exporter per the OTel SDK environment-variable spec.
+
+ ``OTEL_METRICS_EXPORTER`` selects the backend (``otlp`` default; ``console``
+ for debugging; ``prometheus`` or custom values are looked up via entry
+ points). For ``otlp``, ``OTEL_EXPORTER_OTLP_METRICS_PROTOCOL`` then
+ ``OTEL_EXPORTER_OTLP_PROTOCOL`` selects the transport: ``http/protobuf``
+ (default) or ``grpc``.
- Histogram instruments (used for ``timing()`` / ``timer()`` metrics) are aggregated with
- :class:`~opentelemetry.sdk.metrics.view.ExponentialBucketHistogramAggregation`
- so that bucket boundaries adapt automatically to the observed data range. This avoids
- the need to hand-tune explicit bucket boundaries for metrics that span very different
- scales (milliseconds to hours).
+ See:
+ https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#exporter-selection
+ https://opentelemetry.io/docs/specs/otel/protocol/exporter/#specify-protocol
"""
- otel_env_config = load_metrics_env_config()
+ exporter_name = os.environ.get("OTEL_METRICS_EXPORTER", "otlp")
+ if exporter_name == "otlp":
+ protocol = (
+ os.environ.get("OTEL_EXPORTER_OTLP_METRICS_PROTOCOL")
+ or os.environ.get("OTEL_EXPORTER_OTLP_PROTOCOL")
+ or "http/protobuf"
+ )
+ if protocol == "http/protobuf":
+ from opentelemetry.exporter.otlp.proto.http.metric_exporter import OTLPMetricExporter
+
+ return OTLPMetricExporter()
+ if protocol == "grpc":
+ from opentelemetry.exporter.otlp.proto.grpc.metric_exporter import ( # type: ignore[assignment]
+ OTLPMetricExporter,
+ )
- effective_service_name: str = otel_env_config.service_name or service_name or "airflow"
- effective_prefix: str = prefix or DEFAULT_METRIC_NAME_PREFIX
- resource = Resource.create(attributes={SERVICE_NAME: effective_service_name})
+ return OTLPMetricExporter()
+ raise ValueError(f"Unsupported OTLP protocol {protocol!r}; expected 'grpc' or 'http/protobuf'.")
+ eps = entry_points(group="opentelemetry_metrics_exporter", name=exporter_name)
Review Comment:
I don't think anyone is going to use `zipkin` or `prometheus` for the exporter, or that we should support it. But it's in the OTel docs, and you never know what users might do. And this code might try to initialize the exporter instead of throw an error or ignore the env var.
The basic question is whether this part is redundant with the new protocol approach.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3659044702
##########
airflow-core/tests/unit/core/test_settings.py:
##########
@@ -533,3 +533,50 @@ def test_early_return_when_all_none(self):
settings.dispose_orm(do_log=False)
mock_close.assert_not_called()
+
+
+class TestInitializeStats:
Review Comment:
Fair point, and I've tightened this up in two ways.
On the aside: fixed — test_stats_initialized_during_initialize now mocks all the other initialize() steps (configure_orm, configure_logging, configure_otel, etc.), so nothing real runs in the unit test.
On the main concern: the removed tests asserted the call sites (lifespan / DFP run() / triggerer execute() calling stats.initialize), and those call sites are what this PR deletes — so they can't survive in that form. The property they indirectly guarded — "every component process ends up with a working Stats
backend" — now decomposes into exactly two invariants, and both are tested directly:
1. every process that does import airflow gets Stats initialized — TestInitializeStats (this file);
2. every forked child rebuilds its own backend — TestBackendResetAfterFork in the shared observability tests does a real os.fork() and asserts the child's backend is reset and rebuilt through the factory.
Since components no longer carry any stats-init code of their own, there's no component-specific behaviour left that could regress independently of those two invariants. The gunicorn preload case (the one place where "import happens in a different process than the work") is additionally covered by the
multi-worker run described in the thread above.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3659093053
##########
airflow-core/src/airflow/observability/metrics/otel_logger.py:
##########
@@ -26,28 +26,16 @@
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.
- port = None
- if conf.has_option("metrics", "otel_port"):
- port = conf.getint("metrics", "otel_port")
-
- conf_interval = None
- if conf.has_option("metrics", "otel_interval_milliseconds"):
- conf_interval = conf.getfloat("metrics", "otel_interval_milliseconds")
-
- return otel_logger.get_otel_logger(
- host=conf.get("metrics", "otel_host", fallback=None), # ex: "breeze-otel-collector"
- port=port, # ex: 4318
- prefix=conf.get("metrics", "otel_prefix", fallback=None), # ex: "airflow"
+ return otel_logger.configure_otel(
+ host=conf.get("metrics", "otel_host", fallback=None),
+ port=conf.get("metrics", "otel_port", fallback=None),
ssl_active=conf.getboolean("metrics", "otel_ssl_active", fallback=False),
- # PeriodicExportingMetricReader will default to an interval of 60000 millis.
- conf_interval=conf_interval, # ex: 30000
+ service=conf.get("metrics", "otel_service", fallback=None),
+ interval_ms=conf.get("metrics", "otel_interval_milliseconds", fallback=None),
debug=conf.getboolean("metrics", "otel_debugging_on", fallback=False),
- service_name=conf.get("metrics", "otel_service", fallback=None),
- metrics_allow_list=conf.get("metrics", "metrics_allow_list", fallback=None),
- metrics_block_list=conf.get("metrics", "metrics_block_list", fallback=None),
+ prefix=conf.get("metrics", "otel_prefix", fallback="airflow"),
Review Comment:
thank you! good catch on the empty-prefix regression — fixed both parts. configure_otel now takes prefix: str | None = None and applies prefix or DEFAULT_METRIC_NAME_PREFIX when building the logger (restoring what get_otel_logger did), and both shims pass fallback=None instead of hardcoding "airflow", so the default lives
only next to the constant. Added a test covering both the unset and explicitly-empty metrics.otel_prefix cases.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3397514936
##########
airflow-core/src/airflow/observability/metrics/otel_logger.py:
##########
@@ -1,53 +0,0 @@
-# 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() -> 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.
- port = None
- if conf.has_option("metrics", "otel_port"):
- port = conf.getint("metrics", "otel_port")
-
- conf_interval = None
- if conf.has_option("metrics", "otel_interval_milliseconds"):
- conf_interval = conf.getfloat("metrics", "otel_interval_milliseconds")
-
- return otel_logger.get_otel_logger(
Review Comment:
The code that lives under `shared` initializes the `otel_logger` but `airflow-core` and `task-sdk`, both have their own version of `conf`.
This is because these two packages have different versioning and they are released separately. Essentially they are independent.
This works as a factory where the caller reads its own instance of initialized conf, gets the values and passes them to the `shared` code. That's why the code duplication exists.
I don't know if it's a good idea simplifying the factory to just pass `conf` as a param.
* `conf` from `airflow-core`
https://github.com/dsuhinin/airflow/blob/dsuhinin/simplify-otel-logger-config/airflow-core/src/airflow/configuration.py#L821
* `conf` from `task-sdk`
https://github.com/dsuhinin/airflow/blob/dsuhinin/simplify-otel-logger-config/task-sdk/src/airflow/sdk/configuration.py#L325-L341
These two objects could start deviating in the future and even basic functions like `getint` could have different behavior.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on PR #68393:
URL: https://github.com/apache/airflow/pull/68393#issuecomment-4731948205
> This looks much better. Can you remove all `otlp_proto_http` references?
>
> This is one for sure
>
> https://github.com/dsuhinin/airflow/blob/dsuhinin/simplify-otel-logger-config/scripts/ci/docker-compose/integration-otel.yml#L77
I think all the references should be removed now.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3403956201
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -435,70 +437,113 @@ def atexit_register_metrics_flush():
atexit.register(flush_otel_metrics)
-def get_otel_logger(
+def _get_backcompat_config(
+ *,
+ host: str | None,
+ port: str | None,
+ ssl_active: bool,
+ service: str | None,
+ interval_ms: str | None,
+) -> tuple[str | None, float | None, Resource | None]:
+ resource = None
+ if service and not os.environ.get("OTEL_SERVICE_NAME") and not os.environ.get("OTEL_RESOURCE_ATTRIBUTES"):
+ resource = Resource.create(attributes={SERVICE_NAME: service})
+
+ endpoint = None
+ if (
+ host
+ and port
+ and not os.environ.get("OTEL_EXPORTER_OTLP_ENDPOINT")
+ and not os.environ.get("OTEL_EXPORTER_OTLP_METRICS_ENDPOINT")
+ ):
+ scheme = "https" if ssl_active else "http"
+ endpoint = f"{scheme}://{_format_url_host(host)}:{port}/v1/metrics"
+
+ parsed_interval_ms: float | None = None
+ if interval_ms and not os.environ.get("OTEL_METRIC_EXPORT_INTERVAL"):
+ try:
+ parsed_interval_ms = float(interval_ms)
+ except (TypeError, ValueError):
+ log.warning("Invalid metrics.otel_interval_milliseconds value: %r; ignoring.", interval_ms)
+
+ return endpoint, parsed_interval_ms, resource
+
+
+def _load_exporter_from_env() -> MetricExporter:
+ """
+ Load a metric exporter using the ``OTEL_METRICS_EXPORTER`` env var.
+
+ Mirrors the entry-point mechanism used by the OTEL SDK auto-instrumentation
+ configurator. Supported values (from installed packages):
+ - ``otlp`` (default) -- OTLP/gRPC
+ - ``otlp_proto_http`` -- OTLP/HTTP
Review Comment:
I can't find `otlp_proto_http`
https://opentelemetry.io/docs/languages/sdk-configuration/general/#otel_metrics_exporter
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3431001480
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -435,70 +437,112 @@ def atexit_register_metrics_flush():
atexit.register(flush_otel_metrics)
-def get_otel_logger(
+def _get_backcompat_config(
+ *,
+ host: str | None,
+ port: str | None,
+ ssl_active: bool,
+ service: str | None,
+ interval_ms: str | None,
+) -> tuple[str | None, float | None, Resource | None]:
+ resource = None
+ if service and not os.environ.get("OTEL_SERVICE_NAME") and not os.environ.get("OTEL_RESOURCE_ATTRIBUTES"):
+ resource = Resource.create(attributes={SERVICE_NAME: service})
+
+ endpoint = None
+ if (
+ host
+ and port
+ and not os.environ.get("OTEL_EXPORTER_OTLP_ENDPOINT")
+ and not os.environ.get("OTEL_EXPORTER_OTLP_METRICS_ENDPOINT")
+ ):
+ scheme = "https" if ssl_active else "http"
+ endpoint = f"{scheme}://{_format_url_host(host)}:{port}/v1/metrics"
+
+ parsed_interval_ms: float | None = None
+ if interval_ms and not os.environ.get("OTEL_METRIC_EXPORT_INTERVAL"):
+ try:
+ parsed_interval_ms = float(interval_ms)
+ except (TypeError, ValueError):
+ log.warning("Invalid metrics.otel_interval_milliseconds value: %r; ignoring.", interval_ms)
+
+ return endpoint, parsed_interval_ms, resource
+
+
+def _load_exporter_from_env() -> MetricExporter:
+ """
+ Load a metric exporter using the ``OTEL_METRICS_EXPORTER`` env var.
+
+ Mirrors the entry-point mechanism used by the OTEL SDK auto-instrumentation
+ configurator. Supported values (from installed packages):
+ - ``otlp`` (default) -- OTLP/gRPC
+ - ``console`` -- stdout (useful for debugging)
+ """
+ exporter_name = os.environ.get("OTEL_METRICS_EXPORTER", "otlp")
+ eps = entry_points(group="opentelemetry_metrics_exporter", name=exporter_name)
Review Comment:
should be fixed now.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3420654673
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -435,70 +437,134 @@ def atexit_register_metrics_flush():
atexit.register(flush_otel_metrics)
-def get_otel_logger(
+def _get_backcompat_config(
*,
- host: str | None = None,
- port: int | None = None,
- prefix: str | None = None,
- ssl_active: bool = False,
- conf_interval: float | None = None,
- debug: bool = False,
- service_name: str | None = None,
- metrics_allow_list: str | None = None,
- metrics_block_list: str | None = None,
- stat_name_handler: Callable[[str], str] | None = None,
- statsd_influxdb_enabled: bool = False,
-) -> SafeOtelLogger:
+ host: str | None,
+ port: str | None,
+ ssl_active: bool,
+ service: str | None,
+ interval_ms: str | None,
+) -> tuple[str | None, float | None, Resource | None]:
+ resource = None
+ if service and not os.environ.get("OTEL_SERVICE_NAME") and not os.environ.get("OTEL_RESOURCE_ATTRIBUTES"):
+ resource = Resource.create(attributes={SERVICE_NAME: service})
+
+ endpoint = None
+ if (
+ host
+ and port
+ and not os.environ.get("OTEL_EXPORTER_OTLP_ENDPOINT")
+ and not os.environ.get("OTEL_EXPORTER_OTLP_METRICS_ENDPOINT")
+ ):
+ scheme = "https" if ssl_active else "http"
+ endpoint = f"{scheme}://{_format_url_host(host)}:{port}/v1/metrics"
+
+ parsed_interval_ms: float | None = None
+ if interval_ms and not os.environ.get("OTEL_METRIC_EXPORT_INTERVAL"):
+ try:
+ parsed_interval_ms = float(interval_ms)
+ except (TypeError, ValueError):
+ log.warning("Invalid metrics.otel_interval_milliseconds value: %r; ignoring.", interval_ms)
+
+ return endpoint, parsed_interval_ms, resource
+
+
+def _load_exporter_from_env() -> MetricExporter:
"""
- Build and return a :class:`SafeOtelLogger` backed by a configured :class:`MeterProvider`.
+ Pick a metric exporter per the OTel SDK environment-variable spec.
+
+ ``OTEL_METRICS_EXPORTER`` selects the backend (``otlp`` default; ``console``
+ for debugging; ``prometheus`` or custom values are looked up via entry
+ points). For ``otlp``, ``OTEL_EXPORTER_OTLP_METRICS_PROTOCOL`` then
+ ``OTEL_EXPORTER_OTLP_PROTOCOL`` selects the transport: ``http/protobuf``
+ (default) or ``grpc``.
- Histogram instruments (used for ``timing()`` / ``timer()`` metrics) are aggregated with
- :class:`~opentelemetry.sdk.metrics.view.ExponentialBucketHistogramAggregation`
- so that bucket boundaries adapt automatically to the observed data range. This avoids
- the need to hand-tune explicit bucket boundaries for metrics that span very different
- scales (milliseconds to hours).
+ See:
+ https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#exporter-selection
+ https://opentelemetry.io/docs/specs/otel/protocol/exporter/#specify-protocol
"""
- otel_env_config = load_metrics_env_config()
+ exporter_name = os.environ.get("OTEL_METRICS_EXPORTER", "otlp")
+ if exporter_name == "otlp":
+ protocol = (
+ os.environ.get("OTEL_EXPORTER_OTLP_METRICS_PROTOCOL")
+ or os.environ.get("OTEL_EXPORTER_OTLP_PROTOCOL")
+ or "http/protobuf"
+ )
+ if protocol == "http/protobuf":
+ from opentelemetry.exporter.otlp.proto.http.metric_exporter import OTLPMetricExporter
+
+ return OTLPMetricExporter()
+ if protocol == "grpc":
+ from opentelemetry.exporter.otlp.proto.grpc.metric_exporter import ( # type: ignore[assignment]
+ OTLPMetricExporter,
+ )
- effective_service_name: str = otel_env_config.service_name or service_name or "airflow"
- effective_prefix: str = prefix or DEFAULT_METRIC_NAME_PREFIX
- resource = Resource.create(attributes={SERVICE_NAME: effective_service_name})
+ return OTLPMetricExporter()
+ raise ValueError(f"Unsupported OTLP protocol {protocol!r}; expected 'grpc' or 'http/protobuf'.")
+ eps = entry_points(group="opentelemetry_metrics_exporter", name=exporter_name)
Review Comment:
I don't think anyone is going to use `zipkin` or `prometheus` for the exporter, or that we should support it. But it's in the OTel docs, and you never know what users might do.
The basic question is whether this part is redundant with the new protocol approach.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3422766707
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -435,70 +437,134 @@ def atexit_register_metrics_flush():
atexit.register(flush_otel_metrics)
-def get_otel_logger(
+def _get_backcompat_config(
*,
- host: str | None = None,
- port: int | None = None,
- prefix: str | None = None,
- ssl_active: bool = False,
- conf_interval: float | None = None,
- debug: bool = False,
- service_name: str | None = None,
- metrics_allow_list: str | None = None,
- metrics_block_list: str | None = None,
- stat_name_handler: Callable[[str], str] | None = None,
- statsd_influxdb_enabled: bool = False,
-) -> SafeOtelLogger:
+ host: str | None,
+ port: str | None,
+ ssl_active: bool,
+ service: str | None,
+ interval_ms: str | None,
+) -> tuple[str | None, float | None, Resource | None]:
+ resource = None
+ if service and not os.environ.get("OTEL_SERVICE_NAME") and not os.environ.get("OTEL_RESOURCE_ATTRIBUTES"):
+ resource = Resource.create(attributes={SERVICE_NAME: service})
+
+ endpoint = None
+ if (
+ host
+ and port
+ and not os.environ.get("OTEL_EXPORTER_OTLP_ENDPOINT")
+ and not os.environ.get("OTEL_EXPORTER_OTLP_METRICS_ENDPOINT")
+ ):
+ scheme = "https" if ssl_active else "http"
+ endpoint = f"{scheme}://{_format_url_host(host)}:{port}/v1/metrics"
+
+ parsed_interval_ms: float | None = None
+ if interval_ms and not os.environ.get("OTEL_METRIC_EXPORT_INTERVAL"):
+ try:
+ parsed_interval_ms = float(interval_ms)
+ except (TypeError, ValueError):
+ log.warning("Invalid metrics.otel_interval_milliseconds value: %r; ignoring.", interval_ms)
+
+ return endpoint, parsed_interval_ms, resource
+
+
+def _load_exporter_from_env() -> MetricExporter:
"""
- Build and return a :class:`SafeOtelLogger` backed by a configured :class:`MeterProvider`.
+ Pick a metric exporter per the OTel SDK environment-variable spec.
+
+ ``OTEL_METRICS_EXPORTER`` selects the backend (``otlp`` default; ``console``
+ for debugging; ``prometheus`` or custom values are looked up via entry
+ points). For ``otlp``, ``OTEL_EXPORTER_OTLP_METRICS_PROTOCOL`` then
+ ``OTEL_EXPORTER_OTLP_PROTOCOL`` selects the transport: ``http/protobuf``
+ (default) or ``grpc``.
- Histogram instruments (used for ``timing()`` / ``timer()`` metrics) are aggregated with
- :class:`~opentelemetry.sdk.metrics.view.ExponentialBucketHistogramAggregation`
- so that bucket boundaries adapt automatically to the observed data range. This avoids
- the need to hand-tune explicit bucket boundaries for metrics that span very different
- scales (milliseconds to hours).
+ See:
+ https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#exporter-selection
+ https://opentelemetry.io/docs/specs/otel/protocol/exporter/#specify-protocol
"""
- otel_env_config = load_metrics_env_config()
+ exporter_name = os.environ.get("OTEL_METRICS_EXPORTER", "otlp")
+ if exporter_name == "otlp":
+ protocol = (
+ os.environ.get("OTEL_EXPORTER_OTLP_METRICS_PROTOCOL")
+ or os.environ.get("OTEL_EXPORTER_OTLP_PROTOCOL")
+ or "http/protobuf"
+ )
+ if protocol == "http/protobuf":
+ from opentelemetry.exporter.otlp.proto.http.metric_exporter import OTLPMetricExporter
+
+ return OTLPMetricExporter()
+ if protocol == "grpc":
+ from opentelemetry.exporter.otlp.proto.grpc.metric_exporter import ( # type: ignore[assignment]
+ OTLPMetricExporter,
+ )
- effective_service_name: str = otel_env_config.service_name or service_name or "airflow"
- effective_prefix: str = prefix or DEFAULT_METRIC_NAME_PREFIX
- resource = Resource.create(attributes={SERVICE_NAME: effective_service_name})
+ return OTLPMetricExporter()
+ raise ValueError(f"Unsupported OTLP protocol {protocol!r}; expected 'grpc' or 'http/protobuf'.")
+ eps = entry_points(group="opentelemetry_metrics_exporter", name=exporter_name)
Review Comment:
added tests for both metrics and traces to test current functionality.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3397387992
##########
shared/observability/src/airflow_shared/observability/metrics/__init__.py:
##########
@@ -14,3 +14,193 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+from __future__ import annotations
Review Comment:
ahh, ok.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3420632757
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -435,70 +437,134 @@ def atexit_register_metrics_flush():
atexit.register(flush_otel_metrics)
-def get_otel_logger(
+def _get_backcompat_config(
*,
- host: str | None = None,
- port: int | None = None,
- prefix: str | None = None,
- ssl_active: bool = False,
- conf_interval: float | None = None,
- debug: bool = False,
- service_name: str | None = None,
- metrics_allow_list: str | None = None,
- metrics_block_list: str | None = None,
- stat_name_handler: Callable[[str], str] | None = None,
- statsd_influxdb_enabled: bool = False,
-) -> SafeOtelLogger:
+ host: str | None,
+ port: str | None,
+ ssl_active: bool,
+ service: str | None,
+ interval_ms: str | None,
+) -> tuple[str | None, float | None, Resource | None]:
+ resource = None
+ if service and not os.environ.get("OTEL_SERVICE_NAME") and not os.environ.get("OTEL_RESOURCE_ATTRIBUTES"):
+ resource = Resource.create(attributes={SERVICE_NAME: service})
+
+ endpoint = None
+ if (
+ host
+ and port
+ and not os.environ.get("OTEL_EXPORTER_OTLP_ENDPOINT")
+ and not os.environ.get("OTEL_EXPORTER_OTLP_METRICS_ENDPOINT")
+ ):
+ scheme = "https" if ssl_active else "http"
+ endpoint = f"{scheme}://{_format_url_host(host)}:{port}/v1/metrics"
+
+ parsed_interval_ms: float | None = None
+ if interval_ms and not os.environ.get("OTEL_METRIC_EXPORT_INTERVAL"):
+ try:
+ parsed_interval_ms = float(interval_ms)
+ except (TypeError, ValueError):
+ log.warning("Invalid metrics.otel_interval_milliseconds value: %r; ignoring.", interval_ms)
+
+ return endpoint, parsed_interval_ms, resource
+
+
+def _load_exporter_from_env() -> MetricExporter:
"""
- Build and return a :class:`SafeOtelLogger` backed by a configured :class:`MeterProvider`.
+ Pick a metric exporter per the OTel SDK environment-variable spec.
+
+ ``OTEL_METRICS_EXPORTER`` selects the backend (``otlp`` default; ``console``
+ for debugging; ``prometheus`` or custom values are looked up via entry
+ points). For ``otlp``, ``OTEL_EXPORTER_OTLP_METRICS_PROTOCOL`` then
+ ``OTEL_EXPORTER_OTLP_PROTOCOL`` selects the transport: ``http/protobuf``
+ (default) or ``grpc``.
- Histogram instruments (used for ``timing()`` / ``timer()`` metrics) are aggregated with
- :class:`~opentelemetry.sdk.metrics.view.ExponentialBucketHistogramAggregation`
- so that bucket boundaries adapt automatically to the observed data range. This avoids
- the need to hand-tune explicit bucket boundaries for metrics that span very different
- scales (milliseconds to hours).
+ See:
+ https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#exporter-selection
+ https://opentelemetry.io/docs/specs/otel/protocol/exporter/#specify-protocol
"""
- otel_env_config = load_metrics_env_config()
+ exporter_name = os.environ.get("OTEL_METRICS_EXPORTER", "otlp")
+ if exporter_name == "otlp":
+ protocol = (
+ os.environ.get("OTEL_EXPORTER_OTLP_METRICS_PROTOCOL")
+ or os.environ.get("OTEL_EXPORTER_OTLP_PROTOCOL")
+ or "http/protobuf"
+ )
+ if protocol == "http/protobuf":
+ from opentelemetry.exporter.otlp.proto.http.metric_exporter import OTLPMetricExporter
+
+ return OTLPMetricExporter()
+ if protocol == "grpc":
+ from opentelemetry.exporter.otlp.proto.grpc.metric_exporter import ( # type: ignore[assignment]
+ OTLPMetricExporter,
+ )
- effective_service_name: str = otel_env_config.service_name or service_name or "airflow"
- effective_prefix: str = prefix or DEFAULT_METRIC_NAME_PREFIX
- resource = Resource.create(attributes={SERVICE_NAME: effective_service_name})
+ return OTLPMetricExporter()
+ raise ValueError(f"Unsupported OTLP protocol {protocol!r}; expected 'grpc' or 'http/protobuf'.")
+ eps = entry_points(group="opentelemetry_metrics_exporter", name=exporter_name)
Review Comment:
It looks good. Out of curiosity, what happens if the exporter is `zipkin` or `prometheus`? The SDK takes care of the protocol?
Can you add some unit tests for the new logic? I don't know if there are and I've missed them.
Also, traces should follow the same approach. The code seems pretty similar, we can probably extract it to a common function.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3429200555
##########
shared/observability/tests/observability/metrics/test_otel_logger.py:
##########
@@ -502,6 +457,59 @@ def test_get_otel_logger_uses_exponential_histogram_view(self, mock_provider, mo
assert isinstance(view, View)
assert isinstance(view._aggregation, ExponentialBucketHistogramAggregation)
+ @pytest.mark.parametrize(
+ ("provided_env_vars", "expected_module"),
+ [
+ pytest.param(
+ {},
+ "opentelemetry.exporter.otlp.proto.http.metric_exporter",
+ id="default_otlp_no_protocol_uses_http",
+ ),
+ pytest.param(
+ {"OTEL_EXPORTER_OTLP_PROTOCOL": "http/protobuf"},
+ "opentelemetry.exporter.otlp.proto.http.metric_exporter",
+ id="generic_protocol_http_protobuf_uses_http",
+ ),
+ pytest.param(
+ {"OTEL_EXPORTER_OTLP_PROTOCOL": "grpc"},
+ "opentelemetry.exporter.otlp.proto.grpc.metric_exporter",
+ id="generic_protocol_grpc_uses_grpc",
+ ),
+ pytest.param(
+ {"OTEL_EXPORTER_OTLP_METRICS_PROTOCOL": "grpc"},
+ "opentelemetry.exporter.otlp.proto.grpc.metric_exporter",
+ id="metrics_specific_protocol_grpc_uses_grpc",
+ ),
+ pytest.param(
+ {
+ "OTEL_EXPORTER_OTLP_PROTOCOL": "http/protobuf",
+ "OTEL_EXPORTER_OTLP_METRICS_PROTOCOL": "grpc",
+ },
+ "opentelemetry.exporter.otlp.proto.grpc.metric_exporter",
+ id="metrics_specific_protocol_overrides_generic",
+ ),
+ pytest.param(
+ {"OTEL_METRICS_EXPORTER": "console"},
Review Comment:
added more tests for both `traces` and `metrics` logic. should be good now.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3403941801
##########
airflow-core/src/airflow/observability/metrics/stats_utils.py:
##########
@@ -33,7 +33,19 @@ def get_stats_factory() -> Callable:
return statsd_logger.get_statsd_logger
Review Comment:
This is inconsistent. You can either get rid of the other files logger files in this directory and call the factory methods directly here, or bring back `otel_logger.py`.
Same for the duplicated code under `task-sdk`.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3658954162
##########
shared/observability/src/airflow_shared/observability/traces/__init__.py:
##########
@@ -229,15 +231,32 @@ def _get_backcompat_config(conf: ConfigParser) -> tuple[str | None, Resource | N
def _load_exporter_from_env() -> SpanExporter:
"""
- Load a span exporter using the OTEL_TRACES_EXPORTER env var.
+ Pick a span exporter per the OTel SDK environment-variable spec.
+
+ ``OTEL_TRACES_EXPORTER`` selects the backend (``otlp`` default; ``console``
+ for debugging; ``zipkin`` or custom values are looked up via entry points).
+ For ``otlp``, ``OTEL_EXPORTER_OTLP_TRACES_PROTOCOL`` then
+ ``OTEL_EXPORTER_OTLP_PROTOCOL`` selects the transport: ``http/protobuf``
+ (default) or ``grpc``.
- Mirrors the entry-point mechanism used by the OTEL SDK auto-instrumentation
- configurator. Supported values (from installed packages):
- - ``otlp`` (default) — OTLP/gRPC
- - ``otlp_proto_http`` — OTLP/HTTP
- - ``console`` — stdout (useful for debugging)
+ See:
+ https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#exporter-selection
+ https://opentelemetry.io/docs/specs/otel/protocol/exporter/#specify-protocol
"""
exporter_name = os.environ.get("OTEL_TRACES_EXPORTER", "otlp")
+ if exporter_name == "otlp":
Review Comment:
already added a `newsfragment`.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3659007692
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -435,70 +437,130 @@ def atexit_register_metrics_flush():
atexit.register(flush_otel_metrics)
-def get_otel_logger(
+def _get_backcompat_config(
Review Comment:
thank you! good catch — restored both warnings in _get_backcompat_config(), with the wording from the removed get_otel_data_exporter: the "both provided, using the environment variables" one when the deprecated otel_host/otel_port pair is set alongside OTEL_EXPORTER_OTLP_[METRICS_]ENDPOINT, and the deprecation warning on the bridging path. Added a parametrized test covering both paths, plus the no-deprecated-conf case staying silent.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3658940692
##########
airflow-core/src/airflow/settings.py:
##########
@@ -816,6 +835,7 @@ def initialize():
import_local_settings()
configure_logging()
configure_otel(conf)
+ _initialize_stats()
Review Comment:
The per-fork reset is there — it just doesn't live at the call sites. stats.py registers os.register_at_fork(after_in_child=_reset_backend_after_fork) at module level (since #63932), which sets _backend = None in every forked child. So under preload a worker doesn't keep the master's provider: the first metric emitted in the worker goes through _get_backend() → _factory() → get_otel_logger(). That lazy rebuild is exactly what re-runs the factory in the child — and it's what makes the Once() guard reset reachable, not unreachable: without it the rebuilt provider couldn't replace the inherited one. The call sites this PR removes were redundant with that hook. I verified it end to end: gunicorn api-server with preload_app and 2 workers, plus a test plugin touching Stats at import time so the backend and its exporter thread are built in the master before the fork (the worst case here). The master exports its metric, and both workers export request-time metrics tagged with thei
r own PIDs afterwards. I also added a unit test for the hook itself — TestBackendResetAfterFork in the shared observability tests does a real os.fork() and asserts the child's backend is reset and rebuilt fresh.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3404085350
##########
airflow-core/src/airflow/observability/metrics/stats_utils.py:
##########
@@ -33,7 +33,19 @@ def get_stats_factory() -> Callable:
return statsd_logger.get_statsd_logger
Review Comment:
should be fixed now.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #68393:
URL: https://github.com/apache/airflow/pull/68393#issuecomment-5021932155
@dsuhinin This PR has been converted to **draft** because it does not yet meet our [Pull Request quality criteria](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-quality-criteria).
**Issues found:**
- :x: **Merge conflicts**: This PR has merge conflicts with `main` and cannot be merged as-is. Rebase locally and resolve them: `git fetch upstream main && git rebase upstream/main`, resolve the conflicts, then force-push.
> **Note:** Your branch is **671 commits behind `main`**. Some check failures may be caused by changes in the base branch rather than by your PR. Please rebase your branch and push again to get up-to-date CI results.
**What to do next:**
- Fix each issue listed above.
- Make sure static checks pass locally (`prek run --from-ref main --stage pre-commit`).
- Mark the PR as "Ready for review" when you're done.
Converting a PR to draft is **not** a rejection — it is an invitation to bring the PR up to the project's standards so that maintainer review time is spent productively. There is no rush — take your time and work at your own pace. We appreciate your contribution and are happy to wait for updates. If you have questions, feel free to ask on the [Airflow Slack](https://s.apache.org/airflow-slack).
---
_Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this [two-stage triage process](https://github.com/apache/airflow/blob/main/contributing-docs/25_maintainer_pr_triage.md#why-the-first-pass-is-automated) so that our maintainers' limited time is spent where it matters most: the conversation with you._
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3414402300
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -435,70 +437,112 @@ def atexit_register_metrics_flush():
atexit.register(flush_otel_metrics)
-def get_otel_logger(
+def _get_backcompat_config(
+ *,
+ host: str | None,
+ port: str | None,
+ ssl_active: bool,
+ service: str | None,
+ interval_ms: str | None,
+) -> tuple[str | None, float | None, Resource | None]:
+ resource = None
+ if service and not os.environ.get("OTEL_SERVICE_NAME") and not os.environ.get("OTEL_RESOURCE_ATTRIBUTES"):
+ resource = Resource.create(attributes={SERVICE_NAME: service})
+
+ endpoint = None
+ if (
+ host
+ and port
+ and not os.environ.get("OTEL_EXPORTER_OTLP_ENDPOINT")
+ and not os.environ.get("OTEL_EXPORTER_OTLP_METRICS_ENDPOINT")
+ ):
+ scheme = "https" if ssl_active else "http"
+ endpoint = f"{scheme}://{_format_url_host(host)}:{port}/v1/metrics"
+
+ parsed_interval_ms: float | None = None
+ if interval_ms and not os.environ.get("OTEL_METRIC_EXPORT_INTERVAL"):
+ try:
+ parsed_interval_ms = float(interval_ms)
+ except (TypeError, ValueError):
+ log.warning("Invalid metrics.otel_interval_milliseconds value: %r; ignoring.", interval_ms)
+
+ return endpoint, parsed_interval_ms, resource
+
+
+def _load_exporter_from_env() -> MetricExporter:
+ """
+ Load a metric exporter using the ``OTEL_METRICS_EXPORTER`` env var.
+
+ Mirrors the entry-point mechanism used by the OTEL SDK auto-instrumentation
+ configurator. Supported values (from installed packages):
+ - ``otlp`` (default) -- OTLP/gRPC
+ - ``console`` -- stdout (useful for debugging)
+ """
+ exporter_name = os.environ.get("OTEL_METRICS_EXPORTER", "otlp")
+ eps = entry_points(group="opentelemetry_metrics_exporter", name=exporter_name)
Review Comment:
This follows the same approach as traces which ignores the protocol env variable.
##########
shared/observability/src/airflow_shared/observability/traces/__init__.py:
##########
Review Comment:
This approach resolves the type of the exporter (grpc or http) based on the installed packages, in which case `otlp_proto_http` is supported.
According to the OTel spec, the exporter env variable should be left to the generic `otlp` unless some other backend is used or the console for debugging
https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#exporter-selection
The exporter type is then picked based on the protocol env variable. It's defined either by the generic `OTEL_EXPORTER_OTLP_PROTOCOL` or the specific ones `OTEL_EXPORTER_OTLP_TRACES_PROTOCOL` and `OTEL_EXPORTER_OTLP_TRACES_PROTOCOL` and the possible values are `grpc` or `http/protobuf`.
https://opentelemetry.io/docs/specs/otel/protocol/exporter/#specify-protocol
With the current approach, the exporter is picked directly from the installed packages and the protocol environment variable is ignored entirely.
Just removing the param documentation from the comment doesn't do any good because with the current code, you still need to use `otlp_proto_http` to configure an http exporter.
I suggest changing this function to make it follow the OTel spec documentation because that's what users will also read and expect. They won't read the code and the current approach doesn't exist anywhere in the OTel docs.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on PR #68393:
URL: https://github.com/apache/airflow/pull/68393#issuecomment-5093745951
> The consolidation is a genuine improvement and mirroring the traces shape is the right direction — thanks for pushing this through so many rounds.
>
> Caveat up front: I have **not** run this locally, so treat all of this as reading-the-diff impressions rather than verified behaviour. Most of it is small stuff left inline. One thing does worry me though.
>
> **Removing the `lifespan` stats init.** `settings.initialize()` runs exactly once, from `airflow/__init__.py:79`, at first `import airflow` — it is _not_ re-run in a forked child. Every call site this PR removes was a post-fork re-init, and the API server one looks like the exposed case: it runs gunicorn with `"preload_app": True` (`airflow-core/src/airflow/api_fastapi/gunicorn_app.py:276`), so `import airflow` happens in the master and workers are forked from it. `lifespan` was the only per-worker hook.
>
> `stats.initialize()` sets `_backend = None`, and that is what forces the OTel `MeterProvider` — and its `PeriodicExportingMetricReader` background thread — to be rebuilt in the child. That thread doesn't survive `fork()`. If anything touches `Stats` during gunicorn preload, `_backend` gets built in the master and every worker inherits a provider whose exporter thread is dead, with nothing left to reset it. That's the same failure mode #64703 fixed, and `_initialize_api_server_stats()` was added deliberately in #68514 for it.
>
> Centralising the _configuration_ in `settings.initialize()` seems right to me — it's the per-process reset I think needs to stay at each fork boundary, either as the explicit calls or an `os.register_at_fork` hook. Could you sanity-check it against a multi-worker api-server before this goes in? It may also be related to what @xBis7 reported today about task-SDK metrics reaching Prometheus but querying empty, which is still open.
>
> The rest is inline — the traces exporter default change is the one I'd most like a newsfragment for.
Thanks for the thorough read.
On the fork boundary reset: it does stay — it just doesn't live at the call sites anymore. stats.py registers an os.register_at_fork(after_in_child=...) hook at module level (added in #63932, before this PR) that resets _backend in every forked child, so the first metric emission in a worker rebuilds the backend
through the factory. The dead-exporter-thread case is handled too: get_otel_logger() resets the OTel SDK's Once() guard before set_meter_provider() (the #64690 fix), so the child installs a fresh MeterProvider with a live PeriodicExportingMetricReader thread instead of silently keeping the parent's stale one. The
calls this PR removes were redundant with that hook — the lifespan init wasn't the only per-worker reset.
Sanity check, as requested: I ran the API server with gunicorn (preload_app, 2 workers) and a test plugin that touches Stats at import time — i.e. the backend and its exporter thread get built in the master before any fork, which is the worst case you describe. The master exported its metric, and after forking
both workers exported request-time metrics tagged with their own PIDs via the console exporter. So the per-worker rebuild works end to end.
Test coverage: the at-fork reset had no test, so I added one — it forks a real child and asserts the backend is reset and rebuilt fresh (shared/observability/tests/.../test_stats.py::TestBackendResetAfterFork).
Newsfragment: added for the traces default transport change (otlp now defaults to http/protobuf per the OTel spec instead of gRPC), including how to restore the old behaviour via OTEL_EXPORTER_OTLP_PROTOCOL=grpc.
On the task-SDK metrics report — I don't think it's related to this reset path (that's the supervisor/task runner side, not a gunicorn fork), but happy to look at it separately.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3659093053
##########
airflow-core/src/airflow/observability/metrics/otel_logger.py:
##########
@@ -26,28 +26,16 @@
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.
- port = None
- if conf.has_option("metrics", "otel_port"):
- port = conf.getint("metrics", "otel_port")
-
- conf_interval = None
- if conf.has_option("metrics", "otel_interval_milliseconds"):
- conf_interval = conf.getfloat("metrics", "otel_interval_milliseconds")
-
- return otel_logger.get_otel_logger(
- host=conf.get("metrics", "otel_host", fallback=None), # ex: "breeze-otel-collector"
- port=port, # ex: 4318
- prefix=conf.get("metrics", "otel_prefix", fallback=None), # ex: "airflow"
+ return otel_logger.configure_otel(
+ host=conf.get("metrics", "otel_host", fallback=None),
+ port=conf.get("metrics", "otel_port", fallback=None),
ssl_active=conf.getboolean("metrics", "otel_ssl_active", fallback=False),
- # PeriodicExportingMetricReader will default to an interval of 60000 millis.
- conf_interval=conf_interval, # ex: 30000
+ service=conf.get("metrics", "otel_service", fallback=None),
+ interval_ms=conf.get("metrics", "otel_interval_milliseconds", fallback=None),
debug=conf.getboolean("metrics", "otel_debugging_on", fallback=False),
- service_name=conf.get("metrics", "otel_service", fallback=None),
- metrics_allow_list=conf.get("metrics", "metrics_allow_list", fallback=None),
- metrics_block_list=conf.get("metrics", "metrics_block_list", fallback=None),
+ prefix=conf.get("metrics", "otel_prefix", fallback="airflow"),
Review Comment:
thank you! good catch on the empty-prefix regression — fixed both parts. configure_otel now takes prefix: str | None = None and applies prefix or DEFAULT_METRIC_NAME_PREFIX when building the logger (restoring what get_otel_logger did), and both shims pass fallback=None instead of hardcoding "airflow", so the default lives only next to the constant. Added a test covering both the unset and explicitly-empty metrics.otel_prefix cases.
##########
task-sdk/src/airflow/sdk/observability/metrics/otel_logger.py:
##########
@@ -26,28 +26,16 @@
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.
- port = None
- if conf.has_option("metrics", "otel_port"):
- port = conf.getint("metrics", "otel_port")
-
- conf_interval = None
- if conf.has_option("metrics", "otel_interval_milliseconds"):
- conf_interval = conf.getfloat("metrics", "otel_interval_milliseconds")
-
- return otel_logger.get_otel_logger(
- host=conf.get("metrics", "otel_host", fallback=None), # ex: "breeze-otel-collector"
- port=port, # ex: 4318
- prefix=conf.get("metrics", "otel_prefix", fallback=None), # ex: "airflow"
+ return otel_logger.configure_otel(
+ host=conf.get("metrics", "otel_host", fallback=None),
+ port=conf.get("metrics", "otel_port", fallback=None),
ssl_active=conf.getboolean("metrics", "otel_ssl_active", fallback=False),
- # PeriodicExportingMetricReader will default to an interval of 60000 millis.
- conf_interval=conf_interval, # ex: 30000
+ service=conf.get("metrics", "otel_service", fallback=None),
+ interval_ms=conf.get("metrics", "otel_interval_milliseconds", fallback=None),
debug=conf.getboolean("metrics", "otel_debugging_on", fallback=False),
- service_name=conf.get("metrics", "otel_service", fallback=None),
- metrics_allow_list=conf.get("metrics", "metrics_allow_list", fallback=None),
- metrics_block_list=conf.get("metrics", "metrics_block_list", fallback=None),
+ prefix=conf.get("metrics", "otel_prefix", fallback="airflow"),
Review Comment:
should be already fixed.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on PR #68393:
URL: https://github.com/apache/airflow/pull/68393#issuecomment-5088796702
> @dsuhinin This PR has been converted to **draft** because it does not yet meet our [Pull Request quality criteria](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-quality-criteria).
>
> **Issues found:**
>
> * ❌ **Merge conflicts**: This PR has merge conflicts with `main` and cannot be merged as-is. Rebase locally and resolve them: `git fetch upstream main && git rebase upstream/main`, resolve the conflicts, then force-push.
>
> > **Note:** Your branch is **671 commits behind `main`**. Some check failures may be caused by changes in the base branch rather than by your PR. Please rebase your branch and push again to get up-to-date CI results.
>
> **What to do next:**
>
> * Fix each issue listed above.
> * Make sure static checks pass locally (`prek run --from-ref main --stage pre-commit`).
> * Mark the PR as "Ready for review" when you're done.
>
> Converting a PR to draft is **not** a rejection — it is an invitation to bring the PR up to the project's standards so that maintainer review time is spent productively. There is no rush — take your time and work at your own pace. We appreciate your contribution and are happy to wait for updates. If you have questions, feel free to ask on the [Airflow Slack](https://s.apache.org/airflow-slack).
>
> _Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this [two-stage triage process](https://github.com/apache/airflow/blob/main/contributing-docs/25_maintainer_pr_triage.md#why-the-first-pass-is-automated) so that our maintainers' limited time is spent where it matters most: the conversation with you._
all conflicts should be now resolved.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3658940692
##########
airflow-core/src/airflow/settings.py:
##########
@@ -816,6 +835,7 @@ def initialize():
import_local_settings()
configure_logging()
configure_otel(conf)
+ _initialize_stats()
Review Comment:
The per-fork reset is there — it just doesn't live at the call sites. stats.py registers os.register_at_fork(after_in_child=_reset_backend_after_fork) at module level (since #63932), which sets _backend = None in every forked child. So under preload a worker doesn't keep the master's provider: the first metric
emitted in the worker goes through _get_backend() → _factory() → get_otel_logger(). That lazy rebuild is exactly what re-runs the factory in the child — and it's what makes the Once() guard reset reachable, not unreachable: without it the rebuilt provider couldn't replace the inherited one. The call sites this PR
removes were redundant with that hook.
I verified it end to end: gunicorn api-server with preload_app and 2 workers, plus a test plugin touching Stats at import time so the backend and its exporter thread are built in the master before the fork (the worst case here). The master exports its metric, and both workers export request-time metrics tagged
with their own PIDs afterwards. I also added a unit test for the hook itself — TestBackendResetAfterFork in the shared observability tests does a real os.fork() and asserts the child's backend is reset and rebuilt fresh.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3422763891
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -435,70 +437,134 @@ def atexit_register_metrics_flush():
atexit.register(flush_otel_metrics)
-def get_otel_logger(
+def _get_backcompat_config(
*,
- host: str | None = None,
- port: int | None = None,
- prefix: str | None = None,
- ssl_active: bool = False,
- conf_interval: float | None = None,
- debug: bool = False,
- service_name: str | None = None,
- metrics_allow_list: str | None = None,
- metrics_block_list: str | None = None,
- stat_name_handler: Callable[[str], str] | None = None,
- statsd_influxdb_enabled: bool = False,
-) -> SafeOtelLogger:
+ host: str | None,
+ port: str | None,
+ ssl_active: bool,
+ service: str | None,
+ interval_ms: str | None,
+) -> tuple[str | None, float | None, Resource | None]:
+ resource = None
+ if service and not os.environ.get("OTEL_SERVICE_NAME") and not os.environ.get("OTEL_RESOURCE_ATTRIBUTES"):
+ resource = Resource.create(attributes={SERVICE_NAME: service})
+
+ endpoint = None
+ if (
+ host
+ and port
+ and not os.environ.get("OTEL_EXPORTER_OTLP_ENDPOINT")
+ and not os.environ.get("OTEL_EXPORTER_OTLP_METRICS_ENDPOINT")
+ ):
+ scheme = "https" if ssl_active else "http"
+ endpoint = f"{scheme}://{_format_url_host(host)}:{port}/v1/metrics"
+
+ parsed_interval_ms: float | None = None
+ if interval_ms and not os.environ.get("OTEL_METRIC_EXPORT_INTERVAL"):
+ try:
+ parsed_interval_ms = float(interval_ms)
+ except (TypeError, ValueError):
+ log.warning("Invalid metrics.otel_interval_milliseconds value: %r; ignoring.", interval_ms)
+
+ return endpoint, parsed_interval_ms, resource
+
+
+def _load_exporter_from_env() -> MetricExporter:
"""
- Build and return a :class:`SafeOtelLogger` backed by a configured :class:`MeterProvider`.
+ Pick a metric exporter per the OTel SDK environment-variable spec.
+
+ ``OTEL_METRICS_EXPORTER`` selects the backend (``otlp`` default; ``console``
+ for debugging; ``prometheus`` or custom values are looked up via entry
+ points). For ``otlp``, ``OTEL_EXPORTER_OTLP_METRICS_PROTOCOL`` then
+ ``OTEL_EXPORTER_OTLP_PROTOCOL`` selects the transport: ``http/protobuf``
+ (default) or ``grpc``.
- Histogram instruments (used for ``timing()`` / ``timer()`` metrics) are aggregated with
- :class:`~opentelemetry.sdk.metrics.view.ExponentialBucketHistogramAggregation`
- so that bucket boundaries adapt automatically to the observed data range. This avoids
- the need to hand-tune explicit bucket boundaries for metrics that span very different
- scales (milliseconds to hours).
+ See:
+ https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#exporter-selection
+ https://opentelemetry.io/docs/specs/otel/protocol/exporter/#specify-protocol
"""
- otel_env_config = load_metrics_env_config()
+ exporter_name = os.environ.get("OTEL_METRICS_EXPORTER", "otlp")
+ if exporter_name == "otlp":
+ protocol = (
+ os.environ.get("OTEL_EXPORTER_OTLP_METRICS_PROTOCOL")
+ or os.environ.get("OTEL_EXPORTER_OTLP_PROTOCOL")
+ or "http/protobuf"
+ )
+ if protocol == "http/protobuf":
+ from opentelemetry.exporter.otlp.proto.http.metric_exporter import OTLPMetricExporter
+
+ return OTLPMetricExporter()
+ if protocol == "grpc":
+ from opentelemetry.exporter.otlp.proto.grpc.metric_exporter import ( # type: ignore[assignment]
+ OTLPMetricExporter,
+ )
- effective_service_name: str = otel_env_config.service_name or service_name or "airflow"
- effective_prefix: str = prefix or DEFAULT_METRIC_NAME_PREFIX
- resource = Resource.create(attributes={SERVICE_NAME: effective_service_name})
+ return OTLPMetricExporter()
+ raise ValueError(f"Unsupported OTLP protocol {protocol!r}; expected 'grpc' or 'http/protobuf'.")
+ eps = entry_points(group="opentelemetry_metrics_exporter", name=exporter_name)
Review Comment:
in case of `prometheus` is should falls through to entry-point lookup → loads `opentelemetry.exporter.prometheus.PrometheusMetricReader (if installed). Then `PeriodicExportingMetricReader` wraps it.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3658940692
##########
airflow-core/src/airflow/settings.py:
##########
@@ -816,6 +835,7 @@ def initialize():
import_local_settings()
configure_logging()
configure_otel(conf)
+ _initialize_stats()
Review Comment:
The per-fork reset is there — it just doesn't live at the call sites. stats.py registers os.register_at_fork(after_in_child=_reset_backend_after_fork) at module level (since #63932), which sets _backend = None in every forked child. So under preload a worker doesn't keep the master's provider: the first metric emitted in the worker goes through _get_backend() → _factory() → get_otel_logger(). That lazy rebuild is exactly what re-runs the factory in the child — and it's what makes the Once() guard reset reachable, not unreachable: without it the rebuilt provider couldn't replace the inherited one. The call sites this PR removes were redundant with that hook.
I verified it end to end: gunicorn api-server with preload_app and 2 workers, plus a test plugin touching Stats at import time so the backend and its exporter thread are built in the master before the fork (the worst case here). The master exports its metric, and both workers export request-time metrics tagged
with their own PIDs afterwards. I also added a unit test for the hook itself — TestBackendResetAfterFork in the shared observability tests does a real os.fork() and asserts the child's backend is reset and rebuilt fresh.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3658456358
##########
airflow-core/src/airflow/settings.py:
##########
@@ -816,6 +835,7 @@ def initialize():
import_local_settings()
configure_logging()
configure_otel(conf)
+ _initialize_stats()
Review Comment:
This is the part I'm least comfortable with (see the review body for the longer version).
`settings.initialize()` runs once, from `airflow/__init__.py:79`, at `import airflow` — it is not re-run in a forked child. The call sites this PR removes were all *post-fork* re-inits, and `stats.initialize()` setting `_backend = None` is what forces the `MeterProvider` and its `PeriodicExportingMetricReader` thread to be rebuilt in the child. That thread doesn't survive `fork()`.
Under gunicorn with `"preload_app": True` (`airflow-core/src/airflow/api_fastapi/gunicorn_app.py:276`) `import airflow` happens in the master, so if anything touches `Stats` during preload each worker inherits a provider with a dead exporter thread and nothing left to reset it. That's the failure mode #64703 fixed, and `_initialize_api_server_stats()` was added in #68514 for exactly this.
Worth noting too: the fork guard you carefully kept in `configure_otel` —
```python
_metrics_internal._METER_PROVIDER_SET_ONCE._done = False
```
— only does anything if something re-runs the factory in the child. I can't see what would after this change, which suggests it (and issue #64690 behind it) becomes unreachable in the scenario its own comment describes.
Centralising the configuration here seems right; it's the per-process reset that I think needs to stay at each fork boundary, or move to an `os.register_at_fork` hook.
##########
shared/observability/src/airflow_shared/observability/traces/__init__.py:
##########
@@ -229,15 +231,32 @@ def _get_backcompat_config(conf: ConfigParser) -> tuple[str | None, Resource | N
def _load_exporter_from_env() -> SpanExporter:
"""
- Load a span exporter using the OTEL_TRACES_EXPORTER env var.
+ Pick a span exporter per the OTel SDK environment-variable spec.
+
+ ``OTEL_TRACES_EXPORTER`` selects the backend (``otlp`` default; ``console``
+ for debugging; ``zipkin`` or custom values are looked up via entry points).
+ For ``otlp``, ``OTEL_EXPORTER_OTLP_TRACES_PROTOCOL`` then
+ ``OTEL_EXPORTER_OTLP_PROTOCOL`` selects the transport: ``http/protobuf``
+ (default) or ``grpc``.
- Mirrors the entry-point mechanism used by the OTEL SDK auto-instrumentation
- configurator. Supported values (from installed packages):
- - ``otlp`` (default) — OTLP/gRPC
- - ``otlp_proto_http`` — OTLP/HTTP
- - ``console`` — stdout (useful for debugging)
+ See:
+ https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#exporter-selection
+ https://opentelemetry.io/docs/specs/otel/protocol/exporter/#specify-protocol
"""
exporter_name = os.environ.get("OTEL_TRACES_EXPORTER", "otlp")
+ if exporter_name == "otlp":
Review Comment:
This changes the default traces transport, and I think it needs a newsfragment.
On `main`, `OTEL_TRACES_EXPORTER=otlp` (the default) resolves through the OTel entry point to the **gRPC** span exporter — the docstring being replaced says so:
```
- ``otlp`` (default) — OTLP/gRPC
- ``otlp_proto_http`` — OTLP/HTTP
```
The new short-circuit makes `otlp` mean `http/protobuf` unless `OTEL_EXPORTER_OTLP_[TRACES_]PROTOCOL` says otherwise, and your doc change in this PR says the same:
> `- # Core tracing defaults the exporter to OTLP/gRPC.`
> `+ # Core tracing defaults to OTLP over HTTP/protobuf (port 4318, ...)`
Matching the OTel spec default is defensible, but existing deployments pointing `OTEL_EXPORTER_OTLP_TRACES_ENDPOINT` at a gRPC collector on 4317 without an explicit protocol will silently stop delivering traces.
Per [`AGENTS.md`](https://github.com/apache/airflow/blob/main/AGENTS.md):
> For `airflow-core` (and `chart/`, `dev/mypy/`) **user-facing** changes, add a newsfragment in that distribution's `newsfragments/` directory.
`shared/` and `task-sdk/` ship inside `airflow-core`, so could you add `airflow-core/newsfragments/68393.significant.rst` noting the new default and the one-line `OTEL_EXPORTER_OTLP_TRACES_PROTOCOL=grpc` migration?
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -435,70 +437,130 @@ def atexit_register_metrics_flush():
atexit.register(flush_otel_metrics)
-def get_otel_logger(
+def _get_backcompat_config(
Review Comment:
The two deprecation warnings from the removed `get_otel_data_exporter` don't have a replacement here:
```python
log.warning(
"Both the standard OpenTelemetry environment variables and "
"the Airflow OpenTelemetry configs have been provided. "
"Using the OpenTelemetry environment variables. ..."
)
...
log.warning(
"The Airflow OpenTelemetry configs have been deprecated and will be removed in the future. ..."
)
```
After this change the conf values are silently ignored when the env vars are set, and silently bridged when they aren't. All of these keys carry `version_deprecated: 3.2.0` in `config.yml`, and that runtime warning is the only heads-up a deployment gets before they're removed — I'd keep both on the bridging path.
##########
airflow-core/tests/unit/core/test_settings.py:
##########
@@ -533,3 +533,50 @@ def test_early_return_when_all_none(self):
settings.dispose_orm(do_log=False)
mock_close.assert_not_called()
+
+
+class TestInitializeStats:
Review Comment:
These replace `TestInitializeApiServerStats` (3 tests, including `test_stats_initialized_during_lifespan`), `test_stats_initialize_called_on_run` (Dag processor) and `test_stats_initialize_called_on_execute` (triggerer) — but they only assert that `settings.initialize()` calls `settings._initialize_stats()`.
So nothing asserts any more that the API server / Dag processor / triggerer / executor processes actually end up with an initialised `Stats` backend, which is the property my concern on `settings.py` is about. From [`AGENTS.md` § Testing Standards](https://github.com/apache/airflow/blob/main/AGENTS.md):
> Every changed or added behaviour must have a test; every test must fail without the PR's change.
Minor aside on `test_stats_initialized_during_initialize` below: it calls the real `settings.initialize()` with only `prepare_syspath_for_config_and_plugins` and `import_local_settings` mocked, so `configure_orm` / `configure_logging` / `configure_otel` all run for real in a unit test.
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -435,70 +437,130 @@ def atexit_register_metrics_flush():
atexit.register(flush_otel_metrics)
-def get_otel_logger(
+def _get_backcompat_config(
*,
- host: str | None = None,
- port: int | None = None,
- prefix: str | None = None,
- ssl_active: bool = False,
- conf_interval: float | None = None,
- debug: bool = False,
- service_name: str | None = None,
- metrics_allow_list: str | None = None,
- metrics_block_list: str | None = None,
- stat_name_handler: Callable[[str], str] | None = None,
- statsd_influxdb_enabled: bool = False,
-) -> SafeOtelLogger:
+ host: str | None,
+ port: str | None,
+ ssl_active: bool,
+ service: str | None,
+ interval_ms: str | None,
+) -> tuple[str | None, float | None, Resource | None]:
+ resource = None
+ if service and not os.environ.get("OTEL_SERVICE_NAME") and not os.environ.get("OTEL_RESOURCE_ATTRIBUTES"):
+ resource = Resource.create(attributes={SERVICE_NAME: service})
+
+ endpoint = None
+ if (
+ host
+ and port
+ and not os.environ.get("OTEL_EXPORTER_OTLP_ENDPOINT")
+ and not os.environ.get("OTEL_EXPORTER_OTLP_METRICS_ENDPOINT")
+ ):
+ scheme = "https" if ssl_active else "http"
+ endpoint = f"{scheme}://{_format_url_host(host)}:{port}/v1/metrics"
+
+ parsed_interval_ms: float | None = None
+ if interval_ms and not os.environ.get("OTEL_METRIC_EXPORT_INTERVAL"):
+ try:
+ parsed_interval_ms = float(interval_ms)
+ except (TypeError, ValueError):
+ log.warning("Invalid metrics.otel_interval_milliseconds value: %r; ignoring.", interval_ms)
+
+ return endpoint, parsed_interval_ms, resource
+
+
+def _load_exporter_from_env() -> MetricExporter:
"""
- Build and return a :class:`SafeOtelLogger` backed by a configured :class:`MeterProvider`.
+ Pick a metric exporter per the OTel SDK environment-variable spec.
- Histogram instruments (used for ``timing()`` / ``timer()`` metrics) are aggregated with
- :class:`~opentelemetry.sdk.metrics.view.ExponentialBucketHistogramAggregation`
- so that bucket boundaries adapt automatically to the observed data range. This avoids
- the need to hand-tune explicit bucket boundaries for metrics that span very different
- scales (milliseconds to hours).
+ ``OTEL_METRICS_EXPORTER`` selects the backend (``otlp`` default; ``console``
+ for debugging; ``prometheus`` or custom values are looked up via entry
+ points). For ``otlp``, ``OTEL_EXPORTER_OTLP_METRICS_PROTOCOL`` then
+ ``OTEL_EXPORTER_OTLP_PROTOCOL`` selects the transport: ``http/protobuf``
+ (default) or ``grpc``.
+
+ See:
+ https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#exporter-selection
+ https://opentelemetry.io/docs/specs/otel/protocol/exporter/#specify-protocol
"""
- otel_env_config = load_metrics_env_config()
+ exporter_name = os.environ.get("OTEL_METRICS_EXPORTER", "otlp")
+ if exporter_name == "otlp":
+ protocol = _resolve_otlp_protocol("OTEL_EXPORTER_OTLP_METRICS_PROTOCOL")
+ if protocol == "http/protobuf":
+ from opentelemetry.exporter.otlp.proto.http.metric_exporter import OTLPMetricExporter
+
+ return OTLPMetricExporter()
+ if protocol == "grpc":
+ from opentelemetry.exporter.otlp.proto.grpc.metric_exporter import ( # type: ignore[assignment]
+ OTLPMetricExporter,
+ )
- effective_service_name: str = otel_env_config.service_name or service_name or "airflow"
- effective_prefix: str = prefix or DEFAULT_METRIC_NAME_PREFIX
- resource = Resource.create(attributes={SERVICE_NAME: effective_service_name})
+ return OTLPMetricExporter()
+ raise ValueError(f"Unsupported OTLP protocol {protocol!r}; expected 'grpc' or 'http/protobuf'.")
+ eps = entry_points(group="opentelemetry_metrics_exporter", name=exporter_name)
+ ep = next(iter(eps), None)
+ if ep is None:
+ raise RuntimeError(
+ f"No metric exporter found for OTEL_METRICS_EXPORTER={exporter_name!r}. "
+ f"Available: {[e.name for e in entry_points(group='opentelemetry_metrics_exporter')]}"
+ )
+ return ep.load()()
- # https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#periodic-exporting-metricreader
- interval = otel_env_config.interval_ms or conf_interval
- metric_exporter = get_otel_data_exporter(
- otel_env_config=otel_env_config,
+def configure_otel(
+ *,
+ host: str | None = None,
+ port: str | None = None,
+ ssl_active: bool = False,
+ service: str | None = None,
+ interval_ms: str | None = None,
+ debug: bool = False,
+ prefix: str = DEFAULT_METRIC_NAME_PREFIX,
+ allow_list: str | None = None,
+ block_list: str | None = None,
+ stat_name_handler: Callable[[str], str] | None = None,
+ statsd_influxdb_enabled: bool = False,
+) -> SafeOtelLogger:
+ backcompat_endpoint, backcompat_interval_ms, resource = _get_backcompat_config(
host=host,
port=port,
ssl_active=ssl_active,
+ service=service,
+ interval_ms=interval_ms,
)
- readers = [
+ if backcompat_endpoint:
+ os.environ["OTEL_EXPORTER_OTLP_METRICS_ENDPOINT"] = backcompat_endpoint
Review Comment:
Nit: these two writes are process-global — they're inherited by every subprocess started afterwards, and they become the "env wins" input on any later `configure_otel()` call in the same process.
It mirrors what `traces/configure_otel` already does, so the symmetry is clearly deliberate — just worth a one-line comment saying so, like the traces side carries.
##########
airflow-core/src/airflow/observability/metrics/otel_logger.py:
##########
@@ -26,28 +26,16 @@
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.
- port = None
- if conf.has_option("metrics", "otel_port"):
- port = conf.getint("metrics", "otel_port")
-
- conf_interval = None
- if conf.has_option("metrics", "otel_interval_milliseconds"):
- conf_interval = conf.getfloat("metrics", "otel_interval_milliseconds")
-
- return otel_logger.get_otel_logger(
- host=conf.get("metrics", "otel_host", fallback=None), # ex: "breeze-otel-collector"
- port=port, # ex: 4318
- prefix=conf.get("metrics", "otel_prefix", fallback=None), # ex: "airflow"
+ return otel_logger.configure_otel(
+ host=conf.get("metrics", "otel_host", fallback=None),
+ port=conf.get("metrics", "otel_port", fallback=None),
ssl_active=conf.getboolean("metrics", "otel_ssl_active", fallback=False),
- # PeriodicExportingMetricReader will default to an interval of 60000 millis.
- conf_interval=conf_interval, # ex: 30000
+ service=conf.get("metrics", "otel_service", fallback=None),
+ interval_ms=conf.get("metrics", "otel_interval_milliseconds", fallback=None),
debug=conf.getboolean("metrics", "otel_debugging_on", fallback=False),
- service_name=conf.get("metrics", "otel_service", fallback=None),
- metrics_allow_list=conf.get("metrics", "metrics_allow_list", fallback=None),
- metrics_block_list=conf.get("metrics", "metrics_block_list", fallback=None),
+ prefix=conf.get("metrics", "otel_prefix", fallback="airflow"),
Review Comment:
`fallback="airflow"` hardcodes the value of `DEFAULT_METRIC_NAME_PREFIX`, which is exported from the same module you're calling into (`shared/observability/.../metrics/otel_logger.py`) — and it's duplicated in the task-SDK shim too.
There's also a small behaviour change hiding here: `configure_otel` now uses `prefix` directly, where `get_otel_logger` did `prefix or DEFAULT_METRIC_NAME_PREFIX`. So an explicitly-empty `metrics.otel_prefix` used to fall back to `airflow` and now strips the prefix from every metric name.
Importing the constant and keeping `fallback=None` would take care of both.
##########
task-sdk/src/airflow/sdk/observability/metrics/otel_logger.py:
##########
@@ -26,28 +26,16 @@
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.
- port = None
- if conf.has_option("metrics", "otel_port"):
- port = conf.getint("metrics", "otel_port")
-
- conf_interval = None
- if conf.has_option("metrics", "otel_interval_milliseconds"):
- conf_interval = conf.getfloat("metrics", "otel_interval_milliseconds")
-
- return otel_logger.get_otel_logger(
- host=conf.get("metrics", "otel_host", fallback=None), # ex: "breeze-otel-collector"
- port=port, # ex: 4318
- prefix=conf.get("metrics", "otel_prefix", fallback=None), # ex: "airflow"
+ return otel_logger.configure_otel(
+ host=conf.get("metrics", "otel_host", fallback=None),
+ port=conf.get("metrics", "otel_port", fallback=None),
ssl_active=conf.getboolean("metrics", "otel_ssl_active", fallback=False),
- # PeriodicExportingMetricReader will default to an interval of 60000 millis.
- conf_interval=conf_interval, # ex: 30000
+ service=conf.get("metrics", "otel_service", fallback=None),
+ interval_ms=conf.get("metrics", "otel_interval_milliseconds", fallback=None),
debug=conf.getboolean("metrics", "otel_debugging_on", fallback=False),
- service_name=conf.get("metrics", "otel_service", fallback=None),
- metrics_allow_list=conf.get("metrics", "metrics_allow_list", fallback=None),
- metrics_block_list=conf.get("metrics", "metrics_block_list", fallback=None),
+ prefix=conf.get("metrics", "otel_prefix", fallback="airflow"),
Review Comment:
Same as the `airflow-core` shim: `fallback="airflow"` duplicates `DEFAULT_METRIC_NAME_PREFIX`, and an explicitly-empty `metrics.otel_prefix` now strips the prefix instead of falling back to it. Importing the constant and keeping `fallback=None` covers both.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3397360245
##########
shared/observability/src/airflow_shared/observability/metrics/__init__.py:
##########
@@ -14,3 +14,193 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+from __future__ import annotations
Review Comment:
This isn't a good place for configuring OTel metrics. `traces` uses the `__init__.py` file because it has been agreed that OTel will be the only supported implementation for traces across Airflow.
But for metrics, multiple backends are supported. This code belongs under `otel_logger.py` unless it's shared between all the different loggers in this directory.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3414284568
##########
airflow-core/src/airflow/observability/metrics/otel_logger.py:
##########
@@ -1,53 +0,0 @@
-# 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() -> 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.
- port = None
- if conf.has_option("metrics", "otel_port"):
- port = conf.getint("metrics", "otel_port")
-
- conf_interval = None
- if conf.has_option("metrics", "otel_interval_milliseconds"):
- conf_interval = conf.getfloat("metrics", "otel_interval_milliseconds")
-
- return otel_logger.get_otel_logger(
Review Comment:
This isn't relevant anymore. It has been updated. Resolving the comment.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3414284568
##########
airflow-core/src/airflow/observability/metrics/otel_logger.py:
##########
@@ -1,53 +0,0 @@
-# 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() -> 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.
- port = None
- if conf.has_option("metrics", "otel_port"):
- port = conf.getint("metrics", "otel_port")
-
- conf_interval = None
- if conf.has_option("metrics", "otel_interval_milliseconds"):
- conf_interval = conf.getfloat("metrics", "otel_interval_milliseconds")
-
- return otel_logger.get_otel_logger(
Review Comment:
This isn't relevant anymore. It has been updated.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "dsuhinin (via GitHub)" <gi...@apache.org>.
dsuhinin commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3422759318
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -435,70 +437,134 @@ def atexit_register_metrics_flush():
atexit.register(flush_otel_metrics)
-def get_otel_logger(
+def _get_backcompat_config(
*,
- host: str | None = None,
- port: int | None = None,
- prefix: str | None = None,
- ssl_active: bool = False,
- conf_interval: float | None = None,
- debug: bool = False,
- service_name: str | None = None,
- metrics_allow_list: str | None = None,
- metrics_block_list: str | None = None,
- stat_name_handler: Callable[[str], str] | None = None,
- statsd_influxdb_enabled: bool = False,
-) -> SafeOtelLogger:
+ host: str | None,
+ port: str | None,
+ ssl_active: bool,
+ service: str | None,
+ interval_ms: str | None,
+) -> tuple[str | None, float | None, Resource | None]:
+ resource = None
+ if service and not os.environ.get("OTEL_SERVICE_NAME") and not os.environ.get("OTEL_RESOURCE_ATTRIBUTES"):
+ resource = Resource.create(attributes={SERVICE_NAME: service})
+
+ endpoint = None
+ if (
+ host
+ and port
+ and not os.environ.get("OTEL_EXPORTER_OTLP_ENDPOINT")
+ and not os.environ.get("OTEL_EXPORTER_OTLP_METRICS_ENDPOINT")
+ ):
+ scheme = "https" if ssl_active else "http"
+ endpoint = f"{scheme}://{_format_url_host(host)}:{port}/v1/metrics"
+
+ parsed_interval_ms: float | None = None
+ if interval_ms and not os.environ.get("OTEL_METRIC_EXPORT_INTERVAL"):
+ try:
+ parsed_interval_ms = float(interval_ms)
+ except (TypeError, ValueError):
+ log.warning("Invalid metrics.otel_interval_milliseconds value: %r; ignoring.", interval_ms)
+
+ return endpoint, parsed_interval_ms, resource
+
+
+def _load_exporter_from_env() -> MetricExporter:
"""
- Build and return a :class:`SafeOtelLogger` backed by a configured :class:`MeterProvider`.
+ Pick a metric exporter per the OTel SDK environment-variable spec.
+
+ ``OTEL_METRICS_EXPORTER`` selects the backend (``otlp`` default; ``console``
+ for debugging; ``prometheus`` or custom values are looked up via entry
+ points). For ``otlp``, ``OTEL_EXPORTER_OTLP_METRICS_PROTOCOL`` then
+ ``OTEL_EXPORTER_OTLP_PROTOCOL`` selects the transport: ``http/protobuf``
+ (default) or ``grpc``.
- Histogram instruments (used for ``timing()`` / ``timer()`` metrics) are aggregated with
- :class:`~opentelemetry.sdk.metrics.view.ExponentialBucketHistogramAggregation`
- so that bucket boundaries adapt automatically to the observed data range. This avoids
- the need to hand-tune explicit bucket boundaries for metrics that span very different
- scales (milliseconds to hours).
+ See:
+ https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#exporter-selection
+ https://opentelemetry.io/docs/specs/otel/protocol/exporter/#specify-protocol
"""
- otel_env_config = load_metrics_env_config()
+ exporter_name = os.environ.get("OTEL_METRICS_EXPORTER", "otlp")
+ if exporter_name == "otlp":
+ protocol = (
+ os.environ.get("OTEL_EXPORTER_OTLP_METRICS_PROTOCOL")
+ or os.environ.get("OTEL_EXPORTER_OTLP_PROTOCOL")
+ or "http/protobuf"
+ )
+ if protocol == "http/protobuf":
+ from opentelemetry.exporter.otlp.proto.http.metric_exporter import OTLPMetricExporter
+
+ return OTLPMetricExporter()
+ if protocol == "grpc":
+ from opentelemetry.exporter.otlp.proto.grpc.metric_exporter import ( # type: ignore[assignment]
+ OTLPMetricExporter,
+ )
- effective_service_name: str = otel_env_config.service_name or service_name or "airflow"
- effective_prefix: str = prefix or DEFAULT_METRIC_NAME_PREFIX
- resource = Resource.create(attributes={SERVICE_NAME: effective_service_name})
+ return OTLPMetricExporter()
+ raise ValueError(f"Unsupported OTLP protocol {protocol!r}; expected 'grpc' or 'http/protobuf'.")
+ eps = entry_points(group="opentelemetry_metrics_exporter", name=exporter_name)
Review Comment:
in case of `zipkin` I think it should falls through to the entry-point lookup → RuntimeError("No metric exporter found for OTEL_METRICS_EXPORTER='zipkin'. Available: [...]")`
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -435,70 +437,134 @@ def atexit_register_metrics_flush():
atexit.register(flush_otel_metrics)
-def get_otel_logger(
+def _get_backcompat_config(
*,
- host: str | None = None,
- port: int | None = None,
- prefix: str | None = None,
- ssl_active: bool = False,
- conf_interval: float | None = None,
- debug: bool = False,
- service_name: str | None = None,
- metrics_allow_list: str | None = None,
- metrics_block_list: str | None = None,
- stat_name_handler: Callable[[str], str] | None = None,
- statsd_influxdb_enabled: bool = False,
-) -> SafeOtelLogger:
+ host: str | None,
+ port: str | None,
+ ssl_active: bool,
+ service: str | None,
+ interval_ms: str | None,
+) -> tuple[str | None, float | None, Resource | None]:
+ resource = None
+ if service and not os.environ.get("OTEL_SERVICE_NAME") and not os.environ.get("OTEL_RESOURCE_ATTRIBUTES"):
+ resource = Resource.create(attributes={SERVICE_NAME: service})
+
+ endpoint = None
+ if (
+ host
+ and port
+ and not os.environ.get("OTEL_EXPORTER_OTLP_ENDPOINT")
+ and not os.environ.get("OTEL_EXPORTER_OTLP_METRICS_ENDPOINT")
+ ):
+ scheme = "https" if ssl_active else "http"
+ endpoint = f"{scheme}://{_format_url_host(host)}:{port}/v1/metrics"
+
+ parsed_interval_ms: float | None = None
+ if interval_ms and not os.environ.get("OTEL_METRIC_EXPORT_INTERVAL"):
+ try:
+ parsed_interval_ms = float(interval_ms)
+ except (TypeError, ValueError):
+ log.warning("Invalid metrics.otel_interval_milliseconds value: %r; ignoring.", interval_ms)
+
+ return endpoint, parsed_interval_ms, resource
+
+
+def _load_exporter_from_env() -> MetricExporter:
"""
- Build and return a :class:`SafeOtelLogger` backed by a configured :class:`MeterProvider`.
+ Pick a metric exporter per the OTel SDK environment-variable spec.
+
+ ``OTEL_METRICS_EXPORTER`` selects the backend (``otlp`` default; ``console``
+ for debugging; ``prometheus`` or custom values are looked up via entry
+ points). For ``otlp``, ``OTEL_EXPORTER_OTLP_METRICS_PROTOCOL`` then
+ ``OTEL_EXPORTER_OTLP_PROTOCOL`` selects the transport: ``http/protobuf``
+ (default) or ``grpc``.
- Histogram instruments (used for ``timing()`` / ``timer()`` metrics) are aggregated with
- :class:`~opentelemetry.sdk.metrics.view.ExponentialBucketHistogramAggregation`
- so that bucket boundaries adapt automatically to the observed data range. This avoids
- the need to hand-tune explicit bucket boundaries for metrics that span very different
- scales (milliseconds to hours).
+ See:
+ https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#exporter-selection
+ https://opentelemetry.io/docs/specs/otel/protocol/exporter/#specify-protocol
"""
- otel_env_config = load_metrics_env_config()
+ exporter_name = os.environ.get("OTEL_METRICS_EXPORTER", "otlp")
+ if exporter_name == "otlp":
+ protocol = (
+ os.environ.get("OTEL_EXPORTER_OTLP_METRICS_PROTOCOL")
+ or os.environ.get("OTEL_EXPORTER_OTLP_PROTOCOL")
+ or "http/protobuf"
+ )
+ if protocol == "http/protobuf":
+ from opentelemetry.exporter.otlp.proto.http.metric_exporter import OTLPMetricExporter
+
+ return OTLPMetricExporter()
+ if protocol == "grpc":
+ from opentelemetry.exporter.otlp.proto.grpc.metric_exporter import ( # type: ignore[assignment]
+ OTLPMetricExporter,
+ )
- effective_service_name: str = otel_env_config.service_name or service_name or "airflow"
- effective_prefix: str = prefix or DEFAULT_METRIC_NAME_PREFIX
- resource = Resource.create(attributes={SERVICE_NAME: effective_service_name})
+ return OTLPMetricExporter()
+ raise ValueError(f"Unsupported OTLP protocol {protocol!r}; expected 'grpc' or 'http/protobuf'.")
+ eps = entry_points(group="opentelemetry_metrics_exporter", name=exporter_name)
Review Comment:
in case of `zipkin` I think it should falls through to the entry-point lookup →` RuntimeError("No metric exporter found for OTEL_METRICS_EXPORTER='zipkin'. Available: [...]")`
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #68393:
URL: https://github.com/apache/airflow/pull/68393#discussion_r3427328275
##########
shared/observability/tests/observability/metrics/test_otel_logger.py:
##########
@@ -502,6 +457,59 @@ def test_get_otel_logger_uses_exponential_histogram_view(self, mock_provider, mo
assert isinstance(view, View)
assert isinstance(view._aggregation, ExponentialBucketHistogramAggregation)
+ @pytest.mark.parametrize(
+ ("provided_env_vars", "expected_module"),
+ [
+ pytest.param(
+ {},
+ "opentelemetry.exporter.otlp.proto.http.metric_exporter",
+ id="default_otlp_no_protocol_uses_http",
+ ),
+ pytest.param(
+ {"OTEL_EXPORTER_OTLP_PROTOCOL": "http/protobuf"},
+ "opentelemetry.exporter.otlp.proto.http.metric_exporter",
+ id="generic_protocol_http_protobuf_uses_http",
+ ),
+ pytest.param(
+ {"OTEL_EXPORTER_OTLP_PROTOCOL": "grpc"},
+ "opentelemetry.exporter.otlp.proto.grpc.metric_exporter",
+ id="generic_protocol_grpc_uses_grpc",
+ ),
+ pytest.param(
+ {"OTEL_EXPORTER_OTLP_METRICS_PROTOCOL": "grpc"},
+ "opentelemetry.exporter.otlp.proto.grpc.metric_exporter",
+ id="metrics_specific_protocol_grpc_uses_grpc",
+ ),
+ pytest.param(
+ {
+ "OTEL_EXPORTER_OTLP_PROTOCOL": "http/protobuf",
+ "OTEL_EXPORTER_OTLP_METRICS_PROTOCOL": "grpc",
+ },
+ "opentelemetry.exporter.otlp.proto.grpc.metric_exporter",
+ id="metrics_specific_protocol_overrides_generic",
+ ),
+ pytest.param(
+ {"OTEL_METRICS_EXPORTER": "console"},
Review Comment:
We need some tests with more values passed to `OTEL_METRICS_EXPORTER` or `OTEL_TRACES_EXPORTER` for `test_traces.py`.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify OTEL metrics config to mirror traces pattern [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #68393:
URL: https://github.com/apache/airflow/pull/68393#issuecomment-6457254612
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org