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

SceneTimeRange: Support delay now to avoid data drops in charts #509

Merged
merged 5 commits into from
Jan 6, 2024

Conversation

ivanortegaalba
Copy link
Collaborator

@ivanortegaalba ivanortegaalba commented Dec 19, 2023

Dashboards PR: grafana/grafana#79703

This enables support for UNSAFE_nowDelay in Dashboards. It is prefixed as UNSAFE because we don't know how used and useful is this.

This allows us to override the now time by entering a time delay. This option accommodates known delays in data aggregation to avoid null values.

b2aa17d125dfc0aa8caee873760a8f44ebf2dcab

@ivanortegaalba ivanortegaalba added the usecase/core-dashboards A feature we need to be 100% compatible with current dashboards label Dec 19, 2023
@torkelo
Copy link
Member

torkelo commented Dec 19, 2023

I wonder how much this option is actually for used. It has worked a bit badly for years , wonder if we should consider depreciating it.

@@ -42,7 +48,8 @@ export class SceneTimeZoneOverride
this.state.from,
this.state.to,
this.state.timeZone,
this.getAncestorTimeRange().state.fiscalYearStartMonth
this.getAncestorTimeRange().state.fiscalYearStartMonth,
this.state.nowDelay
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.state.nowDelay
this.getAncestorTimeRange().state.nowDelay,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why should we use the nowDelay from the parent only if the timezone is gotten from it?

If we do that, the nowDelay will be only inherited from the ancestor if the timezone is not defined and if the ancestor time range changes.

The delay should be always the defined one, although we want to inherit any delay from parents, which implies many more changes, since we need to always get the delay from the ancestor as we do for the timezone, not only in the evaluation of the value when the timezone changes.

Am I missing something @torkelo? 🤔

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.

I think this looks good (with one minor issue)

it is a feature I am interested in how many actually use, and if we can deprecate it maybe, or find a simpler implementation for. Wonder if we can mark it as internal here in scenes lib?

The problem with how it's imlemented now (and here) s that it causes simple time ranges like Last 15 minutes to read like now-30m to now-1m . I think this now delay should not change how intervals are displayed, just how "now" is evaluated.

An alternative change would be to introduce a global "nowDelay" state that we check in dateMath.parse , so it only changes how we interprete "now", the time range picker and the display of the selected time range does not need to change.

thoughts @dprokop @thanos-karachalios ?

@ivanortegaalba
Copy link
Collaborator Author

I wonder how much this option is actually for used. It has worked a bit badly for years, wonder if we should consider depreciating it.

I opened this PR to track the usage of this setting, so we can track how many dashboards that are actually used contains those settings grafana/grafana#79744

@dprokop
Copy link
Member

dprokop commented Dec 20, 2023

Wonder if we can mark it as internal here in scenes lib?

I think this is a good idea. Let's gather some insights on the usage (thanks @ivanortegaalba for opening the PR!) and decide based on that if deprecation is a way to go.

@torkelo @ivanortegaalba do we agree on continuing with the _UNSAFE_internalPropertyName naming convention for such things? Ref #396

@ivanortegaalba
Copy link
Collaborator Author

Wonder if we can mark it as internal here in scenes lib?

I think this is a good idea. Let's gather some insights on the usage (thanks @ivanortegaalba for opening the PR!) and decide based on that if deprecation is a way to go.

@torkelo @ivanortegaalba do we agree on continuing with the _UNSAFE_internalPropertyName naming convention for such things? Ref #396

I'm ok with that. I like the way to mark it as "do not use it". I'll apply the changes

@ivanortegaalba
Copy link
Collaborator Author

@dprokop I renamed the state property from delayNow to UNSAFE_delayNow. Once you give me the ✅, I'll merge

@ivanortegaalba
Copy link
Collaborator Author

ivanortegaalba commented Jan 5, 2024

The problem with how it's imlemented now (and here) s that it causes simple time ranges like Last 15 minutes to read like now-30m to now-1m . I think this now delay should not change how intervals are displayed, just how "now" is evaluated.

@torkelo I also fixed the display issue with the time range
Captura de pantalla 2024-01-05 a las 22 37 55

@torkelo
Copy link
Member

torkelo commented Jan 6, 2024

@ivanortegaalba great, I think this is ready for merge then!

@ivanortegaalba ivanortegaalba merged commit 11e8212 into main Jan 6, 2024
3 checks passed
@ivanortegaalba ivanortegaalba deleted the implement-delay-now branch January 6, 2024 08:40
@ivanortegaalba ivanortegaalba added patch Increment the patch version when merged release Create a release when this pr is merged labels Jan 6, 2024
@grafanabot
Copy link
Contributor

🚀 PR was released in v1.28.6 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged release Create a release when this pr is merged released usecase/core-dashboards A feature we need to be 100% compatible with current dashboards
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants