fix: skip points outside of archive retention on first pass #82
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,53 +54,72 @@ func ReadPoints(w Archive, name string) ([]whisper.Point, error) { | |
| return nil, fmt.Errorf("whisper file contains no archives for metric: %q", name) | ||
| } | ||
|
|
||
| // Dump one precision level at a time and write into the output slice. | ||
| // Its important to remember that the archive with index 0 (first archive) | ||
| // has the raw data and the highest precision https://graphite.readthedocs.io/en/latest/whisper.html#archives-retention-and-precision | ||
| seenTs := map[uint32]struct{}{} | ||
| lastMinTs := uint32(0) | ||
| var keptPoints []whisper.Point | ||
| for i, a := range w.GetArchives() { | ||
| archivePoints, err := w.DumpArchive(i) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to dump archive %d from whisper metric %s", i, name) | ||
| } | ||
|
|
||
| var minArchiveTs, maxArchiveTs uint32 | ||
| // calculate maxArchiveTs | ||
| for _, p := range archivePoints { | ||
| // We want to track the max timestamp of the archive because we know | ||
| // it virtually represents now() and we wont have newer points. | ||
| // Then the min timestamp of the archive would be maxTs - the archive | ||
| // retention. | ||
| if p.Timestamp > maxArchiveTs { | ||
| maxArchiveTs = p.Timestamp | ||
| } | ||
| } | ||
|
|
||
| // calculate minArchiveTs | ||
| if maxArchiveTs-a.Retention() > maxArchiveTs { // overflow, very big retention | ||
ywwg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| minArchiveTs = 0 | ||
| } else { | ||
| minArchiveTs = maxArchiveTs - a.Retention() | ||
| } | ||
|
|
||
| // For subsequent archives | ||
| if lastMinTs > 0 { | ||
| // if we are already in the second or subsequent archive and we had | ||
| // some points in the prior archives, we want to skip | ||
| // samples in the previous archives | ||
| maxArchiveTs = lastMinTs | ||
|
There was a problem hiding this comment. We update |
||
| } | ||
| lastMinTs = minArchiveTs | ||
|
|
||
| for _, p := range archivePoints { | ||
| if p.Timestamp < minArchiveTs { | ||
|
There was a problem hiding this comment. Shall we check if If we do that, we don't need to keep There was a problem hiding this comment. Actually check for There was a problem hiding this comment. Because points are written to all archives at once, all archives will contain points from timestamps that are within the retentions. So we do need a check like this. I think we can do it more efficiently if we pre-sort the points and then do a double-index comparison
ywwg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| continue | ||
|
There was a problem hiding this comment. @pstibrany and @ywwg This solves problem 1 where we just drop all points that are beyond the retention of the archive but still present in the ring buffer. |
||
| } | ||
| // If we have already seen a point with the same timestamp it means | ||
| // we already have a point from an archive with higher precision that | ||
| // we want to keep. So we skip this point. | ||
| if _, ok := seenTs[p.Timestamp]; ok { | ||
|
There was a problem hiding this comment. @pstibrany and @ywwg This solves problem 2 that we discussed yesterday where we want to keep points from prior archives because they have higher resolutions. |
||
| continue | ||
| } | ||
| // Skip points with time = 0 | ||
| if p.Timestamp == 0 { | ||
| continue | ||
| } | ||
ywwg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| keptPoints = append(keptPoints, whisper.Point{Timestamp: p.Timestamp, Value: p.Value}) | ||
| seenTs[p.Timestamp] = struct{}{} | ||
| } | ||
| } | ||
|
|
||
| // We need to finally sort the kept points again because different archives | ||
| // may overlap and have older points | ||
| sort.Slice(keptPoints, func(i, j int) bool { | ||
| return keptPoints[i].Timestamp < keptPoints[j].Timestamp | ||
| }) | ||
|
There was a problem hiding this comment. With the two additions above, we now only need to sort once. |
||
|
|
||
| return keptPoints, nil | ||
| } | ||
|
|
||
| // ToMimirSamples converts a Whisper metric with the given name to a slice of | ||
|
|
||
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.
Why do we do this for each archive again and again?
Don't all archives have the same "now" and they only differ in their retention period?
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.
Ah, you're right. We had seen the "offset" value in the database dump but that is a byte offset, not a timestamp offset.