Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

poc: DebugAsJson #800

Merged
merged 2 commits into from
May 8, 2024
Merged

poc: DebugAsJson #800

merged 2 commits into from
May 8, 2024

Conversation

dinhani-cw
Copy link
Contributor

The idea is that when a value is traced using debug formatting, it should be printed as JSON instead of Rust format because this integrates better with monitoring tooling.

Copy link

github-actions bot commented May 8, 2024

PR Review 🔍

(Review updated until commit 99f062c)

⏱️ Estimated effort to review [1-5]

4, because the PR involves multiple changes across various configuration structs and enums, integrating a new trait for JSON debug formatting. The changes are spread across many files and affect core configuration components, which requires careful review to ensure compatibility and correctness.

🧪 Relevant tests

No

⚡ Possible issues

Possible Bug: The removal of the standard Debug trait in favor of DebugAsJson might lead to issues where the standard debug output is expected or required for certain operations or logs.

Compatibility Issue: The addition of serde::Serialize to all structs may not be necessary if they are not intended to be serialized, potentially leading to increased compile times and unnecessary code complexity.

🔒 Security concerns

No

Code feedback:
relevant filesrc/config.rs
suggestion      

Consider implementing a fallback or dual trait strategy where both Debug and DebugAsJson are available. This can prevent potential issues in parts of the application that might rely on the standard Debug implementation. [important]

relevant line#[derive(DebugAsJson, Clone, Parser, serde::Serialize)]

relevant filesrc/config.rs
suggestion      

Ensure that all configuration structs that now include serde::Serialize are actually intended to be serialized. If some structs do not need serialization, removing the trait could simplify the code and reduce compilation overhead. [important]

relevant line#[derive(DebugAsJson, Clone, Parser, serde::Serialize)]

relevant filesrc/eth/evm/evm.rs
suggestion      

Verify that the DebugAsJson output is compatible with all logging and monitoring tools currently in use. If not, consider adding configuration options to toggle between Debug and DebugAsJson outputs based on the environment or deployment configuration. [medium]

relevant line#[derive(DebugAsJson, Clone, serde::Serialize)]

relevant filesrc/eth/primitives/account.rs
suggestion      

Review and test the changes in the context of how accounts are handled, especially in error scenarios where detailed debug information might be necessary. Ensure that the new JSON format provides all required information. [medium]

relevant line#[derive(DebugAsJson, Clone, Default, PartialEq, Eq, fake::Dummy, serde::Deserialize, serde::Serialize)]

Copy link

github-actions bot commented May 8, 2024

PR Code Suggestions ✨

CategorySuggestions                                                                                                                                                       
Enhancement
Add the Debug trait to all configurations for enhanced debugging flexibility.

Consider implementing the Debug trait alongside DebugAsJson for all configurations. This
allows for more flexible debugging options, especially in environments where JSON output
may not be necessary or optimal.

src/config.rs [71]

-#[derive(DebugAsJson, Clone, Parser, serde::Serialize)]
+#[derive(Debug, DebugAsJson, Clone, Parser, serde::Serialize)]
 
Maintainability
Review the necessity and impact of the DebugAsJson trait in the project.

Ensure that the DebugAsJson trait is compatible and necessary for all the enums and
structs where it's applied, as it might introduce overhead or complexity if not properly
utilized.

src/config.rs [570]

-#[derive(DebugAsJson, Clone, serde::Serialize)]
+#[derive(Debug, Clone, serde::Serialize)]
 
Best practice
Introduce conditional compilation to toggle between Debug and DebugAsJson.

If DebugAsJson is intended to replace standard Debug for JSON formatted logs, consider
adding conditional compilation flags to toggle between Debug and DebugAsJson based on the
deployment or development needs.

src/config.rs [71]

-#[derive(DebugAsJson, Clone, Parser, serde::Serialize)]
+#[cfg_attr(feature = "json_debug", derive(DebugAsJson))]
+#[cfg_attr(not(feature = "json_debug"), derive(Debug))]
+#[derive(Clone, Parser, serde::Serialize)]
 
Implement tests to validate the JSON output of configurations using DebugAsJson.

Since DebugAsJson is used extensively, create comprehensive tests to ensure that the JSON
output is correct and adheres to expected formats, especially for complex nested
structures.

src/config.rs [71]

 #[derive(DebugAsJson, Clone, Parser, serde::Serialize)]
+// Example test for JSON output
+#[cfg(test)]
+mod tests {
+    use super::*;
+    #[test]
+    fn test_json_output() {
+        let config = CommonConfig { /* fields */ };
+        let json_output = format!("{:?}", config);
+        assert!(serde_json::from_str::<serde_json::Value>(&json_output).is_ok());
+    }
+}
 

@dinhani-cw dinhani-cw marked this pull request as ready for review May 8, 2024 12:27
@dinhani-cw dinhani-cw requested a review from a team as a code owner May 8, 2024 12:27
Copy link

github-actions bot commented May 8, 2024

Persistent review updated to latest commit 99f062c

Copy link

github-actions bot commented May 8, 2024

PR Code Suggestions ✨

CategorySuggestions                                                                                                                                                       
Best practice
Conditionally compile the DebugAsJson derive to include it only in debug builds.

Consider using a conditional compilation flag for the DebugAsJson derive to ensure it is
only included in debug builds. This can help in avoiding the inclusion of debug features
in a release build, which might not be necessary and could lead to potential security
risks or performance issues.

src/config.rs [71]

-#[derive(DebugAsJson, Clone, Parser, serde::Serialize)]
+#[cfg_attr(debug_assertions, derive(DebugAsJson))]
+#[derive(Clone, Parser, serde::Serialize)]
 
Security
Review the use of DebugAsJson to prevent unintended exposure of sensitive information.

Ensure that all configurations using DebugAsJson are intended for debugging purposes and
review whether sensitive information might be exposed through logging or debugging
outputs.

src/config.rs [71]

-#[derive(DebugAsJson, Clone, Parser, serde::Serialize)]
+#[derive(Clone, Parser, serde::Serialize)]
 
Enhancement
Implement a toggle for DebugAsJson based on runtime configuration.

If DebugAsJson is intended for improved logging, consider implementing it in a way that it
can be toggled on or off based on environment variables or configuration settings, rather
than being compiled into the binary.

src/config.rs [71]

-#[derive(DebugAsJson, Clone, Parser, serde::Serialize)]
+#[cfg(feature = "debug_json")]
+#[derive(DebugAsJson)]
+#[derive(Clone, Parser, serde::Serialize)]
 
Possible issue
Verify compatibility of all fields in types derived with DebugAsJson for JSON serialization.

For enums and structs that are derived with DebugAsJson, ensure that all fields within
these types are also compatible with JSON serialization to avoid runtime errors.

src/config.rs [71]

 #[derive(DebugAsJson, Clone, serde::Serialize)]
+// Ensure all fields are serializable
 
Maintainability
Create a base trait or derive macro for common derives to reduce redundancy.

Since DebugAsJson is used extensively across multiple configurations, consider creating a
base trait or derive macro that includes DebugAsJson along with common derives like Clone
and serde::Serialize to simplify the codebase and reduce redundancy.

src/config.rs [71]

-#[derive(DebugAsJson, Clone, Parser, serde::Serialize)]
+#[derive(CommonConfigTraits)]
+// CommonConfigTraits could be a custom derive macro including DebugAsJson, Clone, Parser, and serde::Serialize
 

@dinhani-cw dinhani-cw enabled auto-merge (squash) May 8, 2024 12:30
@dinhani-cw dinhani-cw merged commit c8b7cd5 into main May 8, 2024
26 checks passed
@dinhani-cw dinhani-cw deleted the debug-json branch May 8, 2024 12:33
@arthurmm-cloudwalk
Copy link
Contributor

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants