-
Notifications
You must be signed in to change notification settings - Fork 53
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
chore: Better timing and requestId detail for slower store db queries #2994
Conversation
This PR may contain changes to database schema of one of the drivers. If you are introducing any changes to the schema, make sure the upgrade from the latest release to this change passes without any errors/issues. Please make sure the label |
You can find the image built from this PR at
Built from 1b3419e |
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.
Amazing addition! Thanks so much!
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.
LGTM, just some questions I made.
@@ -41,6 +41,7 @@ method getMessagesV2*( | |||
endTime = none(Timestamp), | |||
maxPageSize = DefaultPageSize, | |||
ascendingOrder = true, | |||
requestId: string, |
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.
If requestId is mandatory here, would it be better at the front of the args?
Maybe it is just my old habit, I know it works in nim though...
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.
If requestId is mandatory here, would it be better at the front of the args? Maybe it is just my old habit, I know it works in nim though...
Ah yes you are right! Nevertheless let me take the license to leave it as is given that we will deprecate this code soon. Thanks for the comment!
@@ -55,6 +56,7 @@ method getMessages*( | |||
hashes = newSeq[WakuMessageHash](0), | |||
maxPageSize = DefaultPageSize, | |||
ascendingOrder = true, | |||
requestId = "", |
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.
Why isn't that mandatory to set here if it is with V2 method?
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.
Why isn't that mandatory to set here if it is with V2 method?
Yes in this case I set it a default value to prevent changing too many tests that we will remove shortly.
004c13e
to
0274ee8
Compare
Description
Better track the
requestId
that comes fromstore
requests to lower layers (archive/database) so that we can link requests with slow queriesChanges
requestId
value from store layer to the database layer.pgasyncpool.nim
: Print information for queries slower than 2 seconds.client.nim
: allow sending customrequestId
.requested
vs the sentrequestId
and they failed because we are adding therequestId
field in theArchiveQuery
object.