Breeze StatsD Integration #29449
Merged
Breeze StatsD Integration #29449
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ferruzzi
commented
Feb 9, 2023
ferruzzi
commented
Feb 9, 2023
ferruzzi
commented
Feb 9, 2023
ferruzzi
commented
Feb 9, 2023
ferruzzi
commented
Feb 9, 2023
ferruzzi
commented
Feb 9, 2023
vincbeck
approved these changes
Feb 9, 2023
Contributor
Author
|
Failing test is |
ferruzzi
commented
Feb 9, 2023
potiuk
approved these changes
Feb 9, 2023
o-nikolas
approved these changes
Feb 10, 2023
Contributor
o-nikolas
left a comment
o-nikolas
left a comment
There was a problem hiding this comment.
Nice, love that this has finally come together after the past couple weeks! 🚀
Contributor
Ohhhh, I've just sent couple links which I personally use before |
ferruzzi
force-pushed
the
ferruzzi/otel/breeze-statsd-integration
branch
from
78834e6 to
1574bfd
Compare
3 tasks
ferruzzi
force-pushed
the
ferruzzi/otel/breeze-statsd-integration
branch
from
1574bfd to
fbbe28f
Compare
Member
|
I am experiencing CI failures on integration tests (eg #29513). That looks related to this PR ( |
Member
|
The root cause is that for commiters on self-hosted runners "all" is used to run tests and in #29513 each integration was run separately in order to accomodate smaller memory on public runners. Let me see if I can fix it quickly. |
Member
|
Fix is coming. It was a mistake in the original implementation @ferruzzi that your change revealed. |
potiuk
added a commit
to potiuk/airflow
that referenced
this pull request
Feb 13, 2023
The change apache#29449 made a distinction between "all" and "available" tests and it turned out that "All" test type uses "available" tests. The name "all" is actually wrong now. It should be renamed to be "all-with-tests" or similar, but this should be done in a separate PR
potiuk
added a commit
that referenced
this pull request
Feb 13, 2023
The change #29449 made a distinction between "all" and "available" tests and it turned out that "All" test type uses "available" tests. The name "all" is actually wrong now. It should be renamed to be "all-with-tests" or similar, but this should be done in a separate PR
potiuk
added a commit
to potiuk/airflow
that referenced
this pull request
Feb 16, 2023
The "all" alias outgrew original meaning with adding statsd integration in apache#29449 which resulted in a problem fixed by apache#29517. The "all" tests as result was not "all" but "all that could be tested". This PR adds "all-testable" alias which as opposed to "all" does not contain "statsd" - it also renames some of the vars/arrays to reflect it.7
potiuk
added a commit
that referenced
this pull request
Feb 17, 2023
The "all" alias outgrew original meaning with adding statsd integration in #29449 which resulted in a problem fixed by #29517. The "all" tests as result was not "all" but "all that could be tested". This PR adds "all-testable" alias which as opposed to "all" does not contain "statsd" - it also renames some of the vars/arrays to reflect it.7
ephraimbuddy
pushed a commit
that referenced
this pull request
Mar 8, 2023
The change #29449 made a distinction between "all" and "available" tests and it turned out that "All" test type uses "available" tests. The name "all" is actually wrong now. It should be renamed to be "all-with-tests" or similar, but this should be done in a separate PR (cherry picked from commit 72c3817)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Running
breeze start-airflow --integration statsdwill launch Breeze along with docker containers for statsd, Prometheus, and Grafana. This allows us to test Airflow's statsd metrics emitting.The general workflow here is that Breeze/Airflow will push metrics to statsd. Prometheus pulls from stats, and Grafana pulls from Prometheus. I found a lovely diagram and a pretty good explanation here which I used as the basis for this change.
I've left a few notes and questions in the review
cc @o-nikolas @vincbeck @syedahsn @vandonr-amz
cc @Taragolis - Since you helped with the research, you might want to have a look.