feat(time)!: epoch timestamps as standard #4252
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
geyslan
reviewed
Aug 23, 2024
|
@geyslan Is the PR ok besides the change you've commented on? I will drop that commit since it's irrelevant. |
NDStrahilevitz
force-pushed
the
time_cleanups
branch
2 times, most recently
from
69318a0 to
37db421
Compare
NDStrahilevitz
force-pushed
the
time_cleanups
branch
from
37db421 to
282dc1d
Compare
NDStrahilevitz
force-pushed
the
time_cleanups
branch
2 times, most recently
from
8b545a9 to
c1ea067
Compare
NDStrahilevitz
changed the title
NDStrahilevitz
force-pushed
the
time_cleanups
branch
3 times, most recently
from
426adc8 to
3423902
Compare
geyslan
changed the title
|
When rebasing, please put ! in the breaking changes commits, like I did in the PR header. Tks. |
NDStrahilevitz
force-pushed
the
time_cleanups
branch
3 times, most recently
from
9437b4b to
1486115
Compare
|
@rscampos Could you review this? |
rscampos
approved these changes
Sep 16, 2024
| func Init(clockID int32) error { | ||
| var err error | ||
| initTimeOnce.Do(func() { | ||
| startTimeMonotonic, errIn := getClockTimeNS(clockID) |
There was a problem hiding this comment.
Nit: The variable startTimeMonotonic could represent either the result from a Monotonic or Boottime clock, depending on the value of clockID, right?. Therefore, it may be a good idea to consider renaming the variable (also in the rest of the function) to better reflect this.
| @@ -208,7 +214,7 @@ func (ctrl *Controller) procTreeExitProcessor(args []trace.Argument) error { | |||
|
|
|||
| return ctrl.processTree.FeedFromExit( | |||
| proctree.ExitFeed{ | |||
| TimeStamp: uint64(ctrl.timeNormalizer.NormalizeTime(int(timestamp))), // time of exit is already a timestamp | |||
| TimeStamp: time.BootToEpochNS(timestamp), // time of exit is already a times)p | |||
Instead of using a helper, and then using it in a specialized processor function - use function currying to enable creating generic time argument processors.
Remove all timeNormalizer object logic. Instead use simple normalization functions and normalize all timestamps in events asap: 1. Context timestamp normalization moved to decode stage 2. Relevant timestamp arguments normalized in processing stage, registered to run first.
NDStrahilevitz
force-pushed
the
time_cleanups
branch
from
1486115 to
8860f6e
Compare
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.
1. Explain what the PR does
1486115 feat(time): normalize all time to epoch
fbbe9f9 feat(output)!: remove relative-time
1a55bdb chore(pipeline): streamline time args normalization
18d3abd chore: refactor time pkg
1486115 feat(time): normalize all time to epoch
1a55bdb chore(pipeline): streamline time args normalization
2. Explain how to test it
-o option:relative-timeshould failparent_start_time,start_time,leader_start_time,parent_process_start_time)3. Other comments
Resolve #4287
Resolve #3820