fix(prometheus_remote_write sink): Default timestamp to now #6823
Conversation
When encoding a metric to the remote write protocol, a timestamp field must be present. When presented with a metric without a timestamp, this sink encoded a timestamp of `0`, which is almost certainly not the correct value. This changes that default to the current time. Signed-off-by: Bruce Guenter <bruce.guenter@datadoghq.com>
Signed-off-by: Bruce Guenter <bruce.guenter@datadoghq.com>
bruceg
added
type: bug
|
Note, a second PR will be coming shortly to handle the other part of #6814. |
| fn now(&mut self) -> i64 { | ||
| *self | ||
| .now | ||
| .get_or_insert_with(|| Utc::now().timestamp_millis()) |
There was a problem hiding this comment.
The naming of this function wouldn't indicate to me that it caches the value of now. I'm wondering if we should just remove that caching.
Alternatively, I could see making this a top-level function and using cfg(test) to define a mock implementation with a known value that the tests could depend on here.
There was a problem hiding this comment.
I figured it being both a simple optimization and a way of providing that all metrics in a batch used the same default timestamp. It has to be per-batch, as otherwise all metrics without a timestamp would share the same time again.
There was a problem hiding this comment.
That said, I am definitely open to renaming it to indicate the purpose better. I'll figure a more descriptive name.
There was a problem hiding this comment.
I can see why it might be desirable for all metrics in the same batch to have the same default timestamp value, but is that an actual requirement? I was thinking it could just always use whatever Utc::now() is, without the caching.
There was a problem hiding this comment.
👍 the naming is definitely better now though.
There was a problem hiding this comment.
No, it's not a requirement, just an optimization. On my desktop, Utc::now is 4-6x as expensive as cloning a string, though amusingly it's faster than cloning on my MBP (which hints that MacOS has an aweful malloc implementation).
There was a problem hiding this comment.
👍 I'm cool with it with the current naming. Thanks for making that change. Just to make sure I understand it, this is actually only applying to one timeseries though, isn't it?
Signed-off-by: Bruce Guenter <bruce.guenter@datadoghq.com>
* fix(prometheus_remote_write sink): Default timestamp to now When encoding a metric to the remote write protocol, a timestamp field must be present. When presented with a metric without a timestamp, this sink encoded a timestamp of `0`, which is almost certainly not the correct value. This changes that default to the current time. * Optimize calculating now to only once per request Signed-off-by: Bruce Guenter <bruce.guenter@datadoghq.com>
When encoding a metric to the remote write protocol, a timestamp field
must be present. When presented with a metric without a timestamp, this
sink encoded a timestamp of
0, which is almost certainly not thecorrect value. This changes that default to the current time.
Closes #6814
Signed-off-by: Bruce Guenter bruce.guenter@datadoghq.com