[improve] Add jdbc common collect e2e code (#3273) #2
Conversation
Signed-off-by: Cyanty <[email protected]>
Signed-off-by: Cyanty <[email protected]>
Signed-off-by: Cyanty <[email protected]>
Signed-off-by: Cyanty <[email protected]>
Signed-off-by: Cyanty <[email protected]>
|
please fix ci |
WalkthroughThe changes introduce enhanced support for database testing using Testcontainers within the Hertzbeat collector modules. A new enum for database image management is added, and a new end-to-end test class is implemented to verify JDBC metric collection against a PostgreSQL Testcontainer. The JDBC URL construction logic is extended to handle "testcontainers" as a platform. Maven configuration files are updated to manage Testcontainers dependencies via a BOM and to include specific JDBC drivers and Testcontainers modules for MySQL and PostgreSQL, ensuring consistent versions and improved test isolation. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant JdbcCommonCollectE2eTest
participant Testcontainer(PostgreSQL)
participant JdbcCommonCollect
TestRunner->>JdbcCommonCollectE2eTest: setUp()
JdbcCommonCollectE2eTest->>Testcontainer(PostgreSQL): Start container
JdbcCommonCollectE2eTest->>JdbcCommonCollect: Initialize with test config
loop For each metric
JdbcCommonCollectE2eTest->>JdbcCommonCollect: collectMetrics(metric)
JdbcCommonCollect->>Testcontainer(PostgreSQL): Connect via JDBC (testcontainers URL)
JdbcCommonCollect-->>JdbcCommonCollectE2eTest: Return collected data
JdbcCommonCollectE2eTest->>JdbcCommonCollectE2eTest: Validate results
end
JdbcCommonCollectE2eTest-->>TestRunner: Test results
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
hertzbeat-e2e/hertzbeat-collector-basic-e2e/src/test/java/org/apache/hertzbeat/collector/collect/basic/database/DatabaseImagesEnum.java (2)
56-63: Consider supporting custom tags in lookup.The current implementation only matches exact "imageName:defaultTag" combinations. Consider enhancing the method to parse custom tags.
public static DatabaseImagesEnum fromFullImageName(String fullImageName) { + // Split the image name and tag + String[] parts = fullImageName.split(":"); + if (parts.length > 0) { + // Match by image name only, ignoring tag differences + return fromImageName(parts[0]); + } for (DatabaseImagesEnum value : values()) { if (value.getFullImageName().equalsIgnoreCase(fullImageName)) { return value; } } throw new IllegalArgumentException("Unknown full database image name: " + fullImageName); }
65-71: Fix class name in toString method.The toString method references an incorrect class name "DatabaseImageNameEnum" which doesn't match the actual class name "DatabaseImagesEnum".
@Override public String toString() { - return "DatabaseImageNameEnum{" + + return "DatabaseImagesEnum{" + "imageName='" + imageName + '\'' + ", defaultTag='" + defaultTag + '\'' + '}'; }hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/database/JdbcCommonCollect.java (2)
386-387: Good implementation of Testcontainers JDBC URL support.The addition of the "testcontainers" case to the URL construction switch statement enables integration with Testcontainers for end-to-end testing. However, hard-coding credentials limits flexibility.
Consider parameterizing the credentials rather than hard-coding them:
-case "testcontainers" -> "jdbc:tc:" + host + ":" + port - + ":///" + (jdbcProtocol.getDatabase() == null ? "" : jdbcProtocol.getDatabase()) + "?user=root&password=root"; +case "testcontainers" -> "jdbc:tc:" + host + ":" + port + + ":///" + (jdbcProtocol.getDatabase() == null ? "" : jdbcProtocol.getDatabase()) + + "?user=" + (StringUtils.hasText(jdbcProtocol.getUsername()) ? jdbcProtocol.getUsername() : "root") + + "&password=" + (StringUtils.hasText(jdbcProtocol.getPassword()) ? jdbcProtocol.getPassword() : "root");
386-387: Add explanatory comment for the Testcontainers case.Adding a comment explaining the purpose and usage of the Testcontainers case would improve maintainability.
+// Special case for test containers JDBC URL format (jdbc:tc:postgresql:15:///test?user=root&password=root) case "testcontainers" -> "jdbc:tc:" + host + ":" + port + ":///" + (jdbcProtocol.getDatabase() == null ? "" : jdbcProtocol.getDatabase()) + "?user=root&password=root";hertzbeat-e2e/hertzbeat-collector-basic-e2e/src/test/java/org/apache/hertzbeat/collector/collect/basic/database/JdbcCommonCollectE2eTest.java (3)
48-53: Consider adding a comment explaining the database image selectionThe code references a
DatabaseImagesEnumclass which appears to be used for selecting test container images. I recommend adding a brief comment explaining the purpose of this enum and why PostgreSQL was chosen as the default test database.
86-94: Document why 'slow_sql' metric requires special handlingThe code has special handling for the "slow_sql" metric, only verifying that it doesn't throw an exception. Please add a comment explaining why this metric needs different validation logic than other metrics.
if ("slow_sql".equals(metricName)) { + // The slow_sql metric requires special handling because it depends on database-specific features + // that may not be available in the test container or requires additional setup. + // For E2E testing purposes, we only verify it doesn't throw exceptions. Metrics finalMetricsDef = metricsDef; assertDoesNotThrow(() -> collectMetrics(finalMetricsDef), String.format("%s failed to collect metrics)", metricName)); log.info("{} metrics validation passed", metricName); continue; // skip slow_sql, extra extensions }
54-69: Consider adding more specific assertions for collection resultsThe test effectively sets up the collection environment, but could benefit from more specific assertions to validate the collected data, not just that collection occurred without errors.
For example, you could enhance the test by validating specific fields or values in the collected metrics:
// Example of more specific validation CollectRep.MetricsData metricsData = validateMetricsCollection(metricsDef, metricName, true); // Verify specific metrics were collected if ("database_size".equals(metricName)) { assertFalse(metricsData.getValuesList().isEmpty(), "Should have collected database size metrics"); // Further specific assertions based on expected values }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/database/JdbcCommonCollect.java(1 hunks)hertzbeat-e2e/hertzbeat-collector-basic-e2e/pom.xml(2 hunks)hertzbeat-e2e/hertzbeat-collector-basic-e2e/src/test/java/org/apache/hertzbeat/collector/collect/basic/database/DatabaseImagesEnum.java(1 hunks)hertzbeat-e2e/hertzbeat-collector-basic-e2e/src/test/java/org/apache/hertzbeat/collector/collect/basic/database/JdbcCommonCollectE2eTest.java(1 hunks)hertzbeat-e2e/pom.xml(1 hunks)
🔇 Additional comments (8)
hertzbeat-e2e/pom.xml (1)
77-88: Well-structured dependency management addition.Adding the Testcontainers BOM (Bill of Materials) as part of dependencyManagement is a great practice. This centralizes version management for all Testcontainers dependencies across the project, ensuring consistency and simplifying future version upgrades.
hertzbeat-e2e/hertzbeat-collector-basic-e2e/pom.xml (3)
34-35: Good versioning practice.Defining database driver versions as properties follows Maven best practices, enabling centralized version management.
59-81: Well-organized Testcontainers dependencies.The dependencies are properly organized with clear comments, correct scoping as "test", and appropriate module selection for database testing with Testcontainers.
83-97: Appropriate JDBC driver configuration.The JDBC drivers are correctly configured with:
- Specific versions tied to properties
- Test scope to limit their inclusion to test classpath only
- Optional flag to prevent transitive inclusion in dependent projects
hertzbeat-e2e/hertzbeat-collector-basic-e2e/src/test/java/org/apache/hertzbeat/collector/collect/basic/database/DatabaseImagesEnum.java (2)
23-26: Clean enum design for test database types.The enum effectively defines the supported database types with appropriate image names and default tags for testing.
47-54: Well-implemented lookup method.The lookup method correctly handles case-insensitive matching of database image names, ensuring flexibility in how tests reference image types.
hertzbeat-e2e/hertzbeat-collector-basic-e2e/src/test/java/org/apache/hertzbeat/collector/collect/basic/database/JdbcCommonCollectE2eTest.java (2)
1-47: Good implementation of E2E tests for JDBC collectionThe class structure and test setup with Mockito look good. The class extends AbstractCollectE2eTest which is appropriate for E2E testing.
81-85: LGTM: Test method initialization looks goodThe test initialization correctly retrieves the job definition and sets up the collection environment.
| CollectRep.MetricsData metricsData = validateMetricsCollection(metricsDef, metricName, true); | ||
| CollectUtil.getConfigmapFromPreCollectData(metricsData); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
The return value of getConfigmapFromPreCollectData is not used
Line 96 calls CollectUtil.getConfigmapFromPreCollectData(metricsData) but doesn't use the return value. Consider either storing and using the return value or adding a comment explaining why it's called without using the result.
-CollectUtil.getConfigmapFromPreCollectData(metricsData);
+// Extract configmap data for potential use in subsequent test iterations
+Map<String, Configmap> configmap = CollectUtil.getConfigmapFromPreCollectData(metricsData);
+if (!configmap.isEmpty()) {
+ configmapFromPreCollectData.add(configmap);
+}Committable suggestion skipped: line range outside the PR's diff.
| @Override | ||
| protected Protocol buildProtocol(Metrics metricsDef) { | ||
| JdbcProtocol jdbcProtocol = metricsDef.getJdbc(); | ||
| jdbcProtocol.setHost(DATABASE_IMAGE_NAME); | ||
| jdbcProtocol.setPort(DATABASE_IMAGE_TAG); | ||
| jdbcProtocol.setDatabase("test"); | ||
| jdbcProtocol.setPlatform("testcontainers"); | ||
| return jdbcProtocol; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify if port assignment is correct
Line 75 assigns DATABASE_IMAGE_TAG to the port property, which seems unusual as ports are typically numeric values. Consider verifying if this is the intended behavior or if it should be using an actual port number.
🏁 Script executed:
#!/bin/bash
# Check how DATABASE_IMAGE_TAG is used and if it's appropriate for port configuration
echo "Checking DatabaseImagesEnum class for tag definition..."
fd -t f -e java "DatabaseImagesEnum.java" | xargs cat
echo "Checking JdbcProtocol class for port field type..."
fd -t f -e java "JdbcProtocol.java" | xargs cat | grep -A 10 "private.*port"Length of output: 2986
🏁 Script executed:
#!/bin/bash
# Display the relevant test file context around the buildProtocol method
fd -t f -e java "JdbcCommonCollectE2eTest.java" | xargs sed -n '1,200p'Length of output: 4427
🏁 Script executed:
#!/bin/bash
# Locate and display AbstractCollectE2eTest class implementation context around container setup or protocol handling
fd -t f -e java "AbstractCollectE2eTest.java" | xargs sed -n '1,200p'Length of output: 6135
Confirm port assignment in buildProtocol
The test currently does:
jdbcProtocol.setHost(DATABASE_IMAGE_NAME);
jdbcProtocol.setPort(DATABASE_IMAGE_TAG);Here, DATABASE_IMAGE_TAG comes from DatabaseImagesEnum.getDefaultTag() (e.g. "15"), which is the Docker image version—not a TCP port. Since JdbcProtocol.port is the port used to connect to the database, this will direct the driver to port 15 rather than the actual database port (e.g. 5432 or the Testcontainers–mapped port).
Please verify and update accordingly:
- If you’re using Testcontainers, start the container and set:
jdbcProtocol.setPort(String.valueOf(container.getFirstMappedPort()));
- Otherwise, use the default database port (e.g.
"5432") or inject it via configuration.
Location:
hertzbeat-e2e/hertzbeat-collector-basic-e2e/src/test/java/org/apache/hertzbeat/collector/collect/basic/database/JdbcCommonCollectE2eTest.java
protected Protocol buildProtocol(Metrics metricsDef) { … }
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
...est/java/org/apache/hertzbeat/collector/collect/basic/database/JdbcCommonCollectE2eTest.java
Show resolved
Hide resolved
...est/java/org/apache/hertzbeat/collector/collect/basic/database/JdbcCommonCollectE2eTest.java
Show resolved
Hide resolved
...est/java/org/apache/hertzbeat/collector/collect/basic/database/JdbcCommonCollectE2eTest.java
Show resolved
Hide resolved
Strong Foundation - Connection Check Enhancement for Better Reliability!
|
|
/refacto-test |
1 similar comment
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Solid Implementation - Let's Address Database Testing Resource Management!
|
| import java.util.Map; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; | ||
|
|
||
| /** | ||
| * E2e test monitored by Jdbc Common |
There was a problem hiding this comment.
Missing Container Cleanup in JdbcCommonCollectE2eTest
The test class uses TestContainers for database testing but doesn't properly manage container lifecycle. TestContainers typically requires explicit container initialization and cleanup. Without proper container management, tests may leave containers running after completion, causing resource leaks, potential port conflicts, and increased resource consumption during test execution. This violates resource management best practices and could lead to unstable test environments.
@Testcontainers
public class JdbcCommonCollectE2eTest extends AbstractCollectE2eTest {
private static final DatabaseImagesEnum databaseImage = DatabaseImagesEnum.fromImageName("postgresql");
private static final String DATABASE_IMAGE_NAME = databaseImage.getImageName();
private static final String DATABASE_IMAGE_TAG = databaseImage.getDefaultTag();
@Container
private static final PostgreSQLContainer<?> postgresContainer = new PostgreSQLContainer<>(databaseImage.getFullImageName())
.withDatabaseName("test")
.withUsername("root")
.withPassword("root");
References
Standard: ISO/IEC 25010 Reliability - Resource Utilization
| } | ||
|
|
||
| public static DatabaseImagesEnum fromImageName(String imageName) { | ||
| for (DatabaseImagesEnum value : values()) { | ||
| if (value.getImageName().equalsIgnoreCase(imageName)) { | ||
| return value; | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing Error Handling in TestContainers Database Type Resolution
The DatabaseImagesEnum.fromImageName method throws an IllegalArgumentException when an unknown database image name is provided, but there's no error handling in the JdbcCommonCollectE2eTest that uses this method with a hardcoded value. If the database type is changed or removed in the future, tests will fail with an uncaught exception rather than gracefully handling the error. This violates error handling best practices and could lead to difficult-to-diagnose test failures.
public static DatabaseImagesEnum fromImageName(String imageName) {
if (imageName == null || imageName.trim().isEmpty()) {
return POSTGRESQL; // Default to PostgreSQL if no name provided
}
for (DatabaseImagesEnum value : values()) {
if (value.getImageName().equalsIgnoreCase(imageName)) {
return value;
}
}
throw new IllegalArgumentException("Unknown database image name: " + imageName);
}References
Standard: Design by Contract - Precondition Validation
| case "dm" -> "jdbc:dm://" + host + ":" + port; | ||
| case "testcontainers" -> "jdbc:tc:" + host + ":" + port |
There was a problem hiding this comment.
Hardcoded Database Credentials in JDBC URL
The code adds a new case for testcontainers that includes hardcoded database credentials ('root'/'root') directly in the JDBC URL. While this is only used for testing purposes, hardcoded credentials in source code are generally considered a security risk as they could be inadvertently exposed or reused in non-test environments.
case "testcontainers" -> "jdbc:tc:" + host + ":" + port
+ "://" + (jdbcProtocol.getDatabase() == null ? "" : jdbcProtocol.getDatabase()) + "?user=" +
(jdbcProtocol.getUsername() == null ? "root" : jdbcProtocol.getUsername()) + "&password=" +
(jdbcProtocol.getPassword() == null ? "root" : jdbcProtocol.getPassword());References
Standard: CWE-798
Standard: OWASP Top 10 2021: A07 - Identification and Authentication Failures
| String.format("%s failed to collect metrics)", metricName)); | ||
| log.info("{} metrics validation passed", metricName); | ||
| continue; // skip slow_sql, extra extensions | ||
| } | ||
| CollectRep.MetricsData metricsData = validateMetricsCollection(metricsDef, metricName, true); | ||
| CollectUtil.getConfigmapFromPreCollectData(metricsData); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing Error Handling in Database Test Implementation
The test method handles the 'slow_sql' metric case differently but doesn't provide sufficient error handling or logging for diagnostic purposes. While the code uses assertDoesNotThrow() to verify execution, it doesn't capture or validate the actual result of the metrics collection. This makes it harder to diagnose test failures and understand what specifically went wrong with slow_sql metric collection.
if ("slow_sql".equals(metricName)) {
Metrics finalMetricsDef = metricsDef;
try {
CollectRep.MetricsData metricsData = collectMetrics(finalMetricsDef).build();
assertNotNull(metricsData, String.format("%s metrics data should not be null", metricName));
log.info("{} metrics validation passed with {} rows", metricName,
metricsData.getValuesCount() > 0 ? metricsData.getValuesCount() : "no");
} catch (Exception e) {
log.error("Error collecting {} metrics: {}", metricName, e.getMessage());
fail(String.format("%s metrics collection failed: %s", metricName, e.getMessage()));
}
continue; // skip slow_sql, extra extensions
}References
Standard: JUnit Best Practices - Error Diagnostics, Clean Code - Error Handling
6 participants
What's changed?
Benefit from testcontainers' rich jdbc driver support (SPI implementation), add jdbc common collect e2e code (postgresql/mysql supported for now)
Checklist
Add or update API
Summary by CodeRabbit
New Features
Chores