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

[Fleet] Fix syncing issue in Agent flyout #135734

Merged
merged 4 commits into from
Jul 5, 2022

Conversation

criamico
Copy link
Contributor

@criamico criamico commented Jul 5, 2022

Summary

Closes #131590

In the instructions I added a setInterval component that every 10 seconds refreshes the AgentStatus and AgentPolicies. Also remove the `isFleetServerHealthy because it doesn't update until a page refresh (preventing the layout to change) and simplified a bit the other checks.
I looked for another way of syncing the panel with the main page but didn't find a better solution.

Repro steps

  • On a fresh Kibana setup navigate to Fleet tab.
  • Observe Add Fleet Server layout.
  • Install a new agent using Advanced>Quickstart mode.
  • Click Continue enrolling Elastic Agent button
  • The flyout should sync with main page and show the correct Add agent layout and not the Add Fleet server instructions

@criamico criamico self-assigned this Jul 5, 2022
@criamico criamico added v8.4.0 v8.3.2 auto-backport Deprecated - use backport:version if exact versions are needed Team:Fleet Team label for Observability Data Collection Fleet team release_note:fix labels Jul 5, 2022
@criamico criamico marked this pull request as ready for review July 5, 2022 16:16
@criamico criamico requested a review from a team as a code owner July 5, 2022 16:16
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@kpollich kpollich self-requested a review July 5, 2022 16:18
@@ -42,11 +40,16 @@ export const Instructions = (props: InstructionProps) => {
refreshAgentPolicies,
} = props;
const fleetStatus = useFleetStatus();
const { isUnhealthy: isFleetServerUnhealthy } = useFleetServerUnhealthy();
const REFRESH_INTERVAL = 10 * 1000;
Copy link
Member

Choose a reason for hiding this comment

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

there is no way to fix the root problem and refresh the status when fleet server is added? or at least only once when the flyout is open?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason when the flyout is first open the fleetStatus is not ready, so you have to reload the page to get it. I couldn't find a solution that refreshes the state, I tried in several ways using useEffect but they mostly ended up int too many rerenders errors.

@@ -42,11 +40,16 @@ export const Instructions = (props: InstructionProps) => {
refreshAgentPolicies,
} = props;
const fleetStatus = useFleetStatus();
const { isUnhealthy: isFleetServerUnhealthy } = useFleetServerUnhealthy();
const REFRESH_INTERVAL = 10 * 1000;
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if polling is the right solution here. Is there a way we can add fleetStatus.refresh to wherever we currently wait on the the existence of a Fleet Server? Because fleetStatus is a context object, we should be able to call refresh and update any other consumers of that context object from anywhere in the component tree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried using fleetStatus.refresh without the setInterval as a first try and didn't work as expected.
An issue that I had a lot with this component is that react signals too many rerenders, which is probably a sign that we are doing something weird.

The solutions I could think of are basically 1- the polling that I implemented here 2- doing as many checks as possible in the parent components of the flyout, but since is called several times in the code it would be a lot of refactoring for a small bug fix.

Copy link
Member

@nchaulet nchaulet Jul 5, 2022

Choose a reason for hiding this comment

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

@criamico if you put the fleetStatus.refresh in a useEffect with an empty array it should only refresh once no?
something like this
useEffect(() => fleetStatus.refresh(), []);

@kpollich
Copy link
Member

kpollich commented Jul 5, 2022

Summarizing the changes I made in 6bd8326:

  • We were checking for the Fleet Server being ready using a separate piece of state than what's stored in the fleetStatus context object provided by the useFleetStatus hook
  • We rely on the fleetStatus object from that hook to determine whether or not to show the "missing fleet server" enrollment instructions
  • So, we needed two sources of state there, because we don't want to have the enrollment instructions disappear as soon as a fleet server is detected
  • I added a separate flag for forceDisplayInstructions to the fleet status context object that allows us to capture that "two sources of truth" concept while still using the fleetStatus object
  • The above allows us to use fleetStatus.refresh when we're polling for a Fleet Server, which syncs the flyout and the instructions views

Long screen recording of the experience as it stands now on a fresh self hosted instance (local dev):

Screen.Recording.2022-07-05.at.2.19.18.PM.mov

@kpollich
Copy link
Member

kpollich commented Jul 5, 2022

This still isn't quite right, because we're stuck in the "Fleet Server" state in the enrollment flyout, so I'll continue to look at that. We're closer now though I think.

@kpollich
Copy link
Member

kpollich commented Jul 5, 2022

Never mind on the above - we don't auto select the Fleet Server Policy 1 policy that we create so at least we're not stuck when the flyout opens after clicking "continue".

Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

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

🚀

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
fleet 839.8KB 839.8KB +2.0B

Page load bundle

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

id before after diff
fleet 111.2KB 111.2KB +11.0B

History

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

cc @criamico

@kpollich kpollich merged commit b8dcdea into elastic:main Jul 5, 2022
kibanamachine pushed a commit that referenced this pull request Jul 5, 2022
* [Fleet] Fix syncing issue in Agent flyout

* Use context based solution instead of polling

* Add clarifying comment

* Fix types

Co-authored-by: Kyle Pollich <[email protected]>
(cherry picked from commit b8dcdea)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.3

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Jul 5, 2022
* [Fleet] Fix syncing issue in Agent flyout

* Use context based solution instead of polling

* Add clarifying comment

* Fix types

Co-authored-by: Kyle Pollich <[email protected]>
(cherry picked from commit b8dcdea)

Co-authored-by: Cristina Amico <[email protected]>
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 release_note:fix Team:Fleet Team label for Observability Data Collection Fleet team v8.3.2 v8.4.0
Projects
None yet
6 participants