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

🚀 feat: implement logs streaming for pods #568

Merged
merged 34 commits into from
Sep 29, 2024

Conversation

naineel1209
Copy link
Contributor

closes #555

📑 Description

[BE]

  • Implemented streaming logs of pod continuously to the client using SSE.
  • Utilized previously coded SSE implementation and made channels to stream the logs to the FE.

[FE]

  • Changed logs state to store array of strings to easily manage logs by using logs.join("\n").
  • Implemented logStream function which streams the logs and sets it in the state.
  • Handled scenario when no logs are available, container tabs are changed and when the log modal is cancelled or OKed.
  • Introduced a new logsSignalController state to be passed to the logStream function to handle the SSE connection scenarios i.e. close the connection via signal when modal is closed, close the current connection via signal and make a new signal for the new connection when the container tab is changed.

Streamed Logs Demo + No more events fired after the modal is closed:
https://github.com/user-attachments/assets/1f3f5fdf-102d-4697-9188-002092279098

✅ Checks

  • [✅] I have tested my code (provide screenshots or screen recordings of a working solution)
  • [✅] I have performed a self-review of my code

ℹ Additional context

@naineel1209 naineel1209 requested a review from a team as a code owner September 12, 2024 18:26
Copy link
Collaborator

@petar-cvit petar-cvit left a comment

Choose a reason for hiding this comment

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

@naineel1209 thanks for the PR! Looks awesome 🧡

Left some comments on code structure

cyclops-ui/src/utils/api/sse/resources.tsx Outdated Show resolved Hide resolved
cyclops-ctrl/pkg/cluster/k8sclient/client.go Outdated Show resolved Hide resolved
cyclops-ui/yarn.lock Outdated Show resolved Hide resolved
Copy link
Collaborator

@petar-cvit petar-cvit left a comment

Choose a reason for hiding this comment

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

@naineel1209, I tried testing it and noticed that when I fetch the logs, those are streamed as expected, but when the window with my Cyclops is not in focus or I change the tab in my browser, it stops the connection and refetches all the logs again. This results in duplicated logs.

Could Abort controller be at fault? To be honest if its the root cause we can remove it, but could you look into it?

@naineel1209
Copy link
Contributor Author

naineel1209 commented Sep 19, 2024

Hi @petar-cvit, I examined the issue, and it was happening due to the default behaviour of the @microsoft/fetch-event-source library, which closes the SSE events when the document is not visible. This caused our request to close as soon as the tab's visibility changed and restart when the tab was in focus again, resulting in fetching the latest 100 logs and streaming the new logs while our old logs were already present.

I have resolved this issue by setting the openWhenHidden flag to true, which keeps the SSE connection open when the tab's visibility changes.

@petar-cvit
Copy link
Collaborator

@naineel1209, that makes sense. This didn't happen when getting updates for workloads getting a new update would not affect data.

However, if the sse gets disconnected for some other reason (e.g. load balancer having a timeout of one minute) it will still refetch all the logs and append them.

Can you handle the case when see reconnects? We should still keep the openWhenHidden, but we need to handle the case of the sse disconnect

- added `isReset` flag to reset the error modal and logs on every successful connection and future reconnections
@naineel1209
Copy link
Contributor Author

@petar-cvit, I reviewed the disconnection issue that leads to duplicate logs upon reconnection and an unresolved error modal, even after a successful reconnection. To tackle this, I introduced an isReset flag, which ensures that both the logs and the error modal are properly reset upon successful connection and during future reconnections, allowing for a fresh stream of logs.

@naineel1209
Copy link
Contributor Author

Hi @petar-cvit, any updates on this?

Copy link
Collaborator

@petar-cvit petar-cvit left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @naineel1209! I removed the abort controller state since it wasn't used.

Also, we need to cover the case when streaming is disabled, but I will do it in a separate PR

@petar-cvit petar-cvit merged commit 7931018 into cyclops-ui:main Sep 29, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stream pod logs
2 participants