htpawel opened a new pull request, #37936:
URL: https://github.com/apache/airflow/pull/37936
---
From statsd documentation
([calling-timing-manually](https://statsd.readthedocs.io/en/v3.3/timing.html#calling-timing-manually)):
```statsd = StatsClient()
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 internal
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 @fe
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
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 lik
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 li
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 (i
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' % de
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
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/
HTRafal commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-2036568387
So `dag...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
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 automate
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
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, jus
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 w
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 wa
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 lo
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'
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 Apac
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
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 del
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
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 th
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
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 sudd
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 appr
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 ei
tanvn commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-210590
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
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 tim
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
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
prevent
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 S
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 comm
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
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
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.
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-unsubscr...@airflow.a
potiuk commented on PR #37936:
URL: https://github.com/apache/airflow/pull/37936#issuecomment-216876
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
38 matches
Mail list logo
