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: increase tracing queue size #1080

Merged
merged 1 commit into from
Jun 12, 2024
Merged

feat: increase tracing queue size #1080

merged 1 commit into from
Jun 12, 2024

Conversation

dinhani-cw
Copy link
Contributor

No description provided.

@dinhani-cw dinhani-cw requested a review from a team as a code owner June 12, 2024 22:45
@dinhani-cw dinhani-cw enabled auto-merge (squash) June 12, 2024 22:45
Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]

2

🧪 Relevant tests

No

🔒 Security concerns

No

⚡ Key issues to review

Possible Performance Issue:
Increasing the tracing queue size to u16::MAX might lead to higher memory consumption which could impact system performance under high load. Consider testing or providing configuration options to adjust this value based on the deployment scenario.

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Maintainability
Refactor the tracer configuration and pipeline setup into separate functions for better code organization

To improve the code structure and readability, consider separating the tracer
configuration and pipeline setup into distinct functions or sections. This separation can
enhance maintainability and make the code easier to navigate and modify.

src/infra/tracing.rs [179-189]

-let tracer_config = trace::config().with_resource(SdkResource::new(vec![KeyValue::new("service.name", service_name)]));
-let batch_config = BatchConfigBuilder::default().with_max_queue_size(u16::MAX as usize).build();
-opentelemetry_otlp::new_pipeline()
-    .tracing()
-    .with_exporter(tracer_exporter)
-    .with_trace_config(tracer_config)
-    .with_batch_config(batch_config)
-    .install_batch(runtime::Tokio)
-    .unwrap()
+let tracer_config = configure_tracer(service_name);
+let batch_config = configure_batch();
+setup_pipeline(tracer_config, batch_config, tracer_exporter);
 
Suggestion importance[1-10]: 9

Why: Separating the tracer configuration and pipeline setup into distinct functions significantly improves code readability and maintainability. This refactor makes the code easier to navigate and modify, which is a substantial improvement.

9
Add a comment to explain the choice of u16::MAX as usize for the maximum queue size

Since u16::MAX as usize is used to set the maximum queue size, it's a good practice to add
a comment explaining why this specific value is chosen. This helps maintain the code's
readability and understandability, especially for values that might seem arbitrary without
context.

src/infra/tracing.rs [182]

+// Set the maximum queue size to the maximum value of u16 to ensure large capacity
 let batch_config = BatchConfigBuilder::default().with_max_queue_size(u16::MAX as usize).build();
 
Suggestion importance[1-10]: 7

Why: Adding a comment improves code readability and helps future maintainers understand the rationale behind using u16::MAX as usize. This is a minor but useful improvement for maintainability.

7
Error handling
Replace unwrap() with expect() to provide a clearer error message in case of failure

Consider handling the potential error from the install_batch method instead of unwrapping
directly. This will prevent the application from panicking in case of an error during the
installation of the batch pipeline. You can use a match statement to handle different
outcomes gracefully.

src/infra/tracing.rs [189]

 .install_batch(runtime::Tokio)
-.unwrap()
+.expect("Failed to install batch pipeline")
 
Suggestion importance[1-10]: 8

Why: Using expect() instead of unwrap() provides a clearer error message, which improves error handling and debugging. However, it would be even better to handle the error more gracefully rather than just providing a message.

8
Performance
Adjust the batch configuration to better align with system performance and resource constraints

It's recommended to verify if the BatchConfigBuilder default settings align with the
system's performance and resource constraints. If the default batch interval or other
parameters are not suitable, consider configuring them explicitly to optimize tracing
performance.

src/infra/tracing.rs [182]

-let batch_config = BatchConfigBuilder::default().with_max_queue_size(u16::MAX as usize).build();
+let batch_config = BatchConfigBuilder::default()
+    .with_max_queue_size(u16::MAX as usize)
+    .with_scheduled_delay_millis(500) // Adjust based on system performance
+    .build();
 
Suggestion importance[1-10]: 6

Why: While adjusting the batch configuration can optimize performance, the specific values for parameters like with_scheduled_delay_millis should be determined based on actual performance testing and requirements. The suggestion is good but may need further refinement.

6

@dinhani-cw dinhani-cw merged commit 90db7b6 into main Jun 12, 2024
25 checks passed
@dinhani-cw dinhani-cw deleted the tracing-queue-size branch June 12, 2024 22:50
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.

1 participant