Fix download serialization to JSON to be a float in bps instead of a string with unit #216
Conversation
nchaulet
added
the
Team:Elastic-Agent-Control-Plane
💚 Build Succeeded
History
cc @nchaulet |
pierrehilbert
added
the
Team:Elastic-Agent-Data-Plane
ycombinator
removed
the
Team:Elastic-Agent-Data-Plane
ycombinator
requested review from
michel-laterman and
blakerouse
and removed request for
belimawr and
rdner
| @@ -33,32 +29,10 @@ type DownloadRate float64 | |||
| func (dr *DownloadRate) MarshalJSON() ([]byte, error) { | |||
| downloadRateBytesPerSecond := float64(*dr) | |||
| if math.IsInf(downloadRateBytesPerSecond, 0) { | |||
| return json.Marshal("+Inf bps") | |||
| return json.Marshal(math.MaxFloat64) | |||
There was a problem hiding this comment.
I fear that this could be misleading, especially if we use this value to get metrics or averages in ES/Kibana...
I get that having the unit is causing problems, but how often is this value actually Inf? Is it because the download rate is too slow or too fast?
I believe a custom marshal/unmarshal here and on Fleet-Server could more accurately handle those situations.
There was a problem hiding this comment.
@belimawr actually I am not sure we ever encounter the Inf issue ever, as with the max float 64 value we could support 1.798e+284YBps,
There was a problem hiding this comment.
Then I believe the best is to encode as +Inf/-Inf and deal with that on the Fleet side as well. Or even not send this value/piece of data as we know it is due to some issue.
The key thing on my mind is that if we show this to our users, it needs to be accurate, or clearly state we can't get this metric for some reason.
There was a problem hiding this comment.
@ycombinator I am curious to have your thoughts on that one as you did the original implementation, does the Inf scenario something that already happens/could happens when computing the download rate in the agent? otherwise it seems we may be able to use the default serialization and do not really care about Inf value no?
There was a problem hiding this comment.
Yes, IIRC, we have occasionally seen +Inf when the download happens really fast, almost instantaneously. But, stepping back and taking a look at the end-goal here, it is to let the user know how fast a download is proceeding. For really fast downloads, a user is not going to care if we report that as +Inf bps (which, of course, has proven problematic being a string) or some really large number like math.MaxFloat64. In some ways, the latter is a bit more realistic even. So I'm +1 for going with math.MaxFloat64 here as the fix.
There was a problem hiding this comment.
If you two agree with math.MaxFloat64 I'm not opposed to it. I'd just make sure we get the correct signal to differentiate between +Inf and -Inf.
There was a problem hiding this comment.
Ah, thanks @belimawr. I should've clarified that -Inf should never happen with the download rate so it's really a question of how we want to represent the +Inf situation as a numeric value.
Description
Related to https://github.com/elastic/ingest-dev/issues/3446
Fix DownloadRate serialization to JSON to be a float in bps instead of a string with unit.
as serializing
Infto json is not possible, I fallback to the max float value, I think it's probably better than crashing for that.