graphite-project / graphite-web

A highly scalable real-time graphing system
http://graphite.readthedocs.org/
Apache License 2.0
5.89k stars 1.26k forks source link

fix shifting of moving* datapoints by one step into the future #2682

Closed zivillian closed 3 years ago

zivillian commented 3 years ago

This is the quick fix from the discussion in #2632

fixes #2632

deniszh commented 3 years ago

Hi @zivillian ,

I'm afraid that's quick and incorrect fix, though... As you can see many function tests are broken now, because of extra 'None' inserted.

zivillian commented 3 years ago

If I'm correct, there is no extra None introduced, but it's missing and still present in the expectedData lists. Is my assumption correct, that the original and moved series should contain the same number of datapoints? If so, I would update the test data to reflect this.

deniszh commented 3 years ago

Ah, mea culpa, you're rightm @zivillian E.g. https://github.com/graphite-project/graphite-web/blob/97f4d85750b3bff2c478c2d3b98cf4368ca84756/webapp/tests/test_functions.py#L4687 Then tests need to be correcred, indeed

zivillian commented 3 years ago

I'll take a look into this, and also add an assertion that the number of datapoints in both series is equal

codecov-io commented 3 years ago

Codecov Report

Merging #2682 (1805baa) into master (528d197) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2682   +/-   ##
=======================================
  Coverage   80.11%   80.11%           
=======================================
  Files          88       88           
  Lines        9403     9403           
  Branches     2002     2002           
=======================================
  Hits         7533     7533           
  Misses       1582     1582           
  Partials      288      288           
Impacted Files Coverage Ξ”
webapp/graphite/render/functions.py 95.37% <100.00%> (ΓΈ)

Continue to review full report at Codecov.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update 528d197...1805baa. Read the comment docs.

deniszh commented 3 years ago

I'm still bit hesitating to merge this TBH. Opinions? /cc @ploxiln

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

zivillian commented 3 years ago

@ploxiln @deniszh any thoughts?

deniszh commented 3 years ago

Thanks, @ploxiln for reviewing this! That's indeed quite tricky. Merging. And thanks to @zivillian for issue and PR too, of course!

deniszh commented 2 years ago

πŸ’” Some backports could not be created

Status Branch Result
❌ 1.1.x Cherrypick failed because the selected commit was a merge commit. Please try again by specifying the parent with the mainline argument:

backport --mainline

or:

backport --mainline

Or refer to the git documentation for more information: https://git-scm.com/docs/git-cherry-pick#Documentation/git-cherry-pick.txt---mainlineparent-number|

To backport manually run: node scripts/backport --pr 2682. For more info read the Backport documentation

deniszh commented 2 years ago

πŸ’” Some backports could not be created

Status Branch Result
❌ 1.1.x Cherrypick failed because the selected commit was a merge commit. Please try again by specifying the parent with the mainline argument:

backport --mainline

or:

backport --mainline

Or refer to the git documentation for more information: https://git-scm.com/docs/git-cherry-pick#Documentation/git-cherry-pick.txt---mainlineparent-number|

To backport manually run: node scripts/backport --pr 2682. For more info read the Backport documentation

deniszh commented 2 years ago

πŸ’” Some backports could not be created

Status Branch Result
❌ 1.1.x Cherrypick failed because the selected commit was a merge commit. Please try again by specifying the parent with the mainline argument:

backport --mainline

or:

backport --mainline

Or refer to the git documentation for more information: https://git-scm.com/docs/git-cherry-pick#Documentation/git-cherry-pick.txt---mainlineparent-number|

To backport manually run: node scripts/backport --pr 2682. For more info read the Backport documentation