Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

Fix uneven length panics #1452

Merged
merged 12 commits into from
Sep 11, 2019
Merged

Fix uneven length panics #1452

merged 12 commits into from
Sep 11, 2019

Conversation

Dieterbe
Copy link
Contributor

@Dieterbe Dieterbe commented Sep 7, 2019

Document concepts better and in particular introduce new concept of a "canonical series".
Now we massage the fetch and fix parameters such that a high resolution series will always look like a native lower resolution serie (one that has the same interval as what we're normalizing to).
In particular, they will now have corresponding timestamps, and the same amount of points.

fix #913 (panic in mergeSeries) - tried the repro case. can't repro anymore.
fix #1095 (mergeSeries and sumSeries) - can not repo either one with this code.
fix #874 (dupe)

@Dieterbe
Copy link
Contributor Author

Dieterbe commented Sep 7, 2019

note to self, this patch was helpful to see what's going on

diff --git a/api/dataprocessor.go b/api/dataprocessor.go
index ff460aea..0ca7042a 100644
--- a/api/dataprocessor.go
+++ b/api/dataprocessor.go
@@ -9,6 +9,7 @@ import (
        "sync"
        "time"
 
+       "github.com/davecgh/go-spew/spew"
        "github.com/grafana/metrictank/api/models"
        "github.com/grafana/metrictank/consolidation"
        "github.com/grafana/metrictank/mdata"
@@ -647,6 +648,19 @@ func mergeSeries(in []models.Series) []models.Series {
        }
        merged := make([]models.Series, len(seriesByTarget))
        i := 0
+       for seg, series := range seriesByTarget {
+               fmt.Printf("mergeSeries  %v - len %d\n", seg, len(series))
+               lens := make(map[int]int)
+               for _, serie := range series {
+                       l := len(serie.Datapoints)
+                       fmt.Println("len points", l)
+                       lens[l] = l
+               }
+               if len(lens) > 1 {
+                       log.Error("mergeSeries detected merge of series of unequal length")
+                       spew.Dump(series)
+               }
+       }

util/overflow.go Outdated Show resolved Hide resolved
Dieterbe and others added 12 commits September 11, 2019 23:08
this allows us to do less work if the context is canceled
checking req.Archive is more robust.
checking consolidator worked, because newRequestContext is
only being called by
getSeriesFixed, which would only specify consolidator.None in case
req.Archive == 0 (noting that req.Consolidator must be > 0, as set
by executePlan)
but relying on that is convoluted. just get the signal from the source.

(and newRequestContext is also called by two tests which leave
req.Consolidator 0 where they would pass consolidator.None)
because it is not always stable
...with the concept of canonical series.

Note that we started using rctx.From and rctx.To as parameters to Fix(),
not req.From and req.To
Aside from the new pre-canonicalization tweak in newRequestContext(),
this is equivalent.
Copy link
Contributor

@fkaleo fkaleo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Dieterbe Dieterbe merged commit e05e1e8 into master Sep 11, 2019
@Dieterbe Dieterbe deleted the fix-uneven-length-panics branch September 11, 2019 21:32
@Dieterbe
Copy link
Contributor Author

should also fix #761

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants