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

feat: add toggleable JSON logs formatting #823

Merged
merged 2 commits into from
May 10, 2024
Merged

Conversation

marcospb19-cw
Copy link
Contributor

No description provided.

@marcospb19-cw marcospb19-cw requested a review from a team as a code owner May 10, 2024 18:57
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

3, because the PR involves changes across multiple Dockerfiles and a Rust source file, which requires understanding of both Rust and Docker configurations. The logic for toggling JSON logs based on an environment variable also needs careful review to ensure it functions as intended.

🧪 Relevant tests

No

⚡ Possible issues

Possible Bug: The use of env::var_os("JSON_LOGS").is_some() for toggling JSON logs might not be robust enough. It will be true for any non-empty value, which could lead to unintended behaviors if the environment variable is set incorrectly.

🔒 Security concerns

No

Code feedback:
relevant filesrc/infra/tracing.rs
suggestion      

Consider using a more explicit check for enabling JSON logs. For example, check for a specific value like "true" to avoid confusion and potential errors if the environment variable is set to an unintended value. [important]

relevant linelet json_logs = env::var_os("JSON_LOGS").is_some();

Copy link

PR Code Suggestions ✨

CategorySuggestions                                                                                                                                                       
Best practice
Improve error handling during logging subscriber initialization.

Consider using tracing_subscriber::fmt().try_init() instead of subscriber.init() to handle
potential errors during the initialization of the subscriber.

src/infra/tracing.rs [42-44]

-subscriber.json().init();
-subscriber.compact().init();
+subscriber.json().try_init().expect("Failed to initialize JSON logging");
+subscriber.compact().try_init().expect("Failed to initialize compact logging");
 
Explicitly define the environment filter for the tracing subscriber for better control.

Use tracing_subscriber::EnvFilter directly instead of EnvFilter::from_default_env() to
explicitly define the environment filter for better clarity and control.

src/infra/tracing.rs [38]

-.with_env_filter(EnvFilter::from_default_env());
+.with_env_filter("info,my_crate=debug");
 
Use consistent method for environment variable checks.

Instead of using env::var_os("JSON_LOGS").is_some(), use env::var("JSON_LOGS").is_ok() for
consistency with other environment variable checks in the code.

src/infra/tracing.rs [40]

-let json_logs = env::var_os("JSON_LOGS").is_some();
+let json_logs = env::var("JSON_LOGS").is_ok();
 
Maintainability
Refactor subscriber initialization into a separate function for better maintainability.

Refactor the conditional logic for initializing the subscriber into a separate function to
improve code readability and maintainability.

src/infra/tracing.rs [41-44]

-if json_logs {
-    subscriber.json().init();
-} else {
-    subscriber.compact().init();
+init_subscriber(subscriber, json_logs);
+
+fn init_subscriber(subscriber: Subscriber, json_logs: bool) {
+    if json_logs {
+        subscriber.json().init();
+    } else {
+        subscriber.compact().init();
+    }
 }
 

@marcospb19-cw marcospb19-cw force-pushed the feat-toggle-json-logs branch from 5f1d0a8 to fee3816 Compare May 10, 2024 19:01
@marcospb19-cw marcospb19-cw enabled auto-merge (squash) May 10, 2024 19:14
@marcospb19-cw marcospb19-cw merged commit cf8bcff into main May 10, 2024
25 checks passed
@marcospb19-cw marcospb19-cw deleted the feat-toggle-json-logs branch May 10, 2024 19:20
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