You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/05/03 00:56:21 UTC
[GitHub] [airflow] Acehaidrey opened a new pull request #8680: Add metric for start/end task run
Acehaidrey opened a new pull request #8680:
URL: https://github.com/apache/airflow/pull/8680
The purpose of this metric is to have a value for when a stat begins and when it ends to set up different rules on these stats in the prometheus side/statsd side etc. This allows for easily building reports as well and is a minor change to enable teams, while also allowing users to black list these stats if they do not want it (black listing changes already committed with community team changes).
---
Make sure to mark the boxes below before creating PR: [x]
- [x ] Description above provides context of the change
- [ x] Unit tests coverage for changes (not needed for documentation changes)
- [ x] Target Github ISSUE in description if exists
- [ x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [x ] Relevant documentation is updated including usage instructions.
- [ x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
---
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#discussion_r419825461
##########
File path: airflow/models/taskinstance.py
##########
@@ -1080,6 +1081,8 @@ def signal_handler(signum, frame):
self.handle_failure(e, test_mode, context)
raise
+ Stats.incr('ti.complete.{}.{}.{}'.format(task.dag_id, task.task_id, self.state), 1, 1)
Review comment:
Just wondering, does it make more sense to put it here rather than after the success callbacks on lines 1087-1093, inside a `finally` block?
##########
File path: airflow/models/taskinstance.py
##########
@@ -1080,6 +1081,8 @@ def signal_handler(signum, frame):
self.handle_failure(e, test_mode, context)
raise
+ Stats.incr('ti.complete.{}.{}.{}'.format(task.dag_id, task.task_id, self.state), 1, 1)
Review comment:
To make these names as meaningful as possible, `starts` and `finishes` (or similar) might be more clear, so it's clear that it's referring to a counter and not a timestamp.
https://airflow.apache.org/docs/stable/metrics.html#counters
* `ti_starts_<dag_id>_<task_id>`
* `ti_finishes_<dag_id>_<task_id>_<state>`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Acehaidrey commented on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
Acehaidrey commented on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-636365468
@turbaszek my apologies - check now ( once build is successful that is )
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Acehaidrey commented on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
Acehaidrey commented on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-624415287
I made the following changes:
* changed the word complete to finish
* added test to verify ti.start is also in the calls for the function
* updated documentation reflecting those changes
I do the separation on the `.` because can do different grouping aggregates on statsd/prometheus side vs using the _
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Acehaidrey closed pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
Acehaidrey closed pull request #8680:
URL: https://github.com/apache/airflow/pull/8680
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Acehaidrey commented on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
Acehaidrey commented on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-634177578
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Acehaidrey commented on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
Acehaidrey commented on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-632927893
@dimberman yes I can rebase and try again, it seems the tests are failing to some connection issue with hive which are not related to this. I'm thinking its intermittent ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins edited a comment on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
jhtimmins edited a comment on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-632485204
Hi @Acehaidrey, sorry been heads down with 3.8 stuff. Happy to take a look tomorrow. thanks for pinging me. Same for @dimberman for your other PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Acehaidrey commented on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
Acehaidrey commented on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-632969149
hey @jhtimmins thanks for the update! That is okay with me. I just don't know how to make this test pass otherwise and @dimberman has asked for it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-632485204
Hi @Acehaidrey, sorry been heads down with 3.8 stuff. Happy to take a look tomorrow. thanks for pinging me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Acehaidrey commented on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
Acehaidrey commented on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-625514762
@jhtimmins when you get a chance
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
dimberman commented on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-632926745
@Acehaidrey can you please fix the tests? Will merge when passing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Acehaidrey commented on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
Acehaidrey commented on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-632915369
not a problem @jhtimmins I know you are all busy! please let me know if I can help ever! would love to be closer to community :) but regarding this PR was there anything else, I did all your changes you asked, so want to make sure.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Acehaidrey edited a comment on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
Acehaidrey edited a comment on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-632969149
hey @jhtimmins thanks for the update! That is okay with me. I just don't know how to make this test pass otherwise and @dimberman has asked for it. do you think we can commit this without the passing for now?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Acehaidrey commented on a change in pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
Acehaidrey commented on a change in pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#discussion_r427740455
##########
File path: airflow/models/taskinstance.py
##########
@@ -1080,6 +1081,8 @@ def signal_handler(signum, frame):
self.handle_failure(e, test_mode, context)
raise
+ Stats.incr('ti.complete.{}.{}.{}'.format(task.dag_id, task.task_id, self.state), 1, 1)
Review comment:
@ashb mind taking a look at thiss so we can close this oout. did the changes asked
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
dimberman commented on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-635994117
Hi @Acehaidrey. The tests are working now. Can you please rebase?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Acehaidrey edited a comment on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
Acehaidrey edited a comment on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-624415287
@jhtimmins
I made the following changes:
* changed the word complete to finish
* added test to verify ti.start is also in the calls for the function
* updated documentation reflecting those changes
I do the separation on the `.` because can do different grouping aggregates on statsd/prometheus side vs using the _
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-632960556
@Acehaidrey Not sure about increasing the memory, but several of us are actively working to reduce the image size to solve that issue. Hard to give an exact timeline, but can say that it's in progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Acehaidrey commented on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
Acehaidrey commented on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-636209212
@dimberman done! it has all passed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
dimberman commented on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-634178314
Hi @Acehaidrey I'm currently fixing an issue where travis can't seem to run the k8s tests. I'm trying to avoid merging anything until that is fixed. I'll make sure to come back to this when that merges.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Acehaidrey commented on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
Acehaidrey commented on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-632944238
hey @dimberman @ashb I am getting some exit code 137 meaning oom issues. that is why tests are failing. but wondering if you knew where to touch those containers to request more memory to make it pass?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-636319193
Hi @Acehaidrey it seems that you have performed merge instead of rebase. Can you please do `git rebase apache/master` (assuming that your Apache remote is called apache)?
https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#id39
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Acehaidrey commented on a change in pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
Acehaidrey commented on a change in pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#discussion_r420502117
##########
File path: airflow/models/taskinstance.py
##########
@@ -1080,6 +1081,8 @@ def signal_handler(signum, frame):
self.handle_failure(e, test_mode, context)
raise
+ Stats.incr('ti.complete.{}.{}.{}'.format(task.dag_id, task.task_id, self.state), 1, 1)
Review comment:
change it to finished
I do the separation on the `.` because can do different grouping aggregates on statsd/prometheus side vs using the `_`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Acehaidrey edited a comment on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
Acehaidrey edited a comment on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-632915369
not a problem @jhtimmins I know you are all busy! please let me know if I can help ever! would love to be closer to community :) but regarding this PR was there anything else, I did all your changes you asked, so want to make sure. CC @ashb or @dimberman for approval
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Acehaidrey commented on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
Acehaidrey commented on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-623739563
@jhtimmins if you don't mind when you get a chance to look at some of this and share your thoughts as well
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek merged pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
turbaszek merged pull request #8680:
URL: https://github.com/apache/airflow/pull/8680
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Acehaidrey commented on pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
Acehaidrey commented on pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#issuecomment-632478686
@ashb @jhtimmins mind helping me close this one out?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Acehaidrey commented on a change in pull request #8680: Add metric for start/end task run
Posted by GitBox <gi...@apache.org>.
Acehaidrey commented on a change in pull request #8680:
URL: https://github.com/apache/airflow/pull/8680#discussion_r420502117
##########
File path: airflow/models/taskinstance.py
##########
@@ -1080,6 +1081,8 @@ def signal_handler(signum, frame):
self.handle_failure(e, test_mode, context)
raise
+ Stats.incr('ti.complete.{}.{}.{}'.format(task.dag_id, task.task_id, self.state), 1, 1)
Review comment:
change it to finished . and moved to a finally so it is better this way
I do the separation on the `.` because can do different grouping aggregates on statsd/prometheus side vs using the `_`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org