Collect telemetry for upload path #34
Open
Conversation
This file contains hidden or 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
port8080
force-pushed
the
ajit/add-xet-telemetry
branch
from
0f05b62 to
b08974a
Compare
Get the telemetry data to the py layer. This will be sent over the telemetry endpoint later. Related to STO-7
port8080
force-pushed
the
ajit/add-xet-telemetry
branch
from
b08974a to
bd328fd
Compare
assafvayner
reviewed
Oct 8, 2024
| @@ -42,7 +42,7 @@ pub trait UploadClient { | |||
| #[async_trait] | |||
| pub trait ReconstructionClient { | |||
| /// Get a entire file by file hash. | |||
| async fn get_file(&self, hash: &MerkleHash, writer: &mut Box<dyn Write + Send>) -> Result<()>; | |||
| async fn get_file(&self, hash: &MerkleHash, writer: &mut Box<dyn Write + Send>) -> Result<u64>; | |||
There was a problem hiding this comment.
add a comment for what this return value is, presumably amount written to writer
| @@ -215,7 +215,8 @@ impl RemoteClient { | |||
| .map_err(|e| CasClientError::InternalError(anyhow!("join error {e}")))??; | |||
| writer.write_all(&piece)?; | |||
| } | |||
| Ok(total_len as usize) | |||
| // Todo: return the bytes which were read from the cache for telemetry | |||
| Ok(total_len as u64) | |||
There was a problem hiding this comment.
this total len is the uncompressed length of the section, not the number of bytes over the network which is what I think you're attempting to track that number should be the difference in the url_range end - start for each piece.
seanses
reviewed
Oct 9, 2024
| @@ -283,29 +288,37 @@ pub(crate) async fn register_new_cas_block( | |||
| cas_data.chunks.clear(); | |||
| cas_data.pending_file_info.clear(); | |||
|
|
|||
| Ok(cas_hash) | |||
| Ok((cas_hash, compressed_bytes_len as u64)) | |||
There was a problem hiding this comment.
nit: using compressed_bytes_len from client side is unreliable, people can inject any number here and that does not impact clean process correctness.
seanses
reviewed
Oct 9, 2024
| @@ -577,13 +585,14 @@ impl Cleaner { | |||
| if cas_data_accumulator.data.len() >= TARGET_CAS_BLOCK_SIZE { | |||
| let mut new_cas_data = take(cas_data_accumulator.deref_mut()); | |||
| drop(cas_data_accumulator); // Release the lock. | |||
| register_new_cas_block( | |||
| let (_cas_hash, compressed_bytes) = register_new_cas_block( | |||
There was a problem hiding this comment.
This compressed_bytes
- doesn't include all bytes produced by this file after dedup: they are also produced in
fn dedup(...). I noticed you modified that function but the return value is not used? - data in this
cas_data_accumulatormay also include bytes produced by other files cleaned before this file using the same PointerFileTranslator.
seanses
requested changes
Oct 9, 2024
Merged
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.
Get the telemetry data to the py layer. This will be sent over the telemetry endpoint later.
Related to STO-7