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

feat!: store wait_time/handle_time in milliseconds #115

Merged
merged 2 commits into from
Nov 23, 2024

Conversation

kbond
Copy link
Member

@kbond kbond commented Nov 15, 2024

@kbond
Copy link
Member Author

kbond commented Nov 15, 2024

@bendavies, @bobvandevijver, mind giving this a review? ❤️

@kbond
Copy link
Member Author

kbond commented Nov 15, 2024

Should I be using bigint? If I'm calculating correct, we can only store ~24 days in milliseconds with a signed int with MySQL.

(pretty sure the answer is yes so I updated to use bigint)

@kbond kbond force-pushed the feat/microseconds branch 2 times, most recently from 2b49974 to 49bef2e Compare November 15, 2024 02:43
@bobvandevijver
Copy link
Contributor

bigint is the way to go indeed, lgtm 👍🏻

@bendavies
Copy link

LGTM, but it's still displaying the times in seconds in the UI?

@bendavies
Copy link

bendavies commented Nov 15, 2024

looks like you can use DateTimeImmutable::format('Uv') to get millseconds, instead of messing with format('U.u') * 1000 ?

https://3v4l.org/ME1X7

@kbond kbond force-pushed the feat/microseconds branch from 49bef2e to 79fb583 Compare November 15, 2024 19:30
@kbond
Copy link
Member Author

kbond commented Nov 15, 2024

looks like you can use DateTimeImmutable::format('Uv') to get millseconds

Excellent find, thanks!

LGTM, but it's still displaying the times in seconds in the UI?

The views show them as human readable durations (ie "4 seconds") but if you hover over, it shows the partial seconds (ie 4.032s).

I decided to have the objects that return durations (ie ProcessedMessage::timeToHandle()) return seconds as a float but what do you think? Should they return the milliseconds as an int then convert to seconds in the UI?

@bendavies
Copy link

Personally, as most of my messages take less than one second, I would prefer to see xxx ms rather than 0s and have to hover.

@bendavies
Copy link

bendavies commented Nov 15, 2024

We could display 0.xxx if less than 1 second?

@kbond
Copy link
Member Author

kbond commented Nov 15, 2024

Ok, I'll see what I can do.

@bobvandevijver
Copy link
Contributor

bobvandevijver commented Nov 16, 2024

Maybe displaying ms if smaller than 5 or 10 seconds, and using s for everything above is nice?

@kbond kbond force-pushed the feat/microseconds branch from 79fb583 to 78cc0e5 Compare November 23, 2024 17:38
@kbond kbond merged commit 4082705 into zenstruck:1.x Nov 23, 2024
46 checks passed
@kbond kbond deleted the feat/microseconds branch November 23, 2024 17:42
@kbond
Copy link
Member Author

kbond commented Nov 28, 2024

Thanks @bendavies! Fixed in #128.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants