Skip to content
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

Merged
merged 4 commits into from
Feb 15, 2023

Conversation

ferruzzi
Copy link
Contributor

@ferruzzi ferruzzi commented Feb 14, 2023

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

Copy link
Contributor

@vincbeck vincbeck left a 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

Comment on lines 60 to 63
- INTEGRATION_STATSD=true
- AIRFLOW__METRICS__STATSD_ON=True
- AIRFLOW__METRICS__STATSD_HOST=statsd-exporter
- AIRFLOW__METRICS__STATSD_PORT=9125
Copy link
Contributor

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

Copy link
Contributor Author

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

scripts/ci/docker-compose/otel-collector-config.yml Outdated Show resolved Hide resolved
@ferruzzi ferruzzi force-pushed the ferruzzi/otel/breeze-otel-integration branch from 8092112 to 2441a90 Compare February 14, 2023 17:52
@ferruzzi
Copy link
Contributor Author

Sorry for the force, I had to rebase

@ferruzzi ferruzzi force-pushed the ferruzzi/otel/breeze-otel-integration branch from 2441a90 to 3786b04 Compare February 14, 2023 18:04
@ferruzzi
Copy link
Contributor Author

Main is broken again, should have not rebased yet I guess.

@potiuk
Copy link
Member

potiuk commented Feb 14, 2023

looks transient error with httpbin. rerunning.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woooho.. progress.

@ferruzzi
Copy link
Contributor Author

Cool. I peeked in the PRs list and there were a LOT of them failing so I assumed it was another main hiccup.

@potiuk
Copy link
Member

potiuk commented Feb 14, 2023

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.

@ferruzzi
Copy link
Contributor Author

FAILED tests/providers/http/hooks/test_http.py::test_do_api_call_async_retryable_error - aiohttp.client_exceptions.ClientConnectionError: Connection refused: GET http://httpbin.org/non_existent_endpoint

I didn't copy the error message from before, but it looks familiar, I think it's the same transient one @potiuk mentioned.

@ferruzzi ferruzzi force-pushed the ferruzzi/otel/breeze-otel-integration branch from 8b70c06 to a08c2ec Compare February 15, 2023 01:07
@potiuk
Copy link
Member

potiuk commented Feb 15, 2023

FAILED tests/providers/http/hooks/test_http.py::test_do_api_call_async_retryable_error - aiohttp.client_exceptions.ClientConnectionError: Connection refused: GET http://httpbin.org/non_existent_endpoint

I didn't copy the error message from before, but it looks familiar, I think it's the same transient one @potiuk mentioned.

I tihnk this is simply some kind of instability of the http://httpbin.org/ - or maybe they (or their systems) THINK that we are DDOS-ing them with our CI and block the attempt if there are too many.

With Airflow's scale of PRs and CI it's actually even likely :D

@ferruzzi
Copy link
Contributor Author

It just passed, so unless someone fixed something then it's intermittent. 👍

@potiuk potiuk merged commit 95e26d5 into apache:main Feb 15, 2023
1 check passed
@ferruzzi ferruzzi changed the title Breeze OpenTelemetry Integration [AIP-49] Breeze OpenTelemetry Integration Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants