-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
[AIP-49] Breeze OpenTelemetry Integration #29521
[AIP-49] Breeze OpenTelemetry Integration #29521
Conversation
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 would not qualify myself as a Docker expert so take my approval lightly but I did not see anything standing out
- INTEGRATION_STATSD=true | ||
- AIRFLOW__METRICS__STATSD_ON=True | ||
- AIRFLOW__METRICS__STATSD_HOST=statsd-exporter | ||
- AIRFLOW__METRICS__STATSD_PORT=9125 |
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.
Are these statsd values being set temporary? Since this is the OTEL integration I would expect to see OTEL env vars here not statsd
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.
Correct, at the moment there is no code in Airflow that would accept other values so I left these for now, but in hindsight that doesn't make any sense I guess. I'll remove them and leave a note to see the statsd one for formatting once they are implemented
8092112
to
2441a90
Compare
Sorry for the force, I had to rebase |
2441a90
to
3786b04
Compare
Main is broken again, should have not rebased yet I guess. |
looks transient error with httpbin. rerunning. |
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.
woooho.. progress.
Cool. I peeked in the PRs list and there were a LOT of them failing so I assumed it was another main hiccup. |
Actually - if my guess is right, then we have to fix it and NOT use httpbin in our tests - this means that they rely on external service we have no control over and SOMEONE my deliberately actively impair our CI. |
I didn't copy the error message from before, but it looks familiar, I think it's the same transient one @potiuk mentioned. |
8b70c06
to
a08c2ec
Compare
I tihnk this is simply some kind of instability of the With Airflow's scale of PRs and CI it's actually even likely :D |
It just passed, so unless someone fixed something then it's intermittent. 👍 |
This PR builds on the recently-merged Breeze StatsD Integration PR.
#29449 established a Docker Compose file which launched everything needed to emit metrics over StatsD and view them in Grafana. This PR lays the groundwork for the same process but uses OpenTelemetry instead of StatsD. It will be a required test environment for upcoming work on AIP-49 OpenTelemetry Support for Apache Airflow.
I included a disclaimer in the documentation page that the underlying work to make this useful is pending.
The first commit b84e96c makes some adjustments to the previous StatsD PR in order to make them more reusable. Second commit 8092112 implements the new work.
cc @o-nikolas @vincbeck @syedahsn @vandonr-amz