Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent behaviour with over() compared to bandTSDB() and window() #2297

Closed
DCBoland opened this Issue Aug 29, 2018 · 4 comments

Comments

Projects
None yet
3 participants
@DCBoland
Copy link
Contributor

DCBoland commented Aug 29, 2018

Using over() results in a seriesSet containing a series starting now, i.e. {shift=0s}. This is unexpected and inconsistent with the behaviour of bandTSDB() and window(), which start period ago. This is an issue when trying to calculate a historical distribution, median etc. using the new aggr() function, as the current value will be included.

The above inconsistency appears to be because over() subtracts period from now at the end of its loop, whereas the other functions do the subtraction at the start of their loops:

now = now.Add(time.Duration(-p))

Potential solutions:

  • Change over()'s behaviour to be in line with the other functions
  • Add parameters to set a start time or offset.

I'm assuming the first option would cause too many problems to be acceptable?

Context:
aggr() function PR

@kylebrandt

This comment has been minimized.

Copy link
Member

kylebrandt commented Aug 29, 2018

over() was created for making time over time graphs (e.g. week over week), for which you want the current week. So I don't think we should change the behavior (but should update the docs to be more clear).

So instead I think we should just add another function that behaves like band and window, I'm not sure what to call it.

@hermanschaaf

This comment has been minimized.

Copy link
Contributor

hermanschaaf commented Aug 30, 2018

If we do create another function, can we add another parameter for explicitly specifying the end time, rather than using period for this? This would allow us to more easily account for minor metric delays, and also achieve what @DCBoland described. Though we may need one like this for band and another for over (with time shifting included)

DCBoland added a commit to DCBoland/bosun that referenced this issue Sep 13, 2018

cmd/bosun: Add OverQuery(), BandQuery() extending Over(), Band() to s…
…upport end time

Adds more general versions of Over() and Band() that support an eduration
parameter to set the end time of the query, like with q() (bosun-monitor#2297)
@DCBoland

This comment has been minimized.

Copy link
Contributor Author

DCBoland commented Sep 13, 2018

While looking at the above, I spotted the undocumented shiftBand function, which appears to return an over-like seriesSet, with the period shift behaviour from band – exactly what I needed!

The above commit is along the lines of @hermanschaaf's suggestion. Adds an eduration parameter to generic versions of band and over, named bandQuery and overQuery, to set the end time as is done with Query. The original functions just wrap these new ones, with period given as the eduration to match the existing behaviour.

@kylebrandt

This comment has been minimized.

Copy link
Member

kylebrandt commented Sep 13, 2018

@DCBoland Nice I like it, adds what people want without messing with current behavior (or at least it shouldn't).

Needs expr documentation in docs.md (can you add for the shiftBand ... easter egg :p ... as well?)

Then if you add some tests it will be easier for me to look over the code and merge it since I don't have test things locally as much.

kylebrandt added a commit that referenced this issue Sep 24, 2018

cmd/bosun: add opentsdb query functions with end duration parameter. (#…
…2310)

Add OverQuery(), BandQuery() extending Over(), Band() to support end time

Adds more general versions of Over() and Band() that support an eduration
parameter to set the end time of the query, like with q() (#2297)

@DCBoland DCBoland closed this Sep 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.