disable fetching alertmanagers on status page in agent mode

[rlankfo]

Took a shot at fixing #9928. This disables fetching alertmanagers from /status page in agent mode.

[juliusv]

Any reason for the capitalization of these functions? It seems unrelated to the rest of the change.

[rlankfo]

The linter didn't like fetchResult being lowercase:

Failed to compile.

src/pages/status/Status.tsx
  Line 94:30:  React Hook "useFetch" is called in function "fetchResult" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter  react-hooks/rules-of-hooks
  Line 95:30:  React Hook "useFetch" is called in function "fetchResult" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter  react-hooks/rules-of-hooks
  Line 96:30:  React Hook "useFetch" is called in function "fetchResult" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter  react-hooks/rules-of-hooks

Search for the keywords to learn more about each error.

[rlankfo]

Just capitalizing these may not be the best approach but it seemed to appease the linter. Open to suggestions here?

[juliusv]

Ah, some of those hook-related linter rules are really annoying, because they are not smart enough to understand enough context much of the time. In this case, the upper-casing only shuts up the linter because it now thinks that we are defining a React component, which we are not (it is actually a custom hook function, but the linter doesn't recognize it as such), so we just "tricking" the linter into believing something wrong to shut it up. Could you try renaming these to useFetchResult instead? Hopefully if they start with useXXX, the linter will detect them as custom hooks.

[rlankfo]

in this case, the linter complains now about using a hook in a callback:

  Line 101:48:  React Hook "useFetchResult" cannot be called inside a callback. React Hooks must be called in a React function component or a custom React Hook function  react-hooks/rules-of-hooks```

[juliusv]

Ah yeah, the general pattern here of calling hooks from a map callback is something that isn't super kosher, as it makes it less certain to the linter that the hooks are always all called, and in the same order (https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level). But that was already the problem before.

If we really wanted to abide by the "rules of hooks" here, we would have to restructure everything, probably just write everything out...

[rlankfo]

I moved this logic into a new functional component. Does this work?

[juliusv]

Thanks, looks good to me besides the comment! Could you change the branch to merge into to be release-2.32 ("Edit" at the top and then change the target branch), since we merge bug fixes into the latest release branch?

[juliusv]

Ah yeah, great! But I think you don't need to pass agentMode in here, right?

[juliusv]

Ah already fixed :)

[rlankfo]

right, noticed this a moment ago as well.

[juliusv]

Looks good now, but would be great to add a regression test for this to https://github.com/prometheus/prometheus/blob/main/web/ui/react-app/src/pages/status/Status.test.tsx - that's a snapshot-based test. I think you can just duplicate the few it('should match table snapshot', () => { ... lines with the Agent mode enabled and then update the saved snapshots for that by pressing i when running the web tests locally (see https://jestjs.io/docs/snapshot-testing#interactive-snapshot-mode) via npm run test from the web/ui directory.

[rlankfo]

Looks good now, but would be great to add a regression test for this to https://github.com/prometheus/prometheus/blob/main/web/ui/react-app/src/pages/status/Status.test.tsx - that's a snapshot-based test. I think you can just duplicate the few it('should match table snapshot', () => { ... lines with the Agent mode enabled and then update the saved snapshots for that by pressing i when running the web tests locally (see https://jestjs.io/docs/snapshot-testing#interactive-snapshot-mode) via npm run test from the web/ui directory.

hmm ok, I tried to create a quick test but it seems like the existing tests cover <StatusContent> component, which gets the useFetch results, after agentMode has been evaluated. I attempted to add a quick test for <Status> but ran into some issues, and I imagine I would need to find a way to mock the results of useFetch? I tried something like this but will have to circle back around on this as it's getting pretty late for me here:

    it('should match table snapshot with agent mode', () => {
      const wrapper = shallow(<Status agentMode={true} />);
      expect(toJson(wrapper)).toMatchSnapshot();
      jest.restoreAllMocks();
    });

[juliusv]

Ah, you are right of course. Sorry, didn't see it tested only the inner component. I think it's good enough for now then, but it would definitely be good to add tests for the outer component somehow later.

Will merge for now, thanks! 👍

[rlankfo]

thanks @juliusv 🙏