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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
