React UI: Add Starting Screen to Individual Pages

[LeviHarrison]

Currently, when the starting screen finishes it redirects to the home page, even if you didn't start there. Also, because readiness is checked only on the site's initial load, navigating to other pages returns an error as before. This PR adds the starting screen to each individual page, and on completion, the page reloads (to re-fetch data), leaving the user on the view they requested. As it's no longer needed, I removed the /starting page.

#8662 (comment)

[juliusv]

Hmmm, so now this is doing a request to /-/ready on every route change, rather than only on the initial page load (which would be sufficient). I can see two ways of preventing that: Either wrapping some global loader component / context around everything that conveys the loading state to the children (all pages), or maybe it's even a rare ok use case for a global JS variable that gets set to true when the app is ready and short-circuits subsequent fetches. If we went the context route, we already have a theme context (

prometheus/web/ui/react-app/src/App.tsx

Line 68 in d44b4c5

<ThemeContext.Provider

) and a path prefix context (

prometheus/web/ui/react-app/src/App.tsx

Line 72 in d44b4c5

<PathPrefixContext.Provider value={basePath}>

) which are wrapped around everything, and I guess we could have a similar StartingContext. Or maybe there's an even better way to solve it?

[LeviHarrison]

Hmmm, so now this is doing a request to /-/ready on every route change, rather than only on the initial page load (which would be sufficient).

Ah, going from the initial comment I thought we wanted that behavior removed.

I don't see any clear advantage between a global variable and a context, besides that a global might be simpler. Both could start off true, and then become false if the check on the initial page load determines the server isn't ready, and then the starting screen could set it back to true when it's done. Up to you.

One other thing to consider is React not liking conditional hooks, which means that any time we include one of our useFetch hooks in a component, it has to be run no matter what. This is why /-/ready checks happen after the data is fetched (because of the possibility of returning early), even though if the server isn't ready, the data will never be used, making it an unnecessary fetch. We might just have to live with it though.

[juliusv]

Ah, going from the initial comment I thought we wanted that behavior removed.

Ah yeah, so I meant that each page should react to the current readiness state separately, but since we're a single-page app with multiple routes that can be navigated to without a full page reload, we can just load the readiness state once initially upon load of the SPA and then consume it from everywhere.

One other thing to consider is React not liking conditional hooks, which means that any time we include one of our useFetch hooks in a component, it has to be run no matter what. This is why /-/ready checks happen after the data is fetched (because of the possibility of returning early), even though if the server isn't ready, the data will never be used, making it an unnecessary fetch. We might just have to live with it though.

Yes, you always have to use the same hooks within one component, but the hooks can internally decide whether they need to do an actual HTTP fetch or not. So if we went for the global variable variant, I would put boolean ready global variable next to the useFetchReady() implementation that is set whenever the first actual fetch() returns that Prometheus is ready, and then whenever useFetchReady() is subsequently called, it sees that ready is already true, in which case it would no longer execute an actual fetch(), but just immediately return that Prometheus is ready. Since that's the smallest change we can do here and seems not too terrible (although I'd always be careful about global variables otherwise), I think that would be good for now. And then later we can still decide on whether it's cleaner to move it to a context.

[LeviHarrison]

OK, now a /-/ready fetch is done only on the initial load and its response is saved. The starting screen's useFetchReadyInterval can set wasReady to true again.

[juliusv]

I'm a bit confused by this string of AND conditions: So e.g. if it's not ready, but it's still loading, then we already return true?

Intuitively I would have expected something closer to:

Suggested change

	if (!ready && !isLoading && !isUnexpected) {
	return ready && !isLoading && !isUnexpected;

[LeviHarrison]

I think that function could be simplified, but my logic here is that we can only determine that the server is on but not ready if:

1. The variable we get indicates it's not ready
2. We have stopped loading.
3. The response is not an error.

If the response is an error, that means something is wrong with the server or it's not on, so we want to let those error messages show and return true. We could probably change this to be showReadyScreen, where we return true in the case that we can determine that we should show the ready screen, and false for everything else.

[juliusv]

Yeah, that would be more along my expectation - treat anything as not ready unless we positively get a working ready message back from Prometheus. Otherwise, the startup indicator component should be responsible for saying "nope it's not ready yet" and either show the loading state or the readiness-loading error.

[juliusv]

I tried adding time.Sleep(30 * time.Second) in main.go before localStorage.Set(db, startTimeMargin) to simulate a startup sequence. It correctly shows the starting spinner on individual pages now, but if I navigate to a different page, it already tries to render that page (yielding a "Service Unavailable" error) although the startup hasn't finished yet... if I change the readiness condition check to the one I mentioned in #8909 (comment), then it seems to do the right thing though.

[juliusv]

Oh whoops, it doesn't. It then keeps reloading the whole page after everything is ready, so need to look a bit more.

[juliusv]

Ok, so the issue is that the continuous update to the readiness state is only maintained in the Starting component, so the parent page never knows when to stop rendering the Starting component because no state updates to it are ever triggered. The most react-ish way is probably to make Starting a wrapper with children, like we already have for the loader of individual page contents (withStatusIndicator() wrapper).

Actually I think it would even be best to put all that startup logic into the existing withStatusIndicator() wrapper as well. Then it would render a startup spinner upon TSDB startup, a normal loading spinner after that, and then the actual page content after that. And we would never even need the full page reload anymore, as that is all just handled via React state updates and re-renderings.

We already use withStatusIndicator() for every page except /graph, but it should be easy enough to make it usable for /graph as well I think.

[LeviHarrison]

So we'll continue to use the useFetchReady function, and we can just keep ready as a piece of state in withStatusIndicator which can then be passed to the Starting component that will set it to true when done.

[juliusv]

@LeviHarrison Yep, that's exactly what I was thinking!

[juliusv]

I just noticed though that withStatusIndicator() is not actually itself responsible for loading anything though, just for displaying an indicator. The loading & state handling is still done by the parent. Gah. We don't actually want to start loading any normal state before the TSDB loading is complete, so probably we can't actually get around adding a full wrapper around all the per-page components :/

[juliusv]

So that would mean keeping a separate start-loader component after all, but then it would have to be wrapped around each page like this:

Where previously you had:

export default Status;

...you could have:

export default withStartingIndicator(Status);

And the withStartingIndicator could be similar to withStatusIndicator(), but actually contain the starting state fetching & handling.

[juliusv]

Nice. I like how all this stuff is now in one place an not in every component.

[juliusv]

Question: doesn't this set isUnexpected to true whenever we get a 200 status code? Should it be an else if and not just an if?

[LeviHarrison]

Oh yeah probably.

[juliusv]

Could you explain the leak a bit? As far as I can see, this is used to control whether the hook state setters are called on a mounted vs. unmounted component, but that shouldn't change anything with regards to actually leaking memory, it would just hide the memory leak warning, right? With the leak being in the form of the still-running fetchStatus(). Would the proper way be to cancel the fetch() request upon unmount? I found https://www.robinwieruch.de/react-warning-cant-call-setstate-on-an-unmounted-component which also presents your workaround, but AFAIU it's more of a warning suppressor than an actual memory leak preventor.

[LeviHarrison]

So I tried using an abort signal to cancel the request and the error still presented itself, although this may require further investigation.

[juliusv]

Hmm, odd. Ok, let's keep it like this for now.

[juliusv]

I think we're one small test failure away from being done:

Failed to compile.

./src/utils/index.ts
  Line 5:10:  'FetchStateReady' is defined but never used  @typescript-eslint/no-unused-vars

[LeviHarrison]

I moved the withStartingIndicator calls to pages/index.ts (feel free to push back on this) and fixed tests.

[LeviHarrison]

I was locked in an endless cycle of eslint not wanting a trailing comma and prettier wanting one in pages/index.ts so I ended up added a prettier ignore.

[juliusv]

Why's this needed vs. just letting prettier do its thing?

[LeviHarrison]

Prettier wants a trailing comma, but when I add one, eslint errors saying there shouldn't be one.

[juliusv]

What the hell. That has never happened to me before, but I'm getting the same. Ok, let's look into that separately later, but that's weird.

[LeviHarrison]

It's not complaining about the trailing comma in the imports in App.tsx, just the one here.

[juliusv]

Now there's just another lint check failing:

/home/circleci/project/web/ui/react-app/src/App.tsx
  7:9  error  Replace `·AlertsPage,·ConfigPage,·FlagsPage,·RulesPage,·ServiceDiscoveryPage,·StatusPage,·TargetsPage,·TSDBStatusPage,·PanelListPage·` with `⏎··AlertsPage,⏎··ConfigPage,⏎··FlagsPage,⏎··RulesPage,⏎··ServiceDiscoveryPage,⏎··StatusPage,⏎··TargetsPage,⏎··TSDBStatusPage,⏎··PanelListPage,⏎`  prettier/prettier

✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

[LeviHarrison]

Yep, working on it now.

[LeviHarrison]

I got one more thing for the starting screen after this is merged, and then I think it's ready for release!