-
Notifications
You must be signed in to change notification settings - Fork 188
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
Track transaction time #798
base: main
Are you sure you want to change the base?
Conversation
fea5249
to
cce7f8b
Compare
@@ -29,6 +29,9 @@ | |||
expect(results["total_query_time"].to_i).to be_within(200).of(750) | |||
expect(results["avg_query_time"].to_i).to be_within(50).of(250) | |||
|
|||
expect(results["total_xact_time"].to_i).to be_within(200).of(750) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is failing in CI:
1) Stats SHOW STATS clients connect and make one query updates *_query_time and *_wait_time
Failure/Error: expect(results["total_xact_time"].to_i).to be_within(200).of(750)
expected 1203 to be within 200 of 750
# ./stats_spec.rb:32:in `block (4 levels) in <top (required)>'
It passes locally for me inside the dev container, but I wonder if that shows that this approach is flawed.
Having said that, I do see other specs fail some of the time, like this, which I don't think I broke:
1) Stats SHOW POOLS bad database name does not change any stats
Failure/Error: expect(results["sv_idle"]).to eq("1")
expected: "1"
got: "2"
(compared using ==)
# ./stats_spec.rb:72:in `block (4 levels) in <top (required)>'
Ha, yes, this is definitely wrong. I added a test that runs more queries (not in a transaction), and it fails with:
When this metric should not have increased in that time. |
cce7f8b
to
04e346d
Compare
04e346d
to
117b686
Compare
# Average is 125ms in the current stats period | ||
expect(results["avg_query_time"].to_i).to be_within(25).of(125.0) | ||
|
||
# Autocommit transactions still increase the count and time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was already the case, and is the same as PgBouncer: https://github.com/pgbouncer/pgbouncer/blob/e2a2a682fe9069366fca970ec2492d774b4f6b40/test/test_admin.py#L74-L75
@drdrsh could you try again, please? I'm seeing an issue when running pgbench in the dev container where both |
I'm not sure if this is the correct way to track it (on the server, with an always-outdated timestamp), but I couldn't see an obvious way to track this on the client side, and logically a transaction must be tied to a particular server connection, right?Now tracking on the client side.Closes #114 (
total_query_time
is already implemented).