Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
mafredri merged 12 commits intomainfrommafredri/fix-coderd-startup-log-eof
Jun 16, 2023

Conversation

mafredri
Copy link
Member

@mafredrimafredri commentedJun 14, 2023
edited
Loading

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]

@mafredrimafredri changed the titlefix: Send startup log EOF and prevent lost messagesfix: send startup log EOF and prevent lost messagesJun 14, 2023
@mafredrimafredriforce-pushed themafredri/fix-coderd-startup-log-eof branch 5 times, most recently from9b3319b to798b1a2CompareJune 14, 2023 16:17
@mafredrimafredriforce-pushed themafredri/fix-coderd-startup-log-eof branch from798b1a2 to624e367CompareJune 14, 2023 16:18
Copy link
Member

@mtojekmtojek 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
MemberAuthor

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 likeSELECT 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.

mtojek reacted with thumbs up emoji

for {
var logs []WorkspaceAgentStartupLog
Copy link
MemberAuthor

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)

mtojek reacted with thumbs up emoji
@mafredri
Copy link
MemberAuthor

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)

@mafredrimafredri requested a review frommtojekJune 16, 2023 13:20
Copy link
Member

@mtojekmtojek left a comment

Choose a reason for hiding this comment

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

👍

@mtojekmtojek self-requested a reviewJune 16, 2023 13:48
@mafredrimafredri changed the titlefix: send startup log EOF and prevent lost messagesfix: Avoid missed logs when streaming startup logsJun 16, 2023
@mafredrimafredri changed the titlefix: Avoid missed logs when streaming startup logsfix: avoid missed logs when streaming startup logsJun 16, 2023
@mafredrimafredri merged commit0c50774 intomainJun 16, 2023
@mafredrimafredri deleted the mafredri/fix-coderd-startup-log-eof branchJune 16, 2023 14:14
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJun 16, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@mtojekmtojekmtojek approved these changes

@kylecarbskylecarbsAwaiting requested review from kylecarbs

Assignees

@mafredrimafredri

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@mafredri@mtojek

[8]ページ先頭

©2009-2025 Movatter.jp