-
Notifications
You must be signed in to change notification settings - Fork 4
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
improvement: Adds timestamp to messages #13
Conversation
8e67182
to
5f081f1
Compare
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.
we'll need tests around the value that gets sent in the payload, rather than from the Options
as it is currently
2b7ff22
to
8ef23dc
Compare
also note that code coverage is being published, you can check it in CircleCI artifacts: https://98-268886384-gh.circle-artifacts.com/0/tmp/artifacts/coverage.html#file2 (you will see there is a new uncovered line in this PR) |
4d3c07c
to
680a50f
Compare
680a50f
to
2222ecd
Compare
2222ecd
to
2b32a47
Compare
2b32a47
to
b1f926d
Compare
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.
few small comments
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.
latest lgtm, one nit
Includes a timestamp for each message/line that flows through the client. Allows the user to provide their own timestamp through Options- and if the timestamp is not provided- the value is defaulted. Semver: Patch Ref: LOG-7156
b1f926d
to
7f8ffc4
Compare
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!
if timestamp.IsZero() { | ||
timestamp = time.Now() | ||
} | ||
line.Timestamp = timestamp.UnixNano() / int64(time.Millisecond) |
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 was also going to originally mention that this logic is a bit confusing to read at first. It's mostly an issue with the variable names not providing enough context. My understanding is that the constant time.Millisecond
is the Duration
of 1ms. Duration
is measured in nanoseconds so time.Millisecond
is the number of nanoseconds in 1 millisecond. That's why the math above works.
Might be worthwhile to drop a comment about it. Not necessary, gonna approve regardless.
improvement: Adds timestamp to messages
Includes a timestamp for each message/line that flows through the client.
Allows the user to provide their own timestamp through Options- and if the timestamp is not provided- a default value is used.
Semver: Patch
Ref: LOG-7156