Add timestamp to module logs #641
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
coolreader18
force-pushed
the
noa/module-log-ts
branch
from
6d312e7 to
400ad83
Compare
|
(the tests needed updating because they separated the log prefix from the message by the 4th colon, but the timestamp has 2 colons in it) |
cloutiertyler
approved these changes
Dec 8, 2023
There was a problem hiding this comment.
This looks good to me. Initially my thought was to go with reducer time, but actually I think wall-time is the better solution to go with here at least for now. My reasoning is that a random user is not going to understand why all of their logs in the same reducer are running at the same exact millisecond.
Ultimately I would recommend stuffing both into the Record type.
coolreader18
force-pushed
the
noa/module-log-ts
branch
from
400ad83 to
b3d92d7
Compare
|
oh lol those weren't even flukes, I just missed them when going through & updating the tests |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of Changes
Unsure about the TODO comment; walltime now breaks the illusion that all of a reducer happens at the same time, albeit not in a way observable to wasm. However, if we go with logical now it's more likely that the order of timestamps in the log would be non-monotonic, and we lose the information of how long between 2 logs internal to the same reducer.
Expected complexity level and risk
1