[AIP-49] OpenTelemetry Traces for Apache Airflow

[howardyoo]

closes #37752

---

This is a PR for AIP-49 which is Open Telemetry support for Airflow. In last year, a group of contributors pushed out the first release of Airflow's commitment to OpenTelemetry by providing OTEL metrics support. This PR addresses the second phase of the OTEL implementation for Airflow, which provides emitting Traces.

[ferruzzi]

This is huge, I'm going to have to take it in multiple chunks. To start with, your static checks are failing. You can run breeze static-checks --all-files locally to see what that is grumpy about.

[ferruzzi]

First chunk of comments.

[ferruzzi]

Another small chunk

Overall it looks impressive. Most of my comments are around formatting and consistency so far. Great work. I'll get more done later and tomorrow.

[ferruzzi]

Almost done with the first pass.

[ferruzzi]

Done with my first pass! LOOOTS of comments, but nothing terribly major. Great work.

[ferruzzi]

Looking better. Made another pass.

[ferruzzi]

We need to get these static checks passing and it looks like it's an issue that breeze should be able to autofix. When you get time, please run breeze static-checks --all-files. It will fail but should automatically fix the issue, then run it again and see if it passes. If it fails the second time, then it's something you'll need to fix manually.

Note, each run is going to take a while... depending on your computer, something like 15 minutes, so maybe run it while you are away from your desk or in a meeting or something.

[potiuk]

Actually breeze static-checks --only-my-changes should run WAY faster and do 9X% up to 100% of the job.

[potiuk]

Static test + docs fix left to go :)

[howardyoo]

Okay... ah..

…

On Tue, May 28, 2024 at 12:28 AM Jarek Potiuk ***@***.***> wrote: Static test + docs fix left to go :) — Reply to this email directly, view it on GitHub <#37948 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHZNLLW2WBZ4DYL27OX2GQTZEQIYHAVCNFSM6AAAAABEJXBRD6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZUGM3TCMRVGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[potiuk]

Anyone wants to take another pass ? @ferruzzi @uranusjr ? Or should I just merge it ?

[potiuk]

One more request before we merge it @howardyoo -> could you please add newsfragment describing the traces?

[howardyoo]

One more request before we merge it @howardyoo -> could you please add newsfragment describing the traces?

No problem! Just added and committed.

[howardyoo]

Gotit. fixingit..

[hussein-awala]

I'm very late to the party, but I have two comments:

• why do we need to configure the OTEL connection via Airflow configs while we can create an Airflow connection contains the credentials and the configurations in the extra and just provide its name? For me this step should be done before merging the PR as it's our recommended way for such things.
• I see that there is an intention to support multiple tracers, but using configuration dedicated for OTEL would complicate the integration later, what we do usually is using two configs: class or module to use + kwargs to pass. In your case you need two configs: the module to use to create the tracer + a connection name as I explained in the first comment.

WDYT?

For the OTEL tracer it looks good, great job!

[howardyoo]

I'm very late to the party, but I have two comments:

Hey @hussein-awala, thank you for your comments. It's pretty late alright, but I do welcome comments always!

• why do we need to configure the OTEL connection via Airflow configs while we can create an Airflow connection contains the credentials and the configurations in the extra and just provide its name? For me this step should be done before merging the PR as it's our recommended way for such things.

For the configuration schemes used for OTEL traces, I have decided to have it closely resemble our previous implementation of OTEL metrics, due to the fact that I did not want to introduce a new way to configure it (and might risk people from getting confused). However, there is another PR that I've created called 'OTEL providers for Apache Airflow', which in that case, it will use the airflow connections. The PR is still far from getting reviewed :-). In order for the OTEL integration to use the airflow connection as you mentioned, this may need to involve a new PR to perhaps modify the OTEL metrics support to also use the connection, something that I believe could be considered as an enhancement in the future.

• I see that there is an intention to support multiple tracers, but using configuration dedicated for OTEL would complicate the integration later, what we do usually is using two configs: class or module to use + kwargs to pass. In your case you need two configs: the module to use to create the tracer + a connection name as I explained in the first comment.

I'm not sure what we do usually means, here. Can you clarify? Do you mean we as Apache Airflow, or someone else (OTEL community) ? Also, I'm not sure what using two configs: class or module to use + kwargs to pass means.

WDYT?

For the OTEL tracer it looks good, great job!

[potiuk]

I'm not sure what we do usually means, here. Can you clarify? Do you mean we as Apache Airflow, or someone else (OTEL community) ? Also, I'm not sure what using two configs: class or module to use + kwargs to pass means.

The thing here is that we are mostly following the "regular" way how OTEL is onfigured (or that's what I understand - @howardyoo to confirm). When you look at OTEL documentation, many of the configuration options there are by the choice of classes and configuring them is mostly about setting the right environment variables or passing a config.yaml file - they read to configure itself. And I think we should keep it this way:

• in Airlfow configuration we can configure enabling OTEL and some "airlfow" side of it
• we leave the detailed configuration of specific OTEL classes to env variables / yaml files or whatever they need

This way we have greater flexibility, do not have to write our own configuration documentation.

Also see https://opentelemetry.io/docs/collector/configuration/#environment-variables

Hey @hussein-awala -> do you have the questions answered ?

[potiuk]

Any comments @hussein-awala ?

[potiuk]

cc: @hussein-awala -> WDYT? I would love to merge that one now :)

[potiuk]

I had a lot of conversation about open-telemetry and traces at Berlin Buzzwords and watched some talks and I really think it is going to make problem diagnosis and resolution much easier @howardyoo @ferruzzi - let's continue with the provider and get it out in 2.10 for people to start using it.

[howardyoo]

I had a lot of conversation about open-telemetry and traces at Berlin Buzzwords and watched some talks and I really think it is going to make problem diagnosis and resolution much easier @howardyoo @ferruzzi - let's continue with the provider and get it out in 2.10 for people to start using it.

I'd like to express my gratitude to @potiuk and @ferruzzi for getting this done! Thank you so much. Yes, our next mountain should be the OTEL instrumentation piece, and also the OTEL providers. We haven't made the PR for part 2, but I'll start working on it. Thank you!