-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Use the regular OTel environment variables for configuring traces and metrics #56150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
jason810496
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks for the PR!
Would it be better to retain the compatibility layer for Airflow-Otel environment variables and configuration, while also raising a deprecation warning? This PR introduces a breaking change for users currently relying on Otel.
| log = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def _parse_kv_str_to_dict(str_var: str) -> dict[str, str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to reuse the util here ?
airflow/airflow-core/src/airflow/traces/utils.py
Lines 94 to 95 in 681b9e9
| def parse_tracestate(tracestate_str: str | None = None) -> dict: | |
| """Parse tracestate string: rojo=00f067aa0ba902b7,congo=t61rcWkgMzE.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, I would like to remove that file (traces/utils.py) entirely. The code that was using it, isn't called anymore.
Generating and assigning a trace ID and a span ID are internal operations of the OTel SDK.
The traceparent and the tracestate belong to the span context which is handled by the propagators API.
airflow/airflow-core/src/airflow/traces/otel_tracer.py
Lines 286 to 294 in aa398f3
| def inject(self) -> dict: | |
| """Inject the current span context into a carrier and return it.""" | |
| carrier: dict[str, str] = {} | |
| TraceContextTextMapPropagator().inject(carrier) | |
| return carrier | |
| def extract(self, carrier: dict) -> Context: | |
| """Extract the span context from a provided carrier.""" | |
| return TraceContextTextMapPropagator().extract(carrier) |
The inject and extract methods are taking care of the context processing.
These methods are a hacky way of handling spans, just like the Airflow config values that are getting hard-coded into the callers to the OTel SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually shocked we don't already have a string-to-dict helper in airflow/utils. I think we used to have one for env_var parsing, but can't find it.
|
@jason810496 Thank you for the review!
Sure, I'll do that. The code of this patch does a validation on the provided configs. I think priorities should be
If the Airflow configs are empty, then it will load the OTel env vars and do the validation. In the future, we will just remove the 1st step and it will go straight to loading the env vars. The Airflow configs have a fallback value. As a result, they will never be empty and the env vars won't be checked. I think the default should be What do you think? |
|
It's complaining about the 2nd upper case letter in a row but https://opentelemetry.io/docs/ I'll update it. |
jason810496
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code of this patch does a validation on the provided configs. I think priorities should be
- Airflow configs
- OTel env variables
If the Airflow configs are empty, then it will load the OTel env vars and do the validation. In the future, we will just remove the 1st step and it will go straight to loading the env vars.
Yes, that is exactly what I'm thinking about. Thanks!
airflow-core/docs/administration-and-deployment/logging-monitoring/metrics.rst
Show resolved
Hide resolved
|
@jason810496 Is there a standard way for marking a config as deprecated? I have found what to do with configs that will change in the future (moved or renamed) but I'm not sure about the ones that will be removed. |
ferruzzi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all of my concerns and nitpicks have been addressed. Thanks!
jason810496
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
airflow-core/docs/administration-and-deployment/logging-monitoring/metrics.rst
Outdated
Show resolved
Hide resolved
airflow-core/docs/administration-and-deployment/logging-monitoring/traces.rst
Outdated
Show resolved
Hide resolved
2910670 to
f72c75d
Compare
|
conflicts :( |
|
@potiuk This PR isn't ready because it depends on #56187. Once that gets merged then this one will be adjusted accordingly. I like the simplicity of #56634. There is the question whether to follow the same approach or not. The difference between #56634 and the current patch is that this PR is doing a validation on the configs. |
|
God question. The simpler the better and I think we should not really validate something that hotel validates (or should) |
This is true, OTel doesn't validate the configs, it just uses the values and then throws errors. I added the validation because the SDK initialization will fail and I wanted to let users know in advance what's wrong. |
Sorry, I oversight this message. airflow/airflow-core/src/airflow/configuration.py Lines 345 to 349 in f3ad20a
I'm not sure how should we deal with removing the config pair as well. |
356657b to
b198691
Compare
b198691 to
4ddb14b
Compare
|
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. |
jason810496
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seem this PR is depends on #56187 .
I will review again after #56187 get merged.
Hi @xBis7
Would you still like to continue with this PR? #56187 was merged in Dec 2025. Thanks!
Is there a standard way for marking a config as deprecated? I have found what to do with configs that will change in the future (moved or renamed) but I'm not sure about the ones that will be removed.
I just traced the relative context again and found that we can leverage version_deprecated and deprecation_reason for airflow-core/src/airflow/config_templates/config.yml:
| "version_deprecated": { | |
| "type": [ | |
| "string", | |
| "null" | |
| ], | |
| "description": "When set to a version string, this option is deprecated as of this version, and will be removed in the future." | |
| }, | |
| "deprecation_reason": { | |
| "type": [ | |
| "string", | |
| "null" | |
| ], | |
| "description": "The reason why this option is deprecated." | |
| }, |
Here is a good PR example of how to deprecate config properly:
https://github.com/apache/airflow/pull/33136/files
|
Hi @jason810496, thank you for your continuous help on this. This PR was left behind due to other work priorities. I resumed looking into it a couple days ago. The branch has diverged so much from main that it's painful to try to rebase it and resolve the conflicts. So, I started a new clean branch and once the changes are done, I'm going to push directly to this one, essentially replacing the branches. I'm going to remove the validations on the environment variables. We will check the env vars first and if the values are none, then we will check the airflow config. I had forgot about adding the deprecation warning. Thanks for reminding me! |
Same though for me as well, rebasing on large conflicts is too painful. |
OTel environment variables
The OpenTelemetry SDK for metrics and traces, can be configured with two ways
There are multiple OTel env variables which can be automatically picked up by the SDK if exported.
https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/
https://opentelemetry.io/docs/languages/sdk-configuration/general/
https://opentelemetry.io/docs/languages/sdk-configuration/otlp-exporter/
Currently, there is no need to support all of them.
Airflow is providing the code configuration based on
How it works
On the OTel side, the environment variables are only checked if there is no code configuration. If a value is provided in the code, then that will take priority and the environment will be ignored.
Airflow has its own version of the OTel configs. It reads the Airflow-OTel properties and based on the values, it uses code to configure the OTel SDK.
If we were to export the regular environment variables and then just call the SDK methods without any parameters, the env values would be automatically used.
The OTel priorities are
The current Airflow priorities are
For Airflow, there is always code configuration and therefore as mentioned above the OTel env vars are ignored.
Changes
This patch removes all OTel related configs from the Airflow configuration except the flags that enable OTel metrics and traces.
The values that we would get from the Airflow config, are now accessed through the following OTel environment variables.
For info regarding the values of each property, check the updated docs.
OTEL_TRACES_EXPORTERreplacesotel_debugging_onif the value isconsolereplaces
OTEL_METRIC_EXPORT_INTERVALreplacesotel_interval_millisecondsWhy
When you have a cluster where multiple applications are running and all of them are using OTel, then it’s common to configure the regular OTel environment variables and export them. That way, you won’t have to configure each project separately to work with your shared otel-collector service.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.