-
Notifications
You must be signed in to change notification settings - Fork 558
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
Print record timestamp in compact record logger #10170
Print record timestamp in compact record logger #10170
Conversation
When troubleshooting issues or writing tests for Timer related topics, we're often not only interested in the Duedate of the timer, but also in the Timestamp of the records. Since these timestamps are generally irrelevant for other types of tests, it makes sense to only log them when there are Timer records present. As a format I've chosen a small but simple form (HH:mm:ss). If needed we could investigate more advanced formatting in the future. E.g. a format that only shows the least amount of data: only seconds if those matter, or include hours if those matter. For now, this short form in combination with hiding it when its not useful seems useful and easy to build. Timestamps are naturally found at the start of log lines, so I've chosen to prefix each record summary with it.
If there are timer events, the compact record logger's Legend should contain [Timestamp], so readers understand the summary. But, the Timestamp isn't always part of the compact log. In that case, the [Timestamp] should also be removed from the Legend.
If there are multiple partitions, the compact record logger's Legend should contain [Partition] and each key will contain a [Pn] prefix, so readers understand the summary. But, there aren't always multiple partitions, and then the partition isn't part of the compact log. In that case, the [Partition] should also be removed from the Legend. I've also split up the Legend construction a bit further, to keep it somewhat readable.
Test Results 853 files + 1 853 suites +1 1h 35m 7s ⏱️ - 9m 6s For more details on these failures, see this check. Results for commit 42936ee. ± Comparison against base commit ec06278. ♻️ This comment has been updated with latest results. |
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.
I like the changes 🏆 Nothing to add LGTM
bors merge |
Build succeeded: |
Successfully created backport PR #10175 for |
Successfully created backport PR #10176 for |
Description
When troubleshooting issues (or writing tests) for Timer related topics, we're often interested in both the Timer record's Duedate, as well as the actual time that the record was written, i.e. the record's Timestamp.
This PR adds the Timestamp to the compact record logger when there are Timer records on the recorded log.
While working on this, I also noticed that the compact record logger's Legend could be improved to only include Partition related info when the partition is actually printed as well (i.e. when there are multiple partitions).
This is another PR in a series of QoL improvements I've recently made to the compact record logger.
Before
After
With Timer records:
Without Timer records:
When there are multiple partitions:
Related issues
relates to #10169
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
backport stable/1.3
) to the PR, in case that fails you need to create backports manually.Testing:
Documentation:
Please refer to our review guidelines.