Moving session recording file ops behind an interface #53702
Conversation
eriktate
added
the
no-changelog
lib/events/filesessions/fileops.go
Outdated
| return trace.Wrap(err) | ||
| } | ||
|
|
||
| if err := f.Truncate(n); err != nil { |
There was a problem hiding this comment.
if any of these operation fails, should we keep the file?
There was a problem hiding this comment.
Probably not. The caller of WritePart() removes the file in case of a failure and there's no reason to hang onto the reserved file if we bail out because of a failed truncation.
The individual part files should probably be removed too, but we don't currently remove them on failure (at least not near the callsite) and I don't really want to change effective behavior with this PR
lib/events/filesessions/fileops.go
Outdated
|
|
||
| func loggingClose(ctx context.Context, closer io.Closer, log *slog.Logger, msg string, args ...any) { | ||
| if err := closer.Close(); err != nil { | ||
| log.ErrorContext(ctx, msg, append(args, "error", err)...) |
There was a problem hiding this comment.
Is there anything that we can do to avoid the allocation here? Perhaps maybe the caller should annotate the logger with any desired args via log.With before calling this function? Alternatively if this is only used in a handful of places, perhaps we could get rid of this function and just call close in line?
There was a problem hiding this comment.
Yeah handling the close inline isn't really annoying enough to warrant the helper. Even when the encrypted version is added, that's only like 3 more spots we'll have to do the check. I opted to remove it for now, but preloading the logger is probably the best way to do something like this going forward, so I'll try to keep that in mind for later 👍
eriktate
force-pushed
the
eriktate/split-modular-session-recording-writes
branch
3 times, most recently
from
b23ae13 to
5c9233c
Compare
public-teleport-github-review-bot
bot
removed the request for review
from doggydogworld
eriktate
force-pushed
the
eriktate/split-modular-session-recording-writes
branch
from
1a1949e to
476ebc1
Compare
This PR prepares session recording to use different file operations based on configuration (e.g. encrypted or not). There shouldn't be any functional changes to how recordings are saved included in this PR