Skip to content

feat(codersdk/agentsdk): add StartupLogsSender and StartupLogsWriter #8129

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

Conversation

mafredri
Copy link
Member

@mafredri mafredri commented Jun 21, 2023

As proposed in #8084 (note that the implementation had to change slightly to accommodate the additional use-case):

This commit adds two new agentsdk functions, StartupLogsSender and
StartupLogsWriter that can be used by any client looking to send
startup logs to coderd.

We also refactor the agent to use these new functions.

As a bonus, agent startup logs are separated into "info" and "error"
levels to separate stdout and stderr.

@mafredri mafredri changed the title feat(agentsdk): add StartupLogsSender and StartupLogsWriter feat(codersdk/agentsdk): add StartupLogsSender and StartupLogsWriter Jun 21, 2023
@mafredri mafredri force-pushed the mafredri/feat-add-startup-logs-sender-writer-to-agentsdk branch 2 times, most recently from 6b97ede to 8ce4e7d Compare June 21, 2023 15:44
This commit adds two new `agentsdk` functions, `StartupLogsSender` and
`StartupLogsWriter` that can be used by any client looking to send
startup logs to coderd.

We also refactor the `agent` to use these new functions.

As a bonus, agent startup logs are separated into "info" and "error"
levels to separate stdout and stderr.
@mafredri mafredri force-pushed the mafredri/feat-add-startup-logs-sender-writer-to-agentsdk branch from 8ce4e7d to 5e48ee1 Compare June 21, 2023 16:11
}()

var queue []StartupLog
sendLog = func(callCtx context.Context, log ...StartupLog) error {
Copy link
Member Author

@mafredri mafredri Jun 21, 2023

Choose a reason for hiding this comment

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

Review: Technically we don't need callCtx here right now because we allow the queue/backlog to grow without bounds, but if we need to limit this in the future, accepting a context here makes it much nicer for the user of the SDK, letting them abort if it takes too long.

@mafredri mafredri force-pushed the mafredri/feat-add-startup-logs-sender-writer-to-agentsdk branch from ec496f8 to cdb64e2 Compare June 21, 2023 16:48
"github.com/coder/retry"
)

type startupLogsWriter struct {
Copy link
Member Author

Choose a reason for hiding this comment

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

Review: I went a round of back-and-forth on making this concurrently safe. Ultimately I opted to revert the concurrency safety in favor of keeping the implementation as short/simple as possible (cdb64e2 (#8129)).

@mafredri mafredri marked this pull request as ready for review June 21, 2023 16:54
@mafredri mafredri requested a review from kylecarbs June 21, 2023 16:54
@mafredri mafredri force-pushed the mafredri/feat-add-startup-logs-sender-writer-to-agentsdk branch from 6435f12 to 9ac5d60 Compare June 21, 2023 18:18
mafredri and others added 2 commits June 22, 2023 13:33
Co-authored-by: Marcin Tojek <mtojek@users.noreply.github.com>
@mtojek
Copy link
Member

mtojek commented Jun 22, 2023

Related: #7767

@mafredri mafredri merged commit 3b9b06f into main Jun 22, 2023
@mafredri mafredri deleted the mafredri/feat-add-startup-logs-sender-writer-to-agentsdk branch June 22, 2023 20:29
@github-actions github-actions bot locked and limited conversation to collaborators Jun 22, 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