Quantise report cache in query side of aws-collector #3671
Conversation
…allel Previously we would merge all reports in a 15-second window. Now we use a 'quantum' of 3 seconds, similar to the single-user app. E.g. a 30-node cluster will have 150 individual reports over 15 seconds, but the new code will merge 5 pre-merged reports plus 20-ish very recent individual ones. This limits the max heap size used for deserialising, since we only do 3 seconds at once per instance. Individual reports are still put into the cache, but should get displaced by the pre-merged ones under LRU.
bboreham
changed the title
app/multitenant/aws_collector.go
Outdated
| reportQuantisationInterval = 3000000000 // 3 seconds in nanoseconds | ||
| // Grace period allows for some gap between the timestamp on reports | ||
| // (assigned when they arrive at collector) and them appearing in DynamoDB query | ||
| gracePeriod = 500000000 // 1/2 second in nanoseconds |
There was a problem hiding this comment.
Why are these not 3 * time.Second and 500 * time.Millisecond, respectively?
There was a problem hiding this comment.
Because timestamps are stored in the database as a number of nanoseconds since the epoch, manipulated in the program as int64 type.
I could easily make these constants time.Duration and call .Nanoseconds() on every use, somewhat harder to convert all the timestamps into time.Time and get the modulus arithmetic correct.
There was a problem hiding this comment.
I did "make these constants time.Duration and call .Nanoseconds() on every use"
app/multitenant/aws_collector.go
Outdated
| var reports []report.Report | ||
| // Fetch a merged report for each time quantum covering the window | ||
| startTS, endTS := start.UnixNano(), end.UnixNano() | ||
| ts := startTS - (startTS % reportQuantisationInterval) |
There was a problem hiding this comment.
This will include reports up to reportQuantisationInterval earlier than startTS. It seems odd to 'run over' like that at the start of the interval but not the end (where we stop short and then fetch individual reports).
There was a problem hiding this comment.
Yes, it extends the window up to 3 seconds into the past. This is assumed to be not noticeable.
We stop short at the near end of the window on the assumption that reports in that range could still arrive in the database.
| return report.MakeReport(), err | ||
| } | ||
| reports = append(reports, quantumReport) | ||
| } |
There was a problem hiding this comment.
I was wondering whether that entire loop could operate on time.Time, converting to unix nanos only in the call to (or even inside) reportForQuantum.
There was a problem hiding this comment.
It’s the modulus that scares me.
I’ve made one bug that way already this year.
Cache merged groups of reports, to reduce the number we handle in parallel.
Previously we would merge all reports in a 15-second window. Now we use a 'quantum' of 3 seconds, similar to the single-user app.
E.g. a 30-node cluster will have 150 individual reports over 15 seconds, but the new code will merge 5 pre-merged reports plus 20-ish very recent individual ones.
This reduces the max heap size used for deserialising, since we only do 3 seconds at once per instance. This improves #3256, but I'm not marking it as a fix since the number done in parallel is still unlimited.
Individual reports are still put into the cache, but should get displaced by the pre-merged ones under LRU.