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!: add wait/handle time to ProcessedMessage #105

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

kbond
Copy link
Member

@kbond kbond commented Nov 10, 2024

Fixes #97.

This is a BC break and will require existing apps to create a migration. The migration would need to either clear the existing ProcessedMessage's or calculate the values when adding the fields.

This isn't ideal but feels like the best way to eliminate the timestamp diff db logic that DB platforms all handle differently.

TODO:

  • UPGRADE.md

@kbond
Copy link
Member Author

kbond commented Nov 11, 2024

💪🏻 Ok, averageWaitTime & averageHandlingTime work consistently across sqlite/mysql/postgres.

I think this is the best way to handle but @Chris53897, @SherinBloemendaal, @bobvandevijver, any thoughts?

@kbond kbond force-pushed the processed-message-times branch from 07ffd52 to 1606748 Compare November 11, 2024 23:47
@kbond kbond force-pushed the processed-message-times branch from 1606748 to 23cc03e Compare November 11, 2024 23:53
@kbond kbond merged commit f7eccc4 into zenstruck:1.x Nov 12, 2024
38 checks passed
@kbond kbond deleted the processed-message-times branch November 12, 2024 02:42
@bobvandevijver
Copy link
Contributor

LGTM 👍🏻

@bendavies
Copy link

bendavies commented Nov 13, 2024

@kbond how about storing micro seconds for sub second precision?
many of my messages show 0 seconds for handled time, it would be great to see milliseconds!!
maybe squeeze in before the 0.5 release?

@kbond
Copy link
Member Author

kbond commented Nov 13, 2024

I was thinking about this.

My only concern is timestamp fields don't store partial seconds (IIRC). Maybe not an issue but the timestamps and wait/handle time's won't match up.

Thinking to store as seconds * 1000 instead of a decimal field to avoid any inconsistencies between db platform (not sure if there are any).

Need to make this decision before 0.5.0 to avoid a BC break for existing data...

@bendavies
Copy link

bendavies commented Nov 13, 2024

here i used decimal(16,6) with DateTimeImmutable->format('U.u') and it seemed fine on postgres and mysql
SymfonyCasts/messenger-monitor-bundle#43

@kbond
Copy link
Member Author

kbond commented Nov 13, 2024

Ok cool, I have tests for different db platforms now so let's see!

@kbond
Copy link
Member Author

kbond commented Nov 13, 2024

Why did you use decimal instead of float for the type? From what I'm seeing, a float would require less bytes than a 16,6 decimal.

@kbond
Copy link
Member Author

kbond commented Nov 13, 2024

Ok, good to know! Decimal it is

@bendavies
Copy link

bendavies commented Nov 13, 2024

i'm actually not sure if my comment back then was a typo and i meant postgres.
but compare these on postgres and you'll see floats suffer from the same problem as php with floats.

SELECT avg(n) FROM unnest('{0.1,0.2}'::float[]) n; # 0.15000000000000002
SELECT avg(n) FROM unnest('{0.1,0.2}'::decimal[]) n; # 0.15000000000000000000

@kbond
Copy link
Member Author

kbond commented Nov 13, 2024

I watched this also: https://planetscale.com/learn/courses/mysql-for-developers/schema/decimals

Which talks about the same thing but... For milli or even micro seconds, this approximation would be fine, no?

@bendavies
Copy link

sure it'll use more bytes for storage, but is that a concern given the other columns on this table which are far larger?
that said, this does seem a valid occasion where an approximation might be seemed good enough.

@bobvandevijver
Copy link
Contributor

Why not just store the milliseconds (or even micro) in the int field? I very much doubt that (assuming 64 bit signed, bigint in mariadb and postgres) wait time and handle time would ever by larger than 2.56*10^9 hours.

Unsigned ints are not supported on postgres, so that is not an option.

Personally I always try to stay away from floats whenever possible.

@kbond
Copy link
Member Author

kbond commented Nov 14, 2024

Ok, I was also thinking storing the milliseconds. I think I'll do that. I doubt there's a need for knowing down to the microsecond.

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.

PostgreSQL averageWaitTime & averageHandlingTime are not working
3 participants