Skip to content

Sanitize Rust Debug Logs#15743

Merged
ibeckermayer merged 4 commits intomasterfrom
isaiah/sanitize-debug-logs
Aug 24, 2022
Merged

Sanitize Rust Debug Logs#15743
ibeckermayer merged 4 commits intomasterfrom
isaiah/sanitize-debug-logs

Conversation

@ibeckermayer
Copy link
Copy Markdown
Contributor

Adds the derivative crate and the vec_u8_debug function, which together allow us to avoid printing gigantic vectors in the debug logs.

together allow us to avoid printing gigantic vectors in the debug
logs.
directory_id: u32,
offset: u64,
path: UnixPath,
#[derivative(Debug(format_with = "util::vec_u8_debug"))]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Food for thought since it might not make a meaningful difference, but I'd argue a manual Debug impl isn't unreasonable here and would avoid adding a new proc macro dependency. Derive macros using syn can bloat build times quite a bit, though admittedly we have 3 other dependencies that use it so the marginal cost might not be very high.

rust-analyzer even autogenerates a default impl, so this would become:

impl fmt::Debug for SharedDirectoryWriteRequest {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.debug_struct("SharedDirectoryWriteRequest")
            .field("completion_id", &self.completion_id)
            .field("directory_id", &self.directory_id)
            .field("offset", &self.offset)
            .field("path", &self.path)
            .field("write_data", &format!("&[u8] of length {}", self.write_data.len()))
            .finish()
    }
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timothyb89 thanks for the heads up.

Do you understand why "Derive macros using syn can bloat build times quite a bit"? Is that specific to "Derive macros" (which I'm assuming means those that use proc_macro_derive), or is it due to syn::parse being slow?

rust-analyzer even autogenerates a default impl

Can you point me to how you got that autogenerated implementation? I tried selecting rust-analyzer: Expand macro recursively in VSCode, however it just gave me this:

// Recursive expansion of Debug! macro
// ====================================

impl core::fmt::Debug for SharedDirectoryWriteRequest {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it, the slowness comes in two parts: the syn crate itself is pretty large and slow to compile, and proc macros' runtime generation can be CPU intensive and/or generate lots of additional code which is itself slow to compile. In our case the dependency is shared which helps. I've occasionally seen multiple syn versions getting built due to incompatible dependencies but that doesn't seem to be the case here per our Cargo.lock.

https://matklad.github.io/2021/09/04/fast-rust-builds.html has some good information, but in any case the real impact of adding derivative for us is probably not very high, I'm just generally apprehensive about adding new proc macros.


Anyway, the trick with rust-analyzer is to start with the empty impl:

impl std::fmt::Debug for Foo {}

Request autocomplete on the error line for "not all trait items implemented", select "Implement missing members", and it'll generate:

impl std::fmt::Debug for Foo {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.debug_struct("Foo").field("name", &self.name).field("age", &self.age).finish()
    }
}

I just added newlines in my earlier example since it just barfs everything onto one line. It works with core::fmt::Debug too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very helpful, thanks. I've changed it to a manual impl 6c99a37

@ibeckermayer ibeckermayer enabled auto-merge (squash) August 24, 2022 18:30
@ibeckermayer ibeckermayer merged commit 9f19745 into master Aug 24, 2022
@github-actions
Copy link
Copy Markdown
Contributor

@ibeckermayer See the table below for backport results.

Branch Result
branch/v10 Failed
branch/v9 Failed

ibeckermayer pushed a commit that referenced this pull request Aug 26, 2022
ibeckermayer pushed a commit that referenced this pull request Aug 26, 2022
ibeckermayer pushed a commit that referenced this pull request Sep 7, 2022
…ring) (#15770)

* Windows Desktop Directory Sharing (#13630)

* `IRP_MJ_CREATE` (#12665)

* `IRP_MJ_QUERY_INFORMATION` (#12717)

* `IRP_MJ_CLOSE` (#12729)

* Refactor rdpdr client (#12750)

* Adding logic for `FILE_SUPERSEDE` (#12829)

* Improve `process_irp_create` (#12830)

* adds return statements that got lost in a merge

* `IRP_MJ_DIRECTORY_CONTROL` (#12870)

* `FileFullDirectoryInformation` (#12908)

* Improve `ClientDriveQueryDirectoryResponse.encode()` (#12912)

* `IRP_MJ_QUERY_VOLUME_INFORMATION` (#13071)

* Fix Shared Directory Request handling when feature is disabled (#13439)

* IRP_MJ_READ, IRP_MJ_WRITE, and IRP_MJ_SET_INFORMATION (#13995)

* Adds constants for sizing calculations (#14051)

Co-authored-by: Łukasz Kozłowski <lukasz.kozlowski@goteleport.com>
Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>

* `UnixPath` and `WindowsPath` (#14267)

* `SharedDirectoryMoveRequest` and `SharedDirectoryMoveResponse` (#14959)

* `SharedDirectoryCreateResponse` update (#15289)

* Fix `process_irp_set_information` (#15364)

* Sanitize Rust Debug Logs (#15743)

* updates rdp-rs ref to include licensing changes

* Updates rdp-rs ref and fixes Cargo

Co-authored-by: Łukasz Kozłowski <lukasz.kozlowski@goteleport.com>
Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>
@zmb3 zmb3 deleted the isaiah/sanitize-debug-logs branch September 9, 2022 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants