feat: migrate proto timestamp to uint64 #134
Conversation
| Val time.Time | ||
| isSet bool | ||
| } | ||
| type TimeMicrosUnix Int64 |
| event.Timestamp = timestamppb.New(event.Timestamp.AsTime().Add( | ||
| time.Duration(float64(time.Millisecond) * from.Start.Val), | ||
| )) | ||
| event.Timestamp = event.Timestamp + uint64( |
There was a problem hiding this comment.
| event.Timestamp = event.Timestamp + uint64( | |
| event.Timestamp += uint64( |
| var base uint64 = 0 | ||
| if event.Timestamp != 0 { | ||
| base = event.Timestamp |
There was a problem hiding this comment.
Can we remove this base now, and just use event.Timestamp += below?
| @@ -38,5 +37,5 @@ message Event { | |||
| google.protobuf.Duration duration = 8; | |||
| uint64 severity = 9; | |||
|
|
|||
| google.protobuf.Timestamp received = 10; | |||
| uint64 received = 10; | |||
There was a problem hiding this comment.
Could you please add a comment to this noting that it's nanoseconds since epoch
| @@ -52,7 +51,7 @@ import "useragent.proto"; | |||
| option go_package = "github.com/elastic/apm-data/model/modelpb"; | |||
|
|
|||
| message APMEvent { | |||
| google.protobuf.Timestamp timestamp = 1; | |||
| uint64 timestamp = 1; | |||
There was a problem hiding this comment.
Could you please add a comment to this noting that it's nanoseconds since epoch
| // TimeToPBTimestamp encodes a time.Time to Unix epoch nanos in uint64 for protobuf. | ||
| func TimeToPBTimestamp(t time.Time) uint64 { | ||
| return uint64(t.UnixNano()) | ||
| } | ||
|
|
||
| // PBTimestampToTime decodes a uint64 of Unix epoch nanos to a time.Time for protobuf. | ||
| func PBTimestampToTime(timestamp uint64) time.Time { | ||
| return time.Unix(0, int64(timestamp)).UTC() | ||
| } | ||
|
|
||
| func PBTimestampNow() uint64 { | ||
| return TimeToPBTimestamp(time.Now()) | ||
| } | ||
|
|
||
| func PBTimestampTruncate(t uint64, d time.Duration) uint64 { | ||
| ns := uint64(d.Nanoseconds()) | ||
| return t - t%ns | ||
| } | ||
|
|
||
| func PBTimestampAdd(t uint64, d time.Duration) uint64 { | ||
| ns := uint64(d.Nanoseconds()) | ||
| return t + ns | ||
| } | ||
|
|
||
| func PBTimestampSub(t uint64, tt time.Time) time.Duration { | ||
| b := TimeToPBTimestamp(tt) | ||
| return time.Duration(t - b) | ||
| } |
There was a problem hiding this comment.
I don't love rebuilding time.Time functions, unless there's a significant performance benefit. Would it be reasonable to just have two functions, to convert to/from time.Time, and perform all time math using the usual time.Time methods? e.g.
// FromTime converts a time.Time to uint64 nanoseconds since Unix epoch.
func FromTime(t time.Time) uint64
// ToTime converts uint64 nanoseconds since Unix epoch to a time.Time.
func ToTime(v uint64) time.Time|
|
||
| // PBTimestampToTime decodes a uint64 of Unix epoch nanos to a time.Time for protobuf. | ||
| func PBTimestampToTime(timestamp uint64) time.Time { | ||
| return time.Unix(0, int64(timestamp)).UTC() |
There was a problem hiding this comment.
Should we be passing in seconds & nanoseconds, so we don't lose so much precision?
|
|
||
| func (t Time) MarshalFastJSON(w *fastjson.Writer) error { | ||
| w.RawByte('"') | ||
| w.Time(time.Time(t), timestampFormat) | ||
| w.Time(time.Unix(0, int64(t)).UTC(), timestampFormat) |
There was a problem hiding this comment.
Should we use PBTimestampToTime here?
Alternatively I think we could leave modeljson alone, and convert to time.Time in the caller.
| signature string // combination of all attributes | ||
| timestamp uint64 |
|
Is this still relevant? |
Replace
timestamppbwithuint64in protobuf modelsUpdate modeljson to use
uint64instead oftime.Timeunder the hood.Update input models to store
int64to avoid int->time->int conversion.Add utility methods for usability.
Caveat: we lose the ability to ingest timestamps before the unix epoch or after the year 2262 (sorry, future apm developers).
Closes #129