[query] Graphite fetches truncated to resolution #1997
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1997 +/- ##
=======================================
Coverage 63.6% 63.6%
=======================================
Files 1122 1122
Lines 106581 106581
=======================================
Hits 67830 67830
Misses 34421 34421
Partials 4330 4330
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #1997 +/- ##
=======================================
Coverage 63.6% 63.6%
=======================================
Files 1122 1122
Lines 106581 106581
=======================================
Hits 67830 67830
Misses 34421 34421
Partials 4330 4330
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1997 +/- ##
=======================================
Coverage 63.6% 63.6%
=======================================
Files 1122 1122
Lines 106581 106581
=======================================
Hits 67830 67830
Misses 34421 34421
Partials 4330 4330
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #1997 +/- ##
=======================================
Coverage 63.6% 63.6%
=======================================
Files 1122 1122
Lines 106581 106581
=======================================
Hits 67830 67830
Misses 34421 34421
Partials 4330 4330
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1997 +/- ##
=======================================
Coverage 63.6% 63.6%
=======================================
Files 1122 1122
Lines 106581 106581
=======================================
Hits 67830 67830
Misses 34421 34421
Partials 4330 4330
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1997 +/- ##
=======================================
Coverage 63.6% 63.6%
=======================================
Files 1122 1122
Lines 106581 106581
=======================================
Hits 67830 67830
Misses 34421 34421
Partials 4330 4330
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #1997 +/- ##
=======================================
Coverage 63.6% 63.6%
=======================================
Files 1122 1122
Lines 106581 106581
=======================================
Hits 67830 67830
Misses 34421 34421
Partials 4330 4330
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1997 +/- ##
=======================================
Coverage 63.6% 63.6%
=======================================
Files 1122 1122
Lines 106581 106581
=======================================
Hits 67830 67830
Misses 34421 34421
Partials 4330 4330
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #1997 +/- ##
=======================================
Coverage 63.6% 63.6%
=======================================
Files 1122 1122
Lines 106581 106581
=======================================
Hits 67830 67830
Misses 34421 34421
Partials 4330 4330
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1997 +/- ##
=========================================
- Coverage 63.6% 63.6% -0.1%
=========================================
Files 1122 1122
Lines 106581 106544 -37
=========================================
- Hits 67830 67789 -41
Misses 34421 34421
- Partials 4330 4334 +4
Continue to review full report at Codecov.
|
| @@ -128,6 +147,7 @@ func translateTimeseries( | |||
| m3list := result.SeriesList | |||
| series := make([]*ts.Series, len(m3list)) | |||
| resolutions := result.Metadata.Resolutions | |||
| fmt.Println("Resolutions", resolutions) | |||
There was a problem hiding this comment.
Probably can remove this yeah?
There was a problem hiding this comment.
Oops thought I got them all
| continue | ||
| } | ||
|
|
||
| if ts.After(end) { |
There was a problem hiding this comment.
I think each step is inclusive at the start and exclusive at the end yeah? i.e. [start, end)
Otherwise a single datapoint would fall into two steps if it aligned with the resolution exactly no?
As such, it sounds like this should be if !ts.Before(end) { break }?
There was a problem hiding this comment.
Also another question... are we worried the values will ever be returned out of order? Heh. Cause if so it'd be better to continue here rather than break. It's possible that would be overly defensive programming, but if the underlying storage ever was something else that didn't use our series iterators I suppose that would be possible... (at graphite would be fine since it always uses consolidation and values.SetValueAt(idx, value).
There was a problem hiding this comment.
Yeah, good point re: !Before; was thinking that I may have dropped some points due to truncation so wanted to keep as much as possible, but the dual-read is a good point.
About the break v continue, I think that if we have non-ordered series coming in, there's a lot of places that would break worse than this haha. Also given it's m3_wrapper.go, I think it's fine to assume it's ordered :P
What this PR does / why we need it:
Graphite fetches would always be truncated to start point, which can cause flapping when refreshing a series.
Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: