fix!(trace): simplify tracing and only write data in complete batches #1437
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
evan-forbes
added
WS: Big Blonks 🔭
evan-forbes
requested review from
cmwaters,
ninabarbakadze and
staheri14
and removed request for
a team
evan-forbes
commented
Jul 29, 2024
pkg/trace/cached_file.go
Outdated
Comment on lines
82
to
86
| _, err := f.flush(buffer) | ||
| if err != nil { | ||
| f.logger.Error("tracer failed to write buffered files to file", "error", err) | ||
| } | ||
| buffer = buffer[:0] // reset buffer |
There was a problem hiding this comment.
here we're continuing to treat the trace data as expendable and instead optimizing for never blocking. If there's an error writing to the file, then we simply log it and throw away the data.
If this is occurring, we either see it in the logs or the data itself.
evan-forbes
changed the title
cmwaters
previously approved these changes
Jul 31, 2024
rootulp
previously approved these changes
Aug 5, 2024
| #### Using environment variables for s3 bucket | ||
|
|
||
| Alternatively, you can set the following environment variables: | ||
| or using aws s3 (after setting up the aws cli ofc): |
There was a problem hiding this comment.
Suggested change
| or using aws s3 (after setting up the aws cli ofc): | |
| or using aws s3 (after [setting up the aws cli](https://docs.aws.amazon.com/cli/latest/userguide/getting-started-quickstart.html)): |
|
converting to a draft as we're still seeing the issue where the last line doesn't get completely written, even when we sync the file after each flush 🤔 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR removes all ability to push trace data automatically. Instead, we can rely on standard tools such as the aws-cli to push to an s3 bucket.
It also changes the mechanism for buffering, instead of buffering raw bytes, it buffers events atomically. This results in only every writing entire events. This should prevent all (if not the vast majority) of instances where we were writing incomplete json data to the end of the file, leaving it unreadable.
Combined with the removal of the ability to read the json files to push to s3, we now avoid always ignoring data when we push. Instead we just push the data after the experiment is ran. Note that we still ignore data if the buffers are full (should never happen assuming we don't log so much as to overwhelm the ssd and keep the buffers sufficiently high).
Lastly, this PR takes advantage of each file having its own buffer to also serialize and write data in parallel, which should further increase performance. We also replaced the mutex with a channel.
closes #1403
Obligatory Versioning Exclusion Note
While this PR is breaking in that it removes a lot of functionality, it doesn't break the config or comet RPC. This follows the practice that we've been doing with the
tracepackage, which is treating it like a dependency. Meaning, this is like if we updated an external package from one major change to another, therefore we don't need to bump core's major release.