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

[Unified observability] Fix refresh button in the overview page #126927

Merged
merged 25 commits into from
Mar 16, 2022

Conversation

afgomez
Copy link
Contributor

@afgomez afgomez commented Mar 4, 2022

Summary

Fixes the refresh button in the overview page.

The state for the time range for the overview page was handled in the URL. The only value present there was the relative range (now-15m...now and similar).

The different panels were reading from this value to determine if they had to re-render or not. The refresh button updated these values in the URL... but they stayed the same because they were relative values.

What we want instead is to use the absolute values of the range (the resolved values for now-15m and now). However the current solution had two issues:

  • The absolute values were recalculated on each re-render.
  • Each panel had its own absolute values, since the state of useTimeRange wasn't shared.

This PR addresses this two issues by creating a new context for the time ranges. This caches the absolute values so they don't update on each re-render and reliably determines when they need to be updated. It also functions as a source of truth for the range state instead of leveraging the URL.

I took the opportunity to also handle here the refresh information for the datepicker.

@afgomez afgomez added release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed Team:Unified observability v8.2.0 labels Mar 4, 2022
@afgomez afgomez requested review from a team as code owners March 4, 2022 16:03
@elasticmachine
Copy link
Contributor

Pinging @elastic/unified-observability (Team:Unified observability)

Alejandro Fernández Gómez added 2 commits March 7, 2022 15:43
Copy link
Contributor

@estermv estermv left a comment

Choose a reason for hiding this comment

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

I did some manual testing and I found a couple of things:

  1. When navigating to the overview page, the timerange is not added to the URL, it's only added when the user interacts with the timepicker. In other apps (tried APM and uptime) it is added when the user navigates to it. Should we do the same?
    It could be a problem when someone wants to save the URL or share it, and they come from a different app where they modified the timerange.
    If it's too much to do in this PR, it should be fine if we open a follow-up issue.

  2. When the timerange selected is today, this week or start and end are absolute, the Refresh button doesn't work:

Screen.Recording.2022-03-08.at.15.26.17.mov

I think it's because the timerange doesn't change, but we should do the request again as there could be more data (the only request done is from the alerts table).

@afgomez
Copy link
Contributor Author

afgomez commented Mar 8, 2022

When navigating to the overview page, the timerange is not added to the URL

I can use a useEffect inside the DatePickerContext to add the values to the URL on first load if they don't exist.

When the timerange selected is today, this week or start and end are absolute, the Refresh button doesn't work

Mmmm, good catch. I guess in those the resolved absolute values don't change.

I can expose the lastUpdated value from the DatePickerContext and use that to trigger the refetch.

Let me address both

@afgomez afgomez requested a review from estermv March 9, 2022 10:50
Copy link
Contributor

@awahab07 awahab07 left a comment

Choose a reason for hiding this comment

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

Functionality works well.

There's a bit of inconsistency of when the range appears or when it doesn't in the url on overview page. When link is clicked, there's no range (though it does appear momentarily). But when page is refreshed, it is there in the url. Is it intentional? It is different than how it was behaving before the PR changes.

Before

Screen.Recording.2022-03-09.at.14.35.24.mov

After

Screen.Recording.2022-03-09.at.14.38.20.mov

@afgomez
Copy link
Contributor Author

afgomez commented Mar 10, 2022

When link is clicked, there's no range (though it does appear momentarily). But when page is refreshed, it is there in the url

@awahab07 it is intentional to show the range in the URL on page load, but it shouldn't flicker like you describe.

I cannot reproduce this behaviour on Firefox, Safari or Chrome. Maybe it was a hiccup of your local install caused when switching branches?

@awahab07
Copy link
Contributor

awahab07 commented Mar 10, 2022

@afgomez yes, it could be due to switching branches. I'll rebuild everything and test again.

@awahab07
Copy link
Contributor

@afgomez I've checked after a fresh build, I still see the same issue (on refresh, url contains time range). I am connecting my local Kibana to a remote edge cluster, probably that's the difference.

@afgomez afgomez requested a review from awahab07 March 14, 2022 12:31
Copy link
Contributor

@awahab07 awahab07 left a comment

Choose a reason for hiding this comment

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

LGTM (except for some failing tests at the time of reviewing)

  • Time range values are correctly preserved across Observability Overview Page and plugins (I focused only on Uptime and User Experience plugins).
  • Absolute and relative time values are correctly passed in requests.
  • Refresh button works well on Uptime and UX plugin.
  • Sharing url correctly populates the time range.

Comment on lines +78 to +90
const absoluteStart = useMemo(
() => getAbsoluteTime(relativeStart),
// `lastUpdated` works as a cache buster
// eslint-disable-next-line react-hooks/exhaustive-deps
[relativeStart, lastUpdated]
);

const absoluteEnd = useMemo(
() => getAbsoluteTime(relativeEnd, { roundUp: true }),
// `lastUpdated` works as a cache buster
// eslint-disable-next-line react-hooks/exhaustive-deps
[relativeEnd, lastUpdated]
);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's always better to not cache derived values, if possible.

The absolute* values are consumed outside of useFetcher as well, see

  const min = moment.utc(absoluteStart).valueOf();
  const max = moment.utc(absoluteEnd).valueOf();

So it's a question of whether getAbsoluteTime is more expensive than moment.utc. If it is, then caching is serving a purpose here. One advantage of leaving them cached in context is that if there were future use cases where plugins consume absolute time values, they won't have to re-calculate.

@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Mar 14, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@afgomez
Copy link
Contributor Author

afgomez commented Mar 15, 2022

@elasticmachine merge upstream

Copy link
Contributor

@estermv estermv left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@afgomez
Copy link
Contributor Author

afgomez commented Mar 15, 2022

@elasticmachine merge upstream

@afgomez
Copy link
Contributor Author

afgomez commented Mar 16, 2022

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observability 362 365 +3

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
observability 355 358 +3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
observability 398.9KB 398.1KB -828.0B
ux 170.1KB 170.2KB +137.0B
total -691.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
observability 87.4KB 89.6KB +2.2KB
Unknown metric groups

API count

id before after diff
observability 358 361 +3

ESLint disabled line counts

id before after diff
observability 37 39 +2
ux 7 13 +6
total +8

Total ESLint disabled count

id before after diff
observability 39 41 +2
ux 8 14 +6
total +8

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@afgomez afgomez merged commit a13e99c into elastic:main Mar 16, 2022
@afgomez afgomez deleted the 125799-fix-overview-datepicker branch March 16, 2022 08:12
@kibanamachine
Copy link
Contributor

⚪ Backport skipped

The pull request was not backported as there were no branches to backport to. If this is a mistake, please apply the desired version labels or run the backport tool manually.

Manual backport

To create the backport manually run:

node scripts/backport --pr 126927

Questions ?

Please refer to the Backport tool documentation

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 126927 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 18, 2022
maksimkovalev pushed a commit to maksimkovalev/kibana that referenced this pull request Mar 18, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 126927 or prevent reminders by adding the backport:skip label.

@smith smith added v9.0.0 and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Mar 21, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 126927 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 22, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 126927 or prevent reminders by adding the backport:skip label.

@smith smith added backport:skip This commit does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. v9.0.0 labels Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Unified observability Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants