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.

Sign up for GitHub

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 22, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5e48ee1
feat(agentsdk): add `StartupLogsSender` and `StartupLogsWriter`
mafredri Jun 21, 2023
d078203
add test for cr
mafredri Jun 21, 2023
c80755e
improve documentatiaon
mafredri Jun 21, 2023
cdb64e2
simplify log writer in favor of documentation
mafredri Jun 21, 2023
867a66e
minor correctness fix for writer
mafredri Jun 21, 2023
0c148ca
fix typo
mafredri Jun 21, 2023
e247bb4
prevent a potential race condition on select during send
mafredri Jun 21, 2023
e98b274
fix use of wrong context in error
mafredri Jun 21, 2023
9ac5d60
fix startup log level streaming via websocket
mafredri Jun 21, 2023
27923a3
Update codersdk/agentsdk/logs_test.go
mafredri Jun 22, 2023
02bea6e
Merge branch 'main' into mafredri/feat-add-startup-logs-sender-writer…
mafredri Jun 22, 2023
b2b4993
Merge branch 'main' into mafredri/feat-add-startup-logs-sender-writer…
mafredri Jun 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
simplify log writer in favor of documentation
  • Loading branch information
@mafredri
mafredri committed Jun 21, 2023
commit cdb64e203bca4569f0071304c4063ffc3713dc23
36 changes: 8 additions & 28 deletions codersdk/agentsdk/logs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"errors"
"io"
"net/http"
"sync"
"time"

"golang.org/x/xerrors"
Expand All @@ -17,25 +16,13 @@ import (
)

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

// The mutex is only required to ensure that calling Close is
// concurrently safe with calling Write.
mu sync.Mutex // Protects following.
closed bool
buf bytes.Buffer // Buffer to track partial lines.

buf bytes.Buffer // Buffer to track partial lines.
ctx context.Context
send func(ctx context.Context, log ...StartupLog) error
level codersdk.LogLevel
}

func (w *startupLogsWriter) Write(p []byte) (int, error) {
w.mu.Lock()
closed := w.closed
w.mu.Unlock()
if closed {
return 0, xerrors.New("writer is closed")
}

n := len(p)
for len(p) > 0 {
nl := bytes.IndexByte(p, '\n')
Expand All @@ -48,13 +35,10 @@ func (w *startupLogsWriter) Write(p []byte) (int, error) {
}

var partial []byte
w.mu.Lock()
if w.buf.Len() > 0 {
partial = w.buf.Bytes()
w.buf.Reset()
}
w.mu.Unlock()

err := w.send(w.ctx, StartupLog{
CreatedAt: time.Now().UTC(), // UTC, like database.Now().
Level: w.level,
Expand All @@ -66,9 +50,7 @@ func (w *startupLogsWriter) Write(p []byte) (int, error) {
p = p[nl+1:]
}
if len(p) > 0 {
w.mu.Lock()
_, err := w.buf.Write(p)
w.mu.Unlock()
if err != nil {
return n, err
}
Expand All @@ -77,12 +59,6 @@ func (w *startupLogsWriter) Write(p []byte) (int, error) {
}

func (w *startupLogsWriter) Close() error {
w.mu.Lock()
defer w.mu.Unlock()
if w.closed {
return xerrors.New("writer is already closed")
}
w.closed = true
if w.buf.Len() > 0 {
defer w.buf.Reset()
return w.send(w.ctx, StartupLog{
Expand All @@ -94,10 +70,14 @@ func (w *startupLogsWriter) Close() error {
return nil
}

// StartupLogsWriter returns an io.WriteCloser that sends logs to the
// server. When closed, any remaining partially written logs will be
// sent to the server. If the context passed to StartupLogsWriter is
// StartupLogsWriter returns an io.WriteCloser that sends logs via the
// provided sender. The sender is expected to be non-blocking. Calling
// Close flushes any remaining partially written log lines but is
// otherwise no-op. If the context passed to StartupLogsWriter is
// canceled, any remaining logs will be discarded.
//
// Neither Write nor Close is not safe for concurrent use and must be
// used by a single goroutine.
func StartupLogsWriter(ctx context.Context, sender func(ctx context.Context, log ...StartupLog) error, level codersdk.LogLevel) io.WriteCloser {
return &startupLogsWriter{
ctx: ctx,
Expand Down
4 changes: 1 addition & 3 deletions codersdk/agentsdk/logs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,9 +208,7 @@ func TestStartupLogsWriter_Write(t *testing.T) {
require.Equal(t, tt.want, got)

err := w.Close()
if tt.closeFirst {
require.Error(t, err)
} else if (err != nil) != tt.wantErr {
if !tt.closeFirst && (err != nil) != tt.wantErr {
t.Errorf("startupLogsWriter.Close() error = %v, wantErr %v", err, tt.wantErr)
return
}
Expand Down