move ProtoReader from lib/events to api/sessionrecording as Reader

[dustinspecker]

This pull request does the following:

• moves ProtoReader from lib/events to api/sessionrecording so that it may be imported by users to parse session recordings in Protobuf binary format
• renames ProtoReader to Reader to avoid leaking implementation detail in name

This pull request is focused on moving and renaming, while #50883 makes changes to implementation and the API.

This pull request should not be merged until #50883 is also ready to be merged, since we do not want to support this API as-is.

changelog: add api/sessionrecording.Reader to support parsing and reading session recordings

[zmb3]

Some additional suggestions for cleanup/improvements.

We should be cautious here, and I'm fine if you want to decline to apply these, or even push them to a future PR so that this PR is more of a direct migration.

[zmb3]

These two fields are only ever used in Read, so they could be extracted to local variables instead of fields on Reader.

[dustinspecker]

Good find. I've moved these to be local to Read.

[fspmarshall]

I can't claim to be an expert on these things, but moving a 64kb buffer into a function-local variable seems iffy to me. It can be easy to accidentally introduce something that causes go to decide to do a heap allocation for the buffer. And even if we're not doing a heap allocation for the buffer, it still means go needs to ensure and zero 64kb of stack space each time Read() is called (potentially tens of thousands of times per second).

I think we should stick to reusing a persistent buffer for the lifetime of the reader.

[dustinspecker]

That sounds good to me and definitely not wanting to impact performance with this changed. I moved sizeBytes as well and messageBytes back to fields on the Reader struct. They are still unexported, so easy to move in the future if something changes.

[dustinspecker]

Some additional suggestions for cleanup/improvements.

We should be cautious here, and I'm fine if you want to decline to apply these, or even push them to a future PR so that this PR is more of a direct migration.

Normally, I would lean towards doing a direct migration. But in this case where we're making this importable then I appreciate the suggestions/improvements to make this easier for us to support consumers (plus easier to change the API now before releasing...). Thank you again for the really thorough reviews!

[zmb3]

1. We should be able to remove type bufferCloser struct { now too.
2. It doesn't look like the Reader's Reset method is ever called, so I would remove it to keep the public API as small as possible.
3. Lastly, maybe we can clear up what all these different readers are with some better names and comments:

• rename reader to rawReader
• rename inner to partReader

// Reader reads Teleport's session recordings
type Reader struct {
	// rawReader is the raw data source we read from
	rawReader io.Reader

	// partReader wraps rawReader and is limited to reading a single
	// (compressed) part from the session recording
	partReader io.Reader

	// gzipReader wraps partReader and decompresses a single part
	// from the session recording
	gzipReader *gzip.Reader

	...
}

[dustinspecker]

1. We should be able to remove type bufferCloser struct { now too.
2. It doesn't look like the Reader's Reset method is ever called, so I would remove it to keep the public API as small as possible.
3. Lastly, maybe we can clear up what all these different readers are with some better names and comments:

• rename reader to rawReader
• rename inner to partReader

// Reader reads Teleport's session recordings
type Reader struct {
	// rawReader is the raw data source we read from
	rawReader io.Reader

	// partReader wraps rawReader and is limited to reading a single
	// (compressed) part from the session recording
	partReader io.Reader

	// gzipReader wraps partReader and decompresses a single part
	// from the session recording
	gzipReader *gzip.Reader

	...
}

1. Thank you - I completely overlooked this. I updated newGzipWriter to take an io.Writer. I'm still leaving our gzipWriter struct since it interacts with the writerPool.
2. I like what you're saying and I'll look at this more. It wasn't as straight forward to remove Reset. We have a number of tests that start failing even though it looks like source code isn't using Reset anywhere. I'll investigate more, but looks like we might have test code testing test code. I want to understand the tests better before just deleting them... I was hoping just constructing a new Reader as a replacement for Reset would suffice, but it looks like some tests may be depending on lastIndex persisting across multiple provided io.Readers via Reset.
3. Thank you for the name suggestions! Updated and added godoc.

UPDATE: Removed Reader.Reset and updated test helper function to remove duplicate events. I still want to investigate the tests better because I'm not really sure what they are testing. It's odd that tests are relying on behavior that is only implemented in test code (dropping duplicate events) across multiple session recordings.

UPDATE2: Reviewing failing integration tests.

UPDATE3: Integration test passed on re-run. I don't think I introduced any flake here, but I'm going to re-run it a few more times to be more confident.

UPDATE4: Integration tests continue to pass without any more failures. Looks like first failure might have been due to node not coming up.

[dustinspecker]

For increased confidence, I created a new pull request right off of master to demonstrate this test change + removing Reset still passes: #50875

[dustinspecker]

Hey @fspmarshall, Zac mentioned you would be a good person to take a look at these changes while he's on vacation. I'm happy to answer any questions. Ultimately, I'm wanting to use the session recording reader in another repository. Thank you in advance!

[rosstimothy]

I'm surprised that the linter isn't catching this(might need to file an issue upstream), but keys should be snake_case

Suggested change

	slog.Int64("skipped-bytes", p.SkippedBytes),
	slog.Int64("skipped-events", p.SkippedEvents),
	slog.Int64("out-of-order-events", p.OutOfOrderEvents),
	slog.Int64("total-events", p.TotalEvents),
	slog.Int64("skipped_bytes", p.SkippedBytes),
	slog.Int64("skipped_events", p.SkippedEvents),
	slog.Int64("out_of_order_events", p.OutOfOrderEvents),
	slog.Int64("total_events", p.TotalEvents),

[dustinspecker]

Hmm. That is interesting about the linter. I'll look around to see if missing some configuration/sloglint version. Otherwise, I'll file an issue upstream.

Updated to use underscores instead of hyphens.

[dustinspecker]

Looks similar to this issue: go-simpler/sloglint#37 (go-simpler/sloglint#35)

[rosstimothy]

Is is possible that gzipReader is assigned after the check on line 129? For instance what if Read is being executed in another goroutine?

[dustinspecker]

I think you're right that is possible. I'm going to move this conversation to the other pull request that is making implementation changes: #50883

[fspmarshall]

Can we separate out this PR into two separate PRs? One that renames/moves but changes no meaningful code, and another for the changes?

This logic is extremely performance sensitive and extremely difficult to debug when an issue comes up. We've put a pretty significant amount of dev hours into tuning and debugging of logic related to event decoding & streaming. We really don't want to accidentally miss a potential issue because it was hidden by the diff of the move, and if a problem does sneak in it will be important to be able to isolate it in the commit log.

[dustinspecker]

Can we separate out this PR into two separate PRs? One that renames/moves but changes no meaningful code, and another for the changes?

This logic is extremely performance sensitive and extremely difficult to debug when an issue comes up. We've put a pretty significant amount of dev hours into tuning and debugging of logic related to event decoding & streaming. We really don't want to accidentally miss a potential issue because it was hidden by the diff of the move, and if a problem does sneak in it will be important to be able to isolate it in the commit log.

Yep, I'll split this out.

EDIT: going to add a commit at the start of this branch that is a pure copy. And then the follow up commits will show removals.

[dustinspecker]

Can we separate out this PR into two separate PRs? One that renames/moves but changes no meaningful code, and another for the changes?

This logic is extremely performance sensitive and extremely difficult to debug when an issue comes up. We've put a pretty significant amount of dev hours into tuning and debugging of logic related to event decoding & streaming. We really don't want to accidentally miss a potential issue because it was hidden by the diff of the move, and if a problem does sneak in it will be important to be able to isolate it in the commit log.

I've split this into two parts now. This pull request is focused only on moving Reader and renaming. To make it easier to review, the first commit is a simply/copy paste of relevant files without any changes (it does not compile, etc.) The next commit removes unneeded functions such as the writer, etc. And follow up commits have other changes related to renaming.

#50883 has changes to implementation details and removal of APIs.

Please let me know if you have any suggestions to make this is easier to review. I know there's a lot here, so happy to spend effort making this easier on reviewers. I'm hoping the addition of the first commit with simply copy/paste makes it easier to see what was removed during the initial move.

[rosstimothy]

Thanks for all your work on this @dustinspecker! If you plan on backporting this and #50883 I'd suggest combining both commits into a single PR on the release branches so they land at the same time.