Skip to content

Conversation

@justonedev1
Copy link
Collaborator

I added timestamp to all gRPC calls replies which is int64 unix timestamp in milliseconds.

@justonedev1 justonedev1 requested a review from knopers8 March 13, 2025 16:47
Copy link
Collaborator

@knopers8 knopers8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! My only concern is related to the point in time that these timestamps are taken. Sometimes it is at the moment of receiving the request, sometimes just before replying. If the request processing takes very little time, I guess it does not matter, but otherwise it could potentially be confusing. How about taking the timestamp always at the beginning of the call? This way it should bring minimal boilerplate code, but keep the behaviour consistent.

@justonedev1 justonedev1 requested a review from knopers8 March 14, 2025 18:31
@justonedev1
Copy link
Collaborator Author

I changed the code to address what you said. Please don't merge this after approving, I need to squash the fixup commit after.

@justonedev1
Copy link
Collaborator Author

uhm... I just now noticed, that I cannot read and put timestamp at the end of the call not beginning. Sorry Should I redo it, or is this fine?

@knopers8
Copy link
Collaborator

uhm... I just now noticed, that I cannot read and put timestamp at the end of the call not beginning. Sorry Should I redo it, or is this fine?

At the end is also fine with me, as long as the behaviour is consistent. But since George is back, perhaps you can check with him.

@justonedev1 justonedev1 requested a review from knopers8 March 17, 2025 14:35
knopers8
knopers8 previously approved these changes Mar 19, 2025
@justonedev1
Copy link
Collaborator Author

it can be merged now. I applied all fixup comments, so history is clean a george cofirmed that everything is working for him

@knopers8 knopers8 merged commit 71dbbe7 into master Mar 19, 2025
2 checks passed
@knopers8 knopers8 deleted the OCTRL-991 branch March 19, 2025 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants