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

DataLinks: Enable value time windowing in TimeSrv #18636

Merged
merged 5 commits into from
Aug 22, 2019

Conversation

dprokop
Copy link
Member

@dprokop dprokop commented Aug 20, 2019

This PR introduces changes to TimeSrv that enables value time windowing.

Given a value time and custom time window size TimeSrv will set time range to be narrowed down to a specified window. I.e given a 5s window and value timestamp, dashboard's time range will be set to (valueTs - 2.5s) - (valueTs + 2.5s).

The change introduced to TimeSrv is about enabling support for valueTime & valueTimeWindow query params that's translated to a custom time range.

@dprokop dprokop requested a review from torkelo August 20, 2019 07:57
@dprokop dprokop changed the title DataLinks: Enable value time windowing in time service DataLinks: Enable value time windowing in TimeSrv Aug 20, 2019
@dprokop
Copy link
Member Author

dprokop commented Aug 20, 2019

@torkelo This supports only symmetrical windows around value time. WDYT about introducing syntax support for value time windowing when the valueTime is a limit value for the time range? I mean:

?valueTime=TS&valueTimeWindow=+window would result in time range:

{ 
  from: valueTimeTS,
  to: valueTimeTS+window
}

?valueTime=TS&valueTimeWindow=-window would result in time range:

{ 
  from: valueTimeTS-window,
  to: valueTimeTS
}

@dprokop dprokop requested a review from ryantxu August 20, 2019 14:15
Copy link
Member

@ryantxu ryantxu left a comment

Choose a reason for hiding this comment

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

valueTime and valueTimeWindow seem kinda specific

what about the parameters time and timeWindow, or maybe time.window?

I would expect time.window to take a duration string, like '5m' or '10s' and then do the math to put time in the middle of the window. If you don't want the middle, then you can calculate and set the to/from parameters directly?

I'm not sure now is the time for this change, but I think time should be saved in TimeSrv along with to/from -- this will be useful for anything trying to query or display based on a single time, not the range

@dprokop
Copy link
Member Author

dprokop commented Aug 21, 2019

what about the parameters time and timeWindow

Yeah, this sounds better.

I would expect time.window to take a duration string, like '5m' or '10s' and then do the math to put time in the middle of the window. If you don't want the middle, then you can calculate and set the to/from parameters directly?

Actually I don't think you could calculate it now, as we don't have support for any expressions in the data links and this would be necessary to increment/decrement value of valueTime variable.

@torkelo
Copy link
Member

torkelo commented Aug 21, 2019

Actually I don't think you could calculate it now, as we don't have support for any expressions in the data links and this would be necessary to increment/decrement value of valueTime variable.

Not sure I understand what you mean. nothing needs to be done in the data links for this:
Imagine an url like
http://localhost:3000/d/5SdHCadmz/panel-tests-graph?time=${__value_time}&time.window=30s

TimeSrv will check for url params time & time.window and set from & to accordingly. At least that is how I imagined this would work.

@dprokop
Copy link
Member Author

dprokop commented Aug 22, 2019

@torkelo I was refering to :

If you don't want the middle, then you can calculate and set the to/from parameters directly?

I have confused from/to(TimeSrv properties) with from/to query params :)

n/w, wil add support for predefined windows!

@dprokop
Copy link
Member Author

dprokop commented Aug 22, 2019

Changed params and added support to time window being specified as interval string or in ms. Discussed with @torkelo and we decided no to focus on ranges relative to __value_time. I.e. case when you want to specify ?from=${__value_time}&to=${__value_time}+5s. Followup issue for supporting such case #18679

@dprokop dprokop added this to In Review in Frontend Platform Backlog via automation Aug 22, 2019
@dprokop dprokop added this to the 6.4 milestone Aug 22, 2019
@dprokop dprokop self-assigned this Aug 22, 2019
Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

Looks good!

@dprokop dprokop merged commit 6d3a05a into master Aug 22, 2019
Frontend Platform Backlog automation moved this from In Review to Done Aug 22, 2019
@dprokop dprokop deleted the data-links/value-time-window branch August 22, 2019 09:42
ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 23, 2019
* grafana/master:
  Streaming: improve JSDocs for DataSourceAPI streaming support (grafana#18672)
  TimeSrv: Enable value time windowing in TimeSrv (grafana#18636)
  Explore: Fixes so Show context shows results again (grafana#18675)
  Graph: Updated y-axis ticks test dashboard (grafana#18677)
  Add typings to package.json in packages (grafana#18640)
  Plugins: better warning when plugins fail to load (grafana#18671)
ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 26, 2019
* grafana/master:
  Heatmap: Add Cividis and Turbo color schemes (grafana#18710)
  Units: add counts/sec (cps) and counts/min (cpm) in Throughput (grafana#18702)
  Dev Docker: Use golang:1.12.9-alpine to prevent glibc mismatch. (grafana#18701)
  Docs: Fix broken link for the Grafana on RHEL or Ubuntu tutorial (grafana#18697)
  Fixes several usability issues with QueryField component  (grafana#18681)
  convert teams section of user profile to react (grafana#18633)
  Singlestat/Gauge/BarGauge: Improvements to decimals logic and added test dashboard (grafana#18676)
  Emails: Change text (grafana#18683)
  Streaming: improve JSDocs for DataSourceAPI streaming support (grafana#18672)
  TimeSrv: Enable value time windowing in TimeSrv (grafana#18636)
  Explore: Fixes so Show context shows results again (grafana#18675)
  Graph: Updated y-axis ticks test dashboard (grafana#18677)
  Add typings to package.json in packages (grafana#18640)
  Plugins: better warning when plugins fail to load (grafana#18671)
  SingleStat2: save options to defaults not override (grafana#18666)
  Packages: Fix path import from grafana/data (grafana#18667)
ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 26, 2019
* grafana/master:
  @grafana/data: improve the CircularVector api (grafana#18716)
  QueryEditor: check if optional func toggleEditorMode is provided (grafana#18705)
  Emails: remove the yarn.lock (grafana#18724)
  OAuth: Support JMES path lookup when retrieving user email (grafana#14683)
  Emails: resurrect template notification (grafana#18686)
  Email: add reply-to and direct attachment (grafana#18715)
  Dashboard: Adds Logs Panel (alpha) as visualization option for Dashboards (grafana#18641)
  Heatmap: Add Cividis and Turbo color schemes (grafana#18710)
  Units: add counts/sec (cps) and counts/min (cpm) in Throughput (grafana#18702)
  Dev Docker: Use golang:1.12.9-alpine to prevent glibc mismatch. (grafana#18701)
  Docs: Fix broken link for the Grafana on RHEL or Ubuntu tutorial (grafana#18697)
  Fixes several usability issues with QueryField component  (grafana#18681)
  convert teams section of user profile to react (grafana#18633)
  Singlestat/Gauge/BarGauge: Improvements to decimals logic and added test dashboard (grafana#18676)
  Emails: Change text (grafana#18683)
  Streaming: improve JSDocs for DataSourceAPI streaming support (grafana#18672)
  TimeSrv: Enable value time windowing in TimeSrv (grafana#18636)
  Explore: Fixes so Show context shows results again (grafana#18675)
  Graph: Updated y-axis ticks test dashboard (grafana#18677)
  Add typings to package.json in packages (grafana#18640)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants