You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "htpawel (via GitHub)" <gi...@apache.org> on 2024/03/06 12:32:57 UTC
[PR] Fix dag task scheduled and queued duration metrics [airflow]
htpawel opened a new pull request, #37936:
URL: https://github.com/apache/airflow/pull/37936
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
From statsd documentation ([calling-timing-manually](https://statsd.readthedocs.io/en/v3.3/timing.html#calling-timing-manually)):
```statsd = StatsClient()
# Pass milliseconds directly
start = time.time()
time.sleep(3)
# You must convert to milliseconds:
dt = int((time.time() - start) * 1000)
statsd.timing('slept', dt)
# Or pass a timedelta
start = datetime.utcnow()
time.sleep(3)
dt = datetime.utcnow() - start
statsd.timing('slept', dt)
```
timedelta object must be in milliseconds rather than seconds. You can check other examples in airflow code also. Then statsd lib converts it to seconds before exporting metric.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-1987341144
cc: @ferruzzi @Bowrna -> I remember there were discussions about it in the past . Did we decide to leave it in s for back-compatibility ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2168222276
Ok. Let's treat it as a bugfix and merge.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-1980771317
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:
- Pay attention to the quality of your code (ruff, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/dev/breeze/doc/README.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#coding-style-and-best-practices).
- Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "Bowrna (via GitHub)" <gi...@apache.org>.
Bowrna commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2014649033
> cc: @ferruzzi @Bowrna -> I remember there were discussions about it in the past . Did we decide to leave it in s for back-compatibility ?
timedelta object statsd handles internally by converting to milliseconds. But there are some case where we explicitly pass the seconds instead of timedelta object and it is stored in statsd as seconds. so I added a fix with different key name where the newly added key name persists the data in milliseconds while the older key stays for backward compatibility with a deprecation message.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "htpawel (via GitHub)" <gi...@apache.org>.
htpawel commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-1988302014
@potiuk @dirrao
I searched entire airflow 2.7.1 code for Stats.timing and except this place only we are providing delta correctly. Only here we are calling .total_seconds() on delta which is incorrect. Current metrics documentation states that those two metrics are in seconds, but they are not. Right now to have useful data (in seconds) you need to multiply metric value by 1000 - which is obvious bug and mismatch between documentation and reality and should be fixed asap imo.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "HTRafal (via GitHub)" <gi...@apache.org>.
HTRafal commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2036568387
So `dag.<dag_id>.<task_id>.queued_duration` documented as `Seconds a task spends in the Queued state, before being Running` is actually a value in **kiloseconds** :D. You have to multiply it by 1000 to get a value in seconds.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "htpawel (via GitHub)" <gi...@apache.org>.
htpawel commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2024592128
Ok now I am not 100% sure it emits it in seconds (it will require further investigation), but anyway it is 100% clear that it expects time delta or milliseconds as input, not seconds (it is even said in the code you sent in comment), to work properly.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2038296339
@Bowrna @HTRafal @tanvn - Does this proposed change look like it will address your comments? If you all confirm, I'll approve and we can get it merged. The code looks fin to me, just want to confirm that it will fix what you are seeing.
@htpawel - Can you think of some kind of unit test on (Stats.timer, satsd_logger.timer and/or otel_logger.timer) that asserts the expected output format? Maybe create a timer, sleep(1), and assert the duration is how we want it to look?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "htpawel (via GitHub)" <gi...@apache.org>.
htpawel commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-1988315034
@dirrao
Nope, documentation is correct I believe, we want it to be seconds, but right now they are not seconds in result because of above bug
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "htpawel (via GitHub)" <gi...@apache.org>.
htpawel commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2070086572
@ferruzzi @o-nikolas
What test did you have in mind? I can't think of any to accomplish this:
> Other than adding a unittest to ensure this doesn't happen again. I'm happy to approve the change.
Developers should use Stats.timer() context manager as much as they can, but there are scenarios like the one above when Stats.timing() is needed. Then developers should pass either timedelta object or milliseconds, but it is not possible to validate this input, you can't know if given number is milliseconds or seconds. That's why statsd third party module is not able to validate that also - it is in documentation.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2144587358
> Sounds like a breaking bug-fix to me .
If this is the case we probably should add a newsfragmant that warn/explain users
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "htpawel (via GitHub)" <gi...@apache.org>.
htpawel commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2111755394
> > What test did you have in mind? I can't think of any to accomplish this
>
> If you are trying to make sure the metric is emitted in seconds and not milliseconds, maybe start a timer, sleep(1), and assert that Stats.timer() was called with a value of 1 and not 1000?
I've already made sure that metric is emitted in milliseconds (if you use it correctly, so pass milliseconds or delta) but if someone will pass seconds it will emit seconds obviously (with ms suffix..) and there is no test / mechanism to prevents this automatically in any way.. Developers just must know that Statsd expects milliseconds or delta object only, that's convention.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "Bowrna (via GitHub)" <gi...@apache.org>.
Bowrna commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2024405440
> @vandonr You probably didn't read the topic fully or misunderstood. Airflow indeed emits most of metrics (or all of them) in seconds, and those two should also be emitted in seconds like you wanted to achieve and like it is stated in documentation and like I also want it to be (and everyone I suppose). But unfortunately they are NOT right now. It is because you need to pass milliseconds or delta time object to Statsd timing (then it will emit metric in seconds). But you are passing seconds which is incorrect. Check all other places in Airflow code - delta time object is passed. This is obvious bug which we need to fix with above bugfix.
@htpawel https://github.com/apache/airflow/issues/33426#issuecomment-1790610192
Please check here. Statsd timing internally sends the stat in milliseconds. I am sharing this here as you told Statsd timing emit metric in seconds. Does stated sends the data in milliseconds but emits in seconds?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2032719082
Sounds like a breaking bug-fix to me . I.e. yes - we know it will be breaking, but it was broken in the first place anyway. @ferruzzi - WDYT? I'd merge it and add significant note about 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "htpawel (via GitHub)" <gi...@apache.org>.
htpawel commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2023979633
@vandonr You probably didn't read the topic fully or misunderstood. Airflow indeed emits most of metrics (or all of them) in seconds, and those two should also be emitted in seconds like you wanted to achieve and like it is stated in documentation and like I also want it to be (and everyone I suppose). But unfortunately they are NOT right now. It is because you need to pass milliseconds or delta time object to Statsd timing (then it will emit metric in seconds). But you are passing seconds which is incorrect. Check all other places in Airflow code - delta time object is passed. This is obvious bug which we need to fix with above bugfix.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2113463312
> Developers just must know that Statsd expects milliseconds or delta object only, that's convention.
That's horrible. So we don't have any way of knowing, catching, or preventing this from happening again? Alright. I'll approve it as-is then.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-1989197373
This would break the dashboard of any user currently monitoring that metric and their timers will suddenly show 1000x longer durations, right?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "tanvn (via GitHub)" <gi...@apache.org>.
tanvn commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2105444490
If we are not going to add any unit tests, I think it is acceptable to merge this PR 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2168222543
Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "htpawel (via GitHub)" <gi...@apache.org>.
htpawel commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2165475953
@eladkal seems like once again there is some flaky test failing, not related to my change. Could someone override its result or rerun only it or fix? Otherwise we will never merge such a simple fix :/
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "htpawel (via GitHub)" <gi...@apache.org>.
htpawel commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2150075639
@eladkal @ferruzzi done
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "vandonr (via GitHub)" <gi...@apache.org>.
vandonr commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2023955101
I don't have anything against converting this metric to milliseconds, I believe I wasn't aware of that statsd recommendation when I wrote that code.
However, there are plenty of other timers that are emitted in seconds in airflow, and I think if we make a migration effort, we might as well migrate all at once rather than little by little.
We must be aware of the high impact this can have for users as well: if they have threshold alerts on those metrics, they will certainly ring when they see the metric go x1000.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "tanvn (via GitHub)" <gi...@apache.org>.
tanvn commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2033357051
I has been using Airflow with statsd-exporter for quite a long time and to be honest, while I found the document about [the timers ](https://airflow.apache.org/docs/apache-airflow/stable/administration-and-deployment/logging-monitoring/metrics.html#timers) sometimes inconsistent, the values exported by Statsd which we are using look good to me (except task scheduled and queued duration metrics as I have not used them yet)
So +1 for this PR and a fix in the document 🙇
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2037849843
KILOSECONDS? Why the heck used KILOSECONDS for anything?
It looks like consensus is that we don't have to deal with deprecation, so that's great.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "tanvn (via GitHub)" <gi...@apache.org>.
tanvn commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2039804076
> @Bowrna @HTRafal @tanvn - Does this proposed change look like it will address your comments? If you all confirm, I'll approve and we can get it merged. The code looks fin to me, just want to confirm that it will fix what you are seeing.
>
> @htpawel - Can you think of some kind of unit test on (Stats.timer, satsd_logger.timer and/or otel_logger.timer) that asserts the expected output format? Maybe create a timer, sleep(1), and assert the duration is how we want it to look? I know this was a really small thing and it really dragged out, sorry about that.
Yes, this PR changes the duration to a timedelta, which is correct.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "Bowrna (via GitHub)" <gi...@apache.org>.
Bowrna commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2039248829
> @Bowrna @HTRafal @tanvn - Does this proposed change look like it will address your comments? If you all confirm, I'll approve and we can get it merged. The code looks fin to me, just want to confirm that it will fix what you are seeing.
>
> @htpawel - Can you think of some kind of unit test on (Stats.timer, satsd_logger.timer and/or otel_logger.timer) that asserts the expected output format? Maybe create a timer, sleep(1), and assert the duration is how we want it to look? I know this was a really small thing and it really dragged out, sorry about that.
yes it does address the comments I shared.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2040259511
Cool. @htpawel - If you can add that quick unit test, I'll approve and merge this.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2087212321
> Developers should use Stats.timer() context manager as much as they can, but there are scenarios like the one above when Stats.timing() is needed. Then developers should pass either timedelta object or milliseconds, but it is not possible to validate this input, you can't know if given number is milliseconds or seconds. That's why statsd third party module is not able to validate that also - it is in documentation.
ACK, running the workflow again after the latest merge from main
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "htpawel (via GitHub)" <gi...@apache.org>.
htpawel commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2165354889
@ferruzzi who has write access and can merge this?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2021462251
Hey @vandonr - since you are the original author of #30612 and likely will be easier to get all the context back - maybe you can comment on that one? Is that the right fix ? Or maybe @ferruzzi @o-nikolas - since you were around when it was implemented - is there any reason those durations should be kept as they are (and maybe there is a constructive proposal how to handle that case. It looks like we have somewhat long lasting issues that those new metrics introduced in #30612 caused and possibly we should find a way how to address it before 2.9.0 rc2 is 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "dirrao (via GitHub)" <gi...@apache.org>.
dirrao commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-1988306459
> @potiuk @dirrao
> I searched entire airflow 2.7.1 code for Stats.timing and except this place only we are providing delta correctly. Only here we are calling .total_seconds() on delta which is incorrect. Current metrics documentation states that those two metrics are in seconds, but they are not. Right now to have useful data (in seconds) you need to multiply metric value by 1000 - which is obvious bug and mismatch between documentation and reality and should be fixed asap imo.
@htpawel,
I have checked the documentation. If so, then documentation needs to updated.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-1992231889
I never said don't fix it. It's just a matter of if we call it a bugfix and fix it now, creating a breaking change and sending users scrambling to figure out why their queues are suddenly 1000 times longer, or if we have to go through the deprecation process so users are informed.
Since this is documented as `seconds`, I think we can call it a bugfix and just do it, but it's still something that needs to be decided.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "htpawel (via GitHub)" <gi...@apache.org>.
htpawel commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-1990952084
@ferruzzi
If someone is using it right now and applied temporary workaround (added *1000 to metric value) with TODO comment like we did - yes, it might break their dashboard. But I don't think it's the reason to not fix this :D First of all, documentation says it is seconds as result, so documentation is lying to us right now. Second, it will be in release notes so if someone is updating airflow without reading them - it's their fault. Third, it's new metric so I doubt many people are using it. Some of them might find its result useless (like I did at the beginning) and drop, some of them might just applied it and be happy they don't have any queues because they believe in documentation that those are seconds. In general it does not look good if you don't want to fix your code to work properly just because people might have already made ugly workarounds themselves to fix your bug IMHO.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "htpawel (via GitHub)" <gi...@apache.org>.
htpawel commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2024692435
Ok, I did further investigation and now I know everything :D
Statsd by convention **should always** emit milliseconds. (See `ms` in their code: `self._send_stat(stat, '%0.6f|ms' % delta, rate)`). And that is why they are requesting time delta object or value in milliseconds: (in their code: `delta can be either a number of milliseconds or a timedelta.`). But later you need to do something with those metrics, so we (and probably most of the people) use statsd-exporter from prometheus. And in statsd-exporter documentation you can read `Timers will be accepted with the ms statsd type. Statsd timer data is transmitted in milliseconds, while Prometheus expects the unit to be seconds. The exporter converts all timer observations to seconds.`. So that is why I thought it is emitted in seconds, I was just checking statsd-exporter /metrics endpoint. So it turns out that not this, but other metrics are wrong in documentation, eg. dagrun.schedule_delay.<dag_id> as it passes time delta to Stats.timing so emits milliseconds but airflow documentation say
s those are seconds. And above two from this PR are indeed emitted in seconds, but obviously it is not correct either as those should be milliseconds and are emitted with `ms` suffix. So there are more things f*cked up generally. Timing/Timer should always receive milliseconds value or time delta object and it emits milliseconds, which can be later consumed by statsd-exporter and exporter in seconds for prometheus to scrape them.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2037914197
> It looks like consensus is that we don't have to deal with deprecation, so that's great.
Agree.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2110714915
> What test did you have in mind? I can't think of any to accomplish this
If you are trying to make sure the metric is emitted in seconds and not milliseconds, maybe start a timer, sleep(1), and assert that Stats.timer() was called with a value of 1 and not 1000?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix dag task scheduled and queued duration metrics [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #37936:
URL: https://github.com/apache/airflow/pull/37936
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org