potiuk commented on code in PR #70758:
URL: https://github.com/apache/airflow/pull/70758#discussion_r4065970534
##########
shared/observability/src/airflow_shared/observability/traces/__init__.py:
##########
@@ -248,7 +248,36 @@ def _load_exporter_from_env() -> SpanExporter:
return ep.load()()
+def _adopt_existing_tracer_provider() -> bool:
+ """
+ Attach Airflow's id generator to a TracerProvider installed by somebody
else.
+
+ OpenTelemetry auto-instrumentation and vendor distros set the global
provider before
+ ``configure_otel`` runs, and ``set_tracer_provider`` then only logs a
warning instead
+ of replacing it -- silently dropping the generator that Airflow's span id
propagation
+ depends on. Prefer selecting the generator through the
``opentelemetry_id_generator``
+ entry point (``OTEL_PYTHON_ID_GENERATOR=airflow``): a ``Tracer`` captures
the generator
+ it is built with, so anything already created by the time we get here
keeps the
+ original one.
+ """
+ provider = trace.get_tracer_provider()
+ if not isinstance(provider, TracerProvider):
+ return False
+ if not isinstance(provider.id_generator, OverrideableRandomIdGenerator):
+ provider.id_generator = OverrideableRandomIdGenerator()
+ log.warning(
+ "A TracerProvider was already installed; patched in %s so Airflow
span ids "
+ "keep propagating. Set OTEL_PYTHON_ID_GENERATOR=airflow to have it
applied "
+ "when the provider is built instead.",
+ OverrideableRandomIdGenerator.__name__,
+ )
+ return True
+
+
def configure_otel(conf: ConfigParser):
+ if _adopt_existing_tracer_provider():
Review Comment:
This runs before `otel_on` is read on line 281, so the adoption happens
whether or not Airflow tracing is enabled.
The case I'd want to be sure about: a deployment under
`opentelemetry-instrument` that has deliberately set `otel_on=False`. Today
`configure_otel` does nothing for them. After this change their
`TracerProvider.id_generator` is replaced and every component start logs the
warning from line 268, recommending an environment variable for a feature they
have switched off.
`test_patches_id_generator_onto_existing_provider` parametrizes
`otel_on=[True, False]` and asserts the patch in both, so this reads as
intentional rather than accidental — could you record the reasoning in a
comment here? My instinct is that the adoption belongs behind the `otel_on`
check, but you have thought about this more than I have.
Separately, this `return` also skips the entire exporter and span-processor
setup below. I think that is the right call — a vendor distro has already
configured its own exporters and a second one would be worse — but an operator
with `otel_host` / `otel_port` in their config gets no indication those
settings have stopped applying. Worth a sentence in the warning on line 268 and
in the new docs section.
##########
airflow-core/pyproject.toml:
##########
@@ -211,6 +211,11 @@ dependencies = [
[project.scripts]
airflow = "airflow.__main__:main"
+# Lets OpenTelemetry auto-instrumentation and vendor distros select Airflow's
id generator
+# with OTEL_PYTHON_ID_GENERATOR=airflow, instead of reaching into a private
module.
+[project.entry-points.opentelemetry_id_generator]
Review Comment:
Worth a note here about a coupling that isn't visible from this file.
The same source is importable by three paths —
`airflow._shared.observability.traces`,
`airflow.sdk._shared.observability.traces` (task-sdk symlinks the same
directory) and `airflow_shared.observability.traces` — and Python treats each
as a separate module with its own `OverrideableRandomIdGenerator` class object.
So the `isinstance` check in `_adopt_existing_tracer_provider` only succeeds
when the entry-point-loaded class and the checking code came through the same
path.
That holds right now: `settings.py:42` is the only caller of
`configure_otel` and imports `airflow._shared…`, matching this entry point
exactly. But if task-sdk ever calls it, the check fails silently and a second
generator gets patched in over a perfectly good one, with a misleading warning.
Either a duck-typed check on the generator side, or a comment here recording
that this path must match whatever `configure_otel` imports, would keep that
from being a surprise later.
--
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]
