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

maxStep in requestContext for Finder #2724

Closed deniszh closed 2 years ago

deniszh commented 2 years ago

This PR adding URL parameter named maxStep which will be added to requestContext (if defined in URL parameters) and then finder plugin can use it.

Some context and explanation.

We have requestContext parameter, which used in Graphite-web code. It can be used for transferring some data between functions (i.e. providing context). This context parameter also accepting by Finder plugins, but by default Finder ignores it. Both changes trying to help fix long standing issue in Graphite, when datapoint period becoming less then windowSize parameter in some functions these functions return empty or wrong result. That can happen with hitcount(), summarize(), smartSummarize(), movingAverage/Min/Max/Median/Sum/Window() and exponentialMovingAverage(). Unfortunately, that issue is not possible to fix in case of Whisper - if we have only aggregated data in low resolution archive we just can't get high resolution data from it. But issue can be solvable with storages that (at least potentially) can do live aggregation - then we can or transfer desired step directly (using maxStep parameter described above) - or Finder plugin can calculate desired step using target list which is already provided in context starting from v1.1.8.

deniszh commented 2 years ago

Rebased, looks better now.

codecov-commenter commented 2 years ago

Codecov Report

Merging #2724 (df473d0) into master (2e58d21) will decrease coverage by 0.01%. The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2724      +/-   ##
==========================================
- Coverage   77.10%   77.09%   -0.02%     
==========================================
  Files         176      176              
  Lines       18968    18972       +4     
  Branches     4032     4034       +2     
==========================================
  Hits        14626    14626              
- Misses       3812     3814       +2     
- Partials      530      532       +2     
Impacted Files Coverage Δ
webapp/graphite/render/views.py 69.66% <0.00%> (-0.34%) :arrow_down:
graphite/render/views.py 69.66% <0.00%> (-0.34%) :arrow_down:
graphite/tags/redis.py 87.03% <0.00%> (ø)
webapp/graphite/tags/redis.py 87.03% <0.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 2e58d21...df473d0. Read the comment docs.

DanCech commented 2 years ago

We can't use normal target parameter for it, so, I creating second one, named targets_serialized.

I'm confused by what we gain by having a pickled version of the same data that's already in targets, can you explain what you mean here?

deniszh commented 2 years ago

Easy - target is not in context. I couldn’t trace what removes it and why but target just not present in requestContext in Finder. :(

On Wed, 3 Nov 2021 at 21:16, Dan Cech @.***> wrote:

We can't use normal target parameter for it, so, I creating second one, named targets_serialized.

I'm confused by what we gain by having a pickled version of the same data that's already in targets, can you explain what you mean here?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/graphite-project/graphite-web/pull/2724#issuecomment-959948556, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJLTVQWVYYCKIRVHO3XVSLUKGRBHANCNFSM5HJYYV6A .

deniszh commented 2 years ago

But you probably right, it's worth to at least find why current setup is not working.

deniszh commented 2 years ago

@DanCech : yes, looks like I started from different version and just missed @replay 's commit which bringing targets into context. 😵‍💫 Removed it and amended description.

deniszh commented 2 years ago

💚 All backports created successfully

Status Branch Result
1.1.x