Skip to content

fix: avoid missed logs when streaming startup logs #8029

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

Merged
merged 12 commits into from
Jun 16, 2023

Conversation

mafredri
Copy link
Member

@mafredri mafredri commented Jun 14, 2023

I've been seeing inconsistent behavior with the startup log streaming and while looking at the implementation, I noticed a few issues:

  • There's a delay between fetching initial logs and subscribing to the pubsub for updates, this can result in lost messages
  • We did not have an indicator for "end of log" meaning to accurately discern it, we'd have to subscribe to both startup script and workspace updates and get an up-to-date lifecycle state of the workspace
  • We were relying on pubsub telling us when "end of log" is reached, but this message was never sent

With this refactor, we are not dependent on state from the pubsub and the agent tries to log an EOF at the end.

Thoughts:

  • Currently we don't do anything special with the EOF message, but it could be used to e.g. show exit status as a final log or something similar

Here's an image where the script has completed, but logs are missing (quite a lot of logs):
image

[test]

@mafredri mafredri changed the title fix: Send startup log EOF and prevent lost messages fix: send startup log EOF and prevent lost messages Jun 14, 2023
@mafredri mafredri force-pushed the mafredri/fix-coderd-startup-log-eof branch 5 times, most recently from 9b3319b to 798b1a2 Compare June 14, 2023 16:17
@mafredri mafredri force-pushed the mafredri/fix-coderd-startup-log-eof branch from 798b1a2 to 624e367 Compare June 14, 2023 16:18
@mafredri mafredri requested review from kylecarbs and mtojek June 14, 2023 16:25
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

It is a relatively large PR, but as long as it's consistent with other logs properties (Level, Output, CreatedAt), it is 👍 from me.

@mafredri
Copy link
Member Author

I will add, I'm not super happy with the DB change to facilitate the EOF. Ultimately I'd like to update the query to be like SELECT array_agg(logs.*) AS logs, (agent.lifecycle not starting) AS eof, but that is dependent on functionality not in sqlc: sqlc-dev/sqlc#2163

Open to alternatives.

The main thing IMO is that we want the log fetching query to be able to signal EOF so that consumers know what's up.

for {
var logs []WorkspaceAgentStartupLog
Copy link
Member Author

Choose a reason for hiding this comment

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

This was a gnarly bug, had me scratching my head for a while 😬 (new test cases caught it)

@mafredri
Copy link
Member Author

I didn't like how I left some edge cases in the implementation, so I've tried to address those now with the last few commits.

  • And EOF can be sent by the agent, or it will be forcefully closed via agent lifecycle state change (agents choice)
  • The EOF log entry is sent to consumers and filtered out in the WebUI
  • Fixed a memory bug the in sdk (currently only used by tests, but will be used by ssh)

@mafredri mafredri requested a review from mtojek June 16, 2023 13:20
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

👍

@mtojek mtojek self-requested a review June 16, 2023 13:48
@mafredri mafredri changed the title fix: send startup log EOF and prevent lost messages fix: Avoid missed logs when streaming startup logs Jun 16, 2023
@mafredri mafredri changed the title fix: Avoid missed logs when streaming startup logs fix: avoid missed logs when streaming startup logs Jun 16, 2023
@mafredri mafredri merged commit 0c50774 into main Jun 16, 2023
@mafredri mafredri deleted the mafredri/fix-coderd-startup-log-eof branch June 16, 2023 14:14
@github-actions github-actions bot locked and limited conversation to collaborators Jun 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants