feat(launcher): db connect options #1182
Conversation
|
Warning Rate limit exceeded@k1rill-fedoseev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 42 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces updates to the Blockscout service launcher library and related components. The primary changes involve updating the Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/blockscout-service-launcher/src/database.rs (1)
148-173: Add documentation for connection pool settings.While the struct fields have basic comments, consider adding more detailed documentation about:
- Recommended values for connection pool settings
- Impact of each timeout setting
- Trade-offs between different configurations
Example documentation for max_connections:
/// Maximum number of connections for a pool +/// +/// This setting controls the maximum size of the connection pool. +/// Choose a value based on: +/// - Your application's concurrency needs +/// - Database server's max_connections limit +/// - Available system resources +/// +/// Recommended range: 10-100 for most applications pub max_connections: Option<u32>,libs/blockscout-service-launcher/Cargo.toml (1)
29-29: Consider adding version constraints for serde_with.The current specification
version = "3"could potentially accept breaking changes in future releases. Consider using a more specific version constraint:-serde_with = {version = "3", optional = true } +serde_with = {version = "~3.0", optional = true }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
libs/blockscout-service-launcher/Cargo.toml(3 hunks)libs/blockscout-service-launcher/src/database.rs(6 hunks)libs/env-collector/Cargo.toml(1 hunks)libs/env-collector/src/lib.rs(3 hunks)service-template/{{project-name}}-server/src/server.rs(1 hunks)
🔇 Additional comments (8)
service-template/{{project-name}}-server/src/server.rs (1)
47-47: LGTM! Clean refactor of database initialization.The change simplifies the database initialization by passing the entire settings object, which aligns with the new function signature and reduces complexity.
libs/blockscout-service-launcher/src/database.rs (3)
3-3: LGTM! Required import for duration serialization.The
serde_withimport is necessary for serializing Duration fields in the new database connection options.
21-21: LGTM! Updated error message to include new database version.The compile error message now correctly includes the new
database-1_0feature option.
30-34: LGTM! Clean refactor of database initialization.The changes improve the function's interface by:
- Consolidating parameters into a single settings structure
- Making the code more maintainable
- Preserving existing functionality
Also applies to: 37-37
libs/env-collector/src/lib.rs (1)
654-713: LGTM! Comprehensive environment variables for database connection options.The changes:
- Add all necessary environment variables for the new connection options
- Use consistent naming and structure
- Include appropriate default values
libs/env-collector/Cargo.toml (1)
16-16: LGTM! Updated dependency to use new database version.The change correctly updates the blockscout-service-launcher dependency to use the new database-1_0 feature.
libs/blockscout-service-launcher/Cargo.toml (2)
3-3: LGTM: Version bump follows semantic versioning.The version increment from 0.15.0 to 0.16.0 appropriately reflects the addition of new database configuration features.
94-94: LGTM: Database feature dependencies updated correctly.The addition of serde_with to the database feature is consistent with the enhanced database configuration capabilities.
k1rill-fedoseev
force-pushed
the
kf/feat/db-connect-options
branch
from
8d302ff to
3980f5f
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libs/blockscout-service-launcher/src/database.rs (2)
Line range hint
38-77: Enhance error handling in database creation.The database creation error handling could be improved by using a custom error type instead of string matching.
Consider creating a custom error type:
#[derive(Debug, thiserror::Error)] pub enum DatabaseError { #[error("Database '{0}' already exists")] AlreadyExists(String), #[error(transparent)] Other(#[from] DbErr), }Then refactor the error handling:
- Err(e) => { - if e.to_string().contains("already exists") { - tracing::info!("database '{db_name}' already exists"); - } else { - return Err(anyhow::anyhow!(e)); - } - } + Err(e) => match DatabaseError::from(e) { + DatabaseError::AlreadyExists(_) => { + tracing::info!("database '{db_name}' already exists"); + } + DatabaseError::Other(e) => return Err(e.into()), + }
234-247: Consider simplifying LevelFilter serialization.The custom serialization could be simplified by using derive macros or the existing
DisplayandFromStrimplementations.Consider using serde's built-in string serialization:
#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] #[serde(rename_all = "lowercase")] pub enum LevelFilter { Off, Error, Warn, Info, Debug, Trace, }
🛑 Comments failed to post (1)
libs/blockscout-service-launcher/src/database.rs (1)
224-224:
⚠️ Potential issueRemove unnecessary type conversions.
The
.into()conversions are unnecessary as we're already usingtracing::log::LevelFilter.Apply this diff to fix the pipeline errors:
- options.sqlx_logging_level(self.sqlx_logging_level.into()); + options.sqlx_logging_level(self.sqlx_logging_level); #[cfg(feature = "database-1_0")] options.sqlx_slow_statements_logging_settings( - self.sqlx_slow_statements_logging_level.into(), + self.sqlx_slow_statements_logging_level, self.sqlx_slow_statements_logging_threshold, );Also applies to: 227-227
🧰 Tools
🪛 GitHub Actions: Test, lint (libs)
[error] 224-224: Useless conversion to the same type:
tracing::log::LevelFilter. Remove.into()fromself.sqlx_logging_level.into()
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
libs/blockscout-service-launcher/src/database.rs (3)
Line range hint
38-77: Consider improving error handling in database creation.The database creation error handling could be more robust. Currently, it only checks if the error message contains "already exists", which might be fragile across different PostgreSQL versions or locales.
Consider using the PostgreSQL error codes instead:
- if e.to_string().contains("already exists") { + if let DbErr::Custom(ref e) = e { + // 42P04 is the PostgreSQL error code for "duplicate_database" + if e.contains("42P04") { tracing::info!("database '{db_name}' already exists"); } else { return Err(anyhow::anyhow!(e)); } + } else { + return Err(anyhow::anyhow!(e)); + }
149-182: Add validation for timeout settings.The struct accepts any Duration values for timeouts without validation. This could lead to operational issues if unreasonable values are provided (e.g., extremely short timeouts or very long lifetimes).
Consider adding validation during deserialization:
impl DatabaseConnectOptionsSettings { fn validate_timeout(name: &str, value: Duration) -> Result<(), String> { const MIN_TIMEOUT: Duration = Duration::from_millis(100); const MAX_TIMEOUT: Duration = Duration::from_secs(3600); if value < MIN_TIMEOUT || value > MAX_TIMEOUT { return Err(format!( "{} must be between {:?} and {:?}", name, MIN_TIMEOUT, MAX_TIMEOUT )); } Ok(()) } }
203-232: Consider adding connection pool metrics.The connection pool configuration is comprehensive, but there's no built-in way to monitor pool health (active connections, wait times, etc.).
Consider adding metrics collection capabilities:
- Add pool statistics collection
- Expose metrics for monitoring systems (e.g., Prometheus)
- Add health check endpoints
This would help with operational monitoring and troubleshooting. Would you like me to provide a detailed implementation proposal?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/blockscout-service-launcher/src/database.rs(6 hunks)
🔇 Additional comments (1)
libs/blockscout-service-launcher/src/database.rs (1)
193-194: Review default SQLx logging configuration.The default configuration enables SQLx logging (
sqlx_logging: true) atDebuglevel. This might be too verbose for production environments and could impact performance.Consider if these defaults are appropriate for your use case. You may want to:
- Disable logging by default
- Set a less verbose default level like
InfoRun this script to check current logging usage across the codebase:
k1rill-fedoseev
force-pushed
the
kf/feat/db-connect-options
branch
from
3980f5f to
081a63a
Compare
Summary by CodeRabbit
Release Notes
New Features
Chores
blockscout-service-launcher.Refactor