-
Notifications
You must be signed in to change notification settings - Fork 288
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
Development
: Add more data for Telemetry
#9345
base: develop
Are you sure you want to change the base?
Conversation
WalkthroughThe changes introduce a new service class, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files ignored due to path filters (2)
src/test/resources/config/application-artemis.yml
is excluded by!**/*.yml
src/test/resources/config/application.yml
is excluded by!**/*.yml
Files selected for processing (4)
- src/main/java/de/tum/cit/aet/artemis/core/service/telemetry/EurekaClientService.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/service/telemetry/TelemetrySendingService.java (4 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/service/telemetry/TelemetryService.java (2 hunks)
- src/test/java/de/tum/cit/aet/artemis/telemetry/TelemetryServiceTest.java (1 hunks)
Additional context used
Path-based instructions (4)
src/main/java/de/tum/cit/aet/artemis/core/service/telemetry/EurekaClientService.java (1)
Pattern
src/main/java/**/*.java
: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/core/service/telemetry/TelemetrySendingService.java (1)
Pattern
src/main/java/**/*.java
: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/core/service/telemetry/TelemetryService.java (1)
Pattern
src/main/java/**/*.java
: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/test/java/de/tum/cit/aet/artemis/telemetry/TelemetryServiceTest.java (1)
Pattern
src/test/java/**/*.java
: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
Additional comments not posted (13)
src/main/java/de/tum/cit/aet/artemis/core/service/telemetry/TelemetryService.java (2)
1-1
: LGTM!Moving the
TelemetryService
class to a dedicatedtelemetry
sub-package aligns with the single responsibility principle and improves code organization.
15-15
: LGTM!The import statement for
ProfileService
follows the coding guideline to avoid wildcard imports.src/test/java/de/tum/cit/aet/artemis/telemetry/TelemetryServiceTest.java (1)
29-29
: LGTM!The change in import statement for
TelemetryService
reflects the restructuring of the package hierarchy. Moving the class to a dedicatedtelemetry
subpackage improves code organization and modularity. The change is consistent with the information provided in the list of alterations and does not introduce any functional or logical issues.src/main/java/de/tum/cit/aet/artemis/core/service/telemetry/EurekaClientService.java (2)
49-73
: LGTM!The method logic is correct, and the implementation is accurate. It follows the provided coding guidelines and best practices.
81-91
: LGTM!The method logic is correct, and the implementation is accurate. It follows the provided coding guidelines and best practices.
src/main/java/de/tum/cit/aet/artemis/core/service/telemetry/TelemetrySendingService.java (8)
1-1
: Package restructuring improves modularityChanging the package to
de.tum.cit.aet.artemis.core.service.telemetry
better organizes telemetry-related services and enhances code maintainability.
24-24
: ImportingProfileService
for profile managementThe import of
ProfileService
is appropriate for accessing profile-related information within the telemetry service.
33-34
: Expansion ofTelemetryData
record aligns with data requirementsAdding new fields to
TelemetryData
allows the collection of more comprehensive telemetry data, fulfilling the PR objectives and adhering to DTO guidelines of minimal necessary data.
41-44
: Dependencies added with appropriate access controlThe declaration of
EurekaClientService
andProfileService
asprivate final
complies with the least access principle and supports immutability.
45-49
: Constructor updated for proper dependency injectionIncluding
EurekaClientService
andProfileService
in the constructor aligns with constructor injection best practices, promoting better testability and adherence to the single responsibility principle.
73-75
: Injection ofdatasourceUrl
using@Value
annotationUsing
@Value("${spring.datasource.url}")
to injectdatasourceUrl
is appropriate and consistent with configuration management practices.
76-78
: Injection ofeurekaEnabled
flagThe use of
@Value("${eureka.client.enabled}")
to inject theeurekaEnabled
property correctly retrieves the configuration value.
79-81
: Injection ofbuildAgentCount
configurationInjecting
buildAgentCount
with@Value("${artemis.continuous-integration.concurrent-build-size}")
appropriately accesses the configuration setting.
src/main/java/de/tum/cit/aet/artemis/core/service/telemetry/TelemetrySendingService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/service/telemetry/TelemetrySendingService.java
Outdated
Show resolved
Hide resolved
// Wait one minute to give other nodes time to connect to the Eureka registry | ||
Thread.sleep(60000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using Thread.sleep
in asynchronous methods
Using Thread.sleep(60000)
in an asynchronous method can lead to thread blockage and resource inefficiency. It's better to use non-blocking scheduling techniques to delay execution.
Consider refactoring to use a scheduled task:
-// Wait one minute to give other nodes time to connect to the Eureka registry
-Thread.sleep(60000);
-numberOfInstances = eurekaClientService.getNumberOfReplicas();
+// Schedule the retrieval of replica count after a delay
+CompletableFuture.runAsync(() -> {
+ try {
+ Thread.sleep(60000);
+ long instances = eurekaClientService.getNumberOfReplicas();
+ // Update the numberOfInstances variable accordingly
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ log.error("Thread was interrupted", e);
+ }
+});
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files ignored due to path filters (1)
src/test/resources/config/application.yml
is excluded by!**/*.yml
Files selected for processing (2)
- src/main/java/de/tum/cit/aet/artemis/core/service/telemetry/TelemetrySendingService.java (4 hunks)
- src/test/java/de/tum/cit/aet/artemis/telemetry/TelemetryServiceTest.java (3 hunks)
Additional context used
Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/core/service/telemetry/TelemetrySendingService.java (1)
Pattern
src/main/java/**/*.java
: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/test/java/de/tum/cit/aet/artemis/telemetry/TelemetryServiceTest.java (1)
Pattern
src/test/java/**/*.java
: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
Additional comments not posted (5)
src/main/java/de/tum/cit/aet/artemis/core/service/telemetry/TelemetrySendingService.java (4)
1-1
: Package refactoring enhances modularityMoving
TelemetrySendingService
to thetelemetry
package improves code organization and aligns the class with its functionality.
33-34
: ExpandedTelemetryData
record captures detailed metricsAdding new fields to
TelemetryData
allows for more comprehensive telemetry reporting, fulfilling the PR objectives of collecting richer data.
98-103
: Refactor to eliminate duplication when creatingTelemetryData
Duplicating the creation of
TelemetryData
instances can be streamlined to enhance readability and maintainability.Consider refactoring as follows:
TelemetryData telemetryData; +String contactInfo = null; +String adminName = null; if (sendAdminDetails) { + contactInfo = contact; + adminName = operatorAdminName; } -telemetryData = new TelemetryData(version, serverUrl, operator, activeProfiles, profileService.isProductionActive(), dataSource, numberOfInstances, buildAgentCount, - contact, operatorAdminName); +telemetryData = new TelemetryData(version, serverUrl, operator, activeProfiles, profileService.isProductionActive(), dataSource, numberOfInstances, buildAgentCount, + contactInfo, adminName);Likely invalid or redundant comment.
91-91
: Consider enhancing data source detection logicThe current logic assumes only MySQL or defaults to PostgreSQL, which may misclassify other data sources. To improve robustness, handle additional data source types or add validation for unknown types.
Apply this diff to improve the detection logic:
-var dataSource = datasourceUrl.startsWith("jdbc:mysql") ? "mysql" : "postgresql"; +String dataSource; +if (datasourceUrl.startsWith("jdbc:mysql")) { + dataSource = "mysql"; +} else if (datasourceUrl.startsWith("jdbc:postgresql")) { + dataSource = "postgresql"; +} else { + dataSource = "unknown"; + log.warn("Unknown datasource URL: {}", datasourceUrl); +}Likely invalid or redundant comment.
src/test/java/de/tum/cit/aet/artemis/telemetry/TelemetryServiceTest.java (1)
92-92
: Review the necessity of increasing timeouts in testsThe timeout in the
await()
calls has been increased from 1 second to 2 seconds. Please verify if this increase is necessary. Longer timeouts can slow down the test suite. If possible, optimize the tests to complete within the original timeframe.Also applies to: 112-112
|
||
private String eurekaRequestUrl; | ||
|
||
private byte[] appliciationsBody; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix typo in variable name 'appliciationsBody'
There is a typo in the variable name appliciationsBody
; it should be applicationsBody
. This typo occurs in the declaration and all usages of the variable. Correcting this will prevent potential compilation errors and improve code clarity.
Apply this diff to correct the typo:
- private byte[] appliciationsBody;
+ private byte[] applicationsBody;
- appliciationsBody = mapper.writeValueAsBytes(Map.of(...));
+ applicationsBody = mapper.writeValueAsBytes(Map.of(...));
- .body(appliciationsBody));
+ .body(applicationsBody));
Also applies to: 73-74, 86-86, 107-107
catch (Exception ignored) { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle exceptions in the catch block to aid debugging
The empty catch block suppresses any exceptions that may occur during the construction of eurekaRequestUrl
. This can make debugging difficult if an error occurs. It's advisable to handle the exception appropriately or at least log it.
Consider logging the exception:
- catch (Exception ignored) {
- }
+ catch (Exception e) {
+ // Log the exception for debugging purposes
+ e.printStackTrace();
+ }
Alternatively, if the exception can be safely ignored, add a comment explaining why:
catch (Exception ignored) {
+ // Exception can be ignored because defaultZoneUrl is guaranteed to be valid in the test environment
}
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 9
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files ignored due to path filters (1)
src/test/resources/config/application.yml
is excluded by!**/*.yml
Files selected for processing (3)
- src/main/java/de/tum/cit/aet/artemis/core/service/telemetry/TelemetrySendingService.java (3 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/service/telemetry/TelemetryService.java (4 hunks)
- src/test/java/de/tum/cit/aet/artemis/telemetry/TelemetryServiceTest.java (3 hunks)
Additional context used
Path-based instructions (3)
src/main/java/de/tum/cit/aet/artemis/core/service/telemetry/TelemetrySendingService.java (1)
Pattern
src/main/java/**/*.java
: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/core/service/telemetry/TelemetryService.java (1)
Pattern
src/main/java/**/*.java
: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/test/java/de/tum/cit/aet/artemis/telemetry/TelemetryServiceTest.java (1)
Pattern
src/test/java/**/*.java
: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
Additional comments not posted (14)
src/main/java/de/tum/cit/aet/artemis/core/service/telemetry/TelemetryService.java (2)
57-57
: LGTM!The method call to
sendTelemetryByPostRequest
has been correctly updated with the new parameterseurekaEnabled
andsendAdminDetails
.
1-1
: Verify all references to the updated package.The package declaration has changed to
de.tum.cit.aet.artemis.core.service.telemetry
. Ensure that all references toTelemetryService
throughout the codebase have been updated to reflect this change to preventClassNotFoundException
or other runtime issues.Run the following script to verify the package change:
src/main/java/de/tum/cit/aet/artemis/core/service/telemetry/TelemetrySendingService.java (6)
1-1
: Package restructuring improves code organizationUpdating the package to
de.tum.cit.aet.artemis.core.service.telemetry
enhances modularity by grouping telemetry-related classes together.
24-24
: Necessary import ofProfileService
Importing
ProfileService
is appropriate, as it's used to check if the production profile is active.
41-49
: Good use of constructor injection for dependenciesInjecting
EurekaClientService
andProfileService
via the constructor adheres to the dependency injection principle, enhancing testability and modularity.
73-74
: Confirm the accuracy ofbuildAgentCount
valueEnsure that the
buildAgentCount
retrieved from${artemis.continuous-integration.concurrent-build-size}
accurately reflects the number of build agents in the system, especially if the configuration can change at runtime.
90-90
: Consider enhancing data source detection logicThe current logic assumes only MySQL or defaults to PostgreSQL, which might misidentify other data sources. Please consider handling additional data source types or adding validation to prevent incorrect identification.
98-99
: Refactor to eliminate duplication when creatingTelemetryData
There's duplicated code when setting up
TelemetryData
instances. Refactoring can simplify the code and enhance maintainability.src/test/java/de/tum/cit/aet/artemis/telemetry/TelemetryServiceTest.java (6)
96-96
: Ensure Consistent Use of Boolean Flags in TestsWhen setting
telemetryServiceSpy.sendAdminDetails = false;
, ensure that the default value istrue
in the test context to validate the change effectively. This helps confirm that the test specifically checks the absence of personal data when the flag is disabled.Check the default value of
sendAdminDetails
in theTelemetryService
class to ensure the test behaves as intended.
63-63
: Use Consistent Exception Handling in Test MethodsThe
init()
method now throwsJsonProcessingException
. Verify if other test methods need to handle or declare this exception, ensuring consistency across the test class.Check if any overridden methods or interfaces require updating due to this change.
63-71
: Handle Exceptions Appropriately Instead of Empty Catch BlockUsing an empty catch block can suppress exceptions that may indicate underlying issues. Rather than ignoring the exception, consider logging it to aid in debugging if an unexpected error occurs.
Apply this diff to log the exception:
try { var eurekaURI = new URI(defaultZoneUrl); eurekaRequestUrl = eurekaURI.getScheme() + "://" + eurekaURI.getAuthority() + "/api/eureka/applications"; } - catch (Exception ignored) { - // Exception can be ignored because defaultZoneUrl is guaranteed to be valid in the test environment + catch (Exception e) { + // Log the exception for debugging purposes + e.printStackTrace(); }Alternatively, if the exception can truly be ignored, ensure that the caught exception is as specific as possible (e.g.,
URISyntaxException
) and maintain a comment explaining why it is safe to ignore.[Alternatively, as per the guidelines, the assistant should note that this was already addressed in past comments.]
However, since this issue was previously raised and addressed by adding a comment, you can consider this resolved.
91-91
: Review Increased Timeout DurationThe timeout in the
await
assertion has been increased from 1 second to 2 seconds in multiple tests. Ensure that this change is necessary and that the longer timeout does not mask potential performance issues.Consider whether the increased timeout is required due to environmental factors or if it indicates a slowdown in the code under test. If the latter, investigate and address the root cause.
Also applies to: 106-106, 126-126
14-15
: Import Only Necessary ClassesSome of the imported classes, such as
java.util.Map
orcom.fasterxml.jackson.core.JsonProcessingException
, might not be needed if they are not used elsewhere in the code. Review the imports and remove any that are unnecessary to keep the codebase clean.Also applies to: 31-31
73-73
: Ensure Compatibility with Older Java VersionsThe use of
Map.of()
andList.of()
requires Java 9 or higher. If your project aims to maintain compatibility with earlier Java versions, consider using alternative methods for creating maps and lists.Run the following script to check the project's Java version compatibility:
Verification successful
Java Version Compatibility Confirmed
The project's main
build.gradle
file specifies Java 21, which fully supportsMap.of()
andList.of()
methods. These methods are compatible with Java 9 and above, so there's no compatibility issue with older Java versions in the main project.Note:
- The main
build.gradle
usesJavaVersion.VERSION_21
- Some template
build.gradle
files use Java 17, which also supports these methodsThe original concern about maintaining compatibility with earlier Java versions is not applicable in this context.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the project is set to use Java 9 or higher. # Test: Look for Java version in build files. Expect: Java version 9 or higher. rg --type gradle --type maven 'sourceCompatibility|java.version' -A 2Length of output: 101
Script:
#!/bin/bash # Description: Check Java version in Gradle and Maven build files # Search for Gradle build files echo "Searching Gradle build files:" fd -e gradle -e gradle.kts | xargs -I {} sh -c 'echo "File: {}"; cat {} | grep -i "sourceCompatibility\|targetCompatibility\|JavaVersion"' echo -e "\nSearching Maven POM files:" fd -e xml -n pom.xml | xargs -I {} sh -c 'echo "File: {}"; cat {} | grep -i "<java.version>\|<maven.compiler.source>\|<maven.compiler.target>"'Length of output: 1894
@Value("${artemis.telemetry.destination}") | ||
private String destination; | ||
@Value("${artemis.telemetry.sendAdminDetails}") | ||
public boolean sendAdminDetails; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restrict field access to adhere to the principle of least access.
The fields sendAdminDetails
and eurekaEnabled
are declared as public
. To follow best practices and the principle of least access, they should be declared as private
unless they need to be accessed from outside this class.
Apply this diff to adjust the access modifiers:
- public boolean sendAdminDetails;
+ private boolean sendAdminDetails;
- public boolean eurekaEnabled;
+ private boolean eurekaEnabled;
Also applies to: 37-37
public record TelemetryData(String version, String serverUrl, String operator, List<String> profiles, boolean isProductionInstance, String dataSource, long numberOfNodes, | ||
long buildAgentCount, String contact, String adminName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure compliance with data privacy regulations when transmitting PII
The TelemetryData
record now includes potentially sensitive information such as contact
and adminName
. Please verify that collecting and transmitting this data complies with applicable data privacy laws and organizational policies. Consider anonymizing or securely handling PII before transmission.
/** | ||
* Assembles the telemetry data, and sends it to the external telemetry server. | ||
* | ||
* @throws Exception if the writing the telemetry data to a json format fails, or the connection to the telemetry server fails | ||
*/ | ||
@Async | ||
public void sendTelemetryByPostRequest() throws Exception { | ||
List<String> activeProfiles = Arrays.asList(env.getActiveProfiles()); | ||
public void sendTelemetryByPostRequest(boolean eurekaEnabled, boolean sendAdminDetails) throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specify precise exceptions instead of general Exception
Declaring throws Exception
is too broad and can obscure specific issues. It's better to specify the exact exceptions that might be thrown to improve code readability and error handling.
Apply this diff to specify precise exceptions:
-public void sendTelemetryByPostRequest(boolean eurekaEnabled, boolean sendAdminDetails) throws Exception {
+public void sendTelemetryByPostRequest(boolean eurekaEnabled, boolean sendAdminDetails) throws JsonProcessingException, RestClientException {
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
public void sendTelemetryByPostRequest(boolean eurekaEnabled, boolean sendAdminDetails) throws Exception { | |
public void sendTelemetryByPostRequest(boolean eurekaEnabled, boolean sendAdminDetails) throws JsonProcessingException, RestClientException { |
@Value("${spring.datasource.url}") | ||
private String datasourceUrl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
Implement validation and error handling for datasourceUrl
The current implementation lacks necessary checks and error handling for datasourceUrl
, which could lead to runtime errors. Consider the following improvements:
- Add null and empty checks for
datasourceUrl
before usage. - Implement error handling for cases where the URL doesn't match expected patterns.
- Use a more robust method to determine the database type, possibly leveraging Spring's
DataSourceProperties
orEnvironment
for consistency.
Example:
@Autowired
private Environment env;
// In the method where datasourceUrl is used
if (datasourceUrl == null || datasourceUrl.isEmpty()) {
throw new IllegalStateException("Datasource URL is not configured properly");
}
String dataSource;
if (datasourceUrl.startsWith("jdbc:mysql")) {
dataSource = "mysql";
} else if (datasourceUrl.startsWith("jdbc:postgresql")) {
dataSource = "postgresql";
} else {
throw new IllegalStateException("Unsupported database type in datasource URL");
}
These changes will improve the robustness of the TelemetrySendingService
and prevent potential runtime errors.
Analysis chain
Validate the datasource URL before usage
While retrieving the datasourceUrl
, ensure that it's validated to prevent potential NullPointerExceptions or malformed URLs, which could lead to runtime errors.
Please run the following script to check for null or empty datasourceUrl
assignments:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for assignments of 'datasourceUrl' that might be null or empty.
# Test: Search for places where 'datasourceUrl' could be null. Expect: No assignments of null values to 'datasourceUrl'.
rg --type java 'datasourceUrl\s*=\s*null' -A 2
Length of output: 48
Script:
#!/bin/bash
# Description: Analyze usage and potential validation of datasourceUrl
# Search for usage of datasourceUrl
echo "Usage of datasourceUrl:"
rg --type java "datasourceUrl" -A 2 -B 2
# Look for potential validation methods or checks
echo -e "\nPotential validation or checks:"
rg --type java "(validate|check|verify).*datasourceUrl" -A 2 -B 2
# Check for error handling mechanisms
echo -e "\nError handling related to datasourceUrl:"
rg --type java "(try|catch|throw).*datasourceUrl" -A 2 -B 2
Length of output: 1678
Script:
#!/bin/bash
# Description: Double-check for any other usages or validations of datasourceUrl
# Search for any other usages of datasourceUrl in the entire codebase
echo "All usages of datasourceUrl:"
rg --type java "datasourceUrl" -A 2 -B 2
# Look for any potential initialization or configuration of datasourceUrl
echo -e "\nPotential initialization or configuration:"
rg --type java "spring\.datasource\.url" -A 2 -B 2
Length of output: 2088
@Test | ||
void testSendTelemetry_TelemetryEnabledWithoutPersonalData() throws Exception { | ||
telemetryServiceSpy.sendAdminDetails = false; | ||
mockServer.expect(ExpectedCount.once(), requestTo(new URI(eurekaRequestUrl))).andExpect(method(HttpMethod.GET)) | ||
.andExpect(header(HttpHeaders.AUTHORIZATION, "Basic YWRtaW46YWRtaW4=")) | ||
.andRespond(withStatus(HttpStatus.OK).contentType(MediaType.APPLICATION_JSON).body(applicationsBody)); | ||
|
||
mockServer.expect(ExpectedCount.once(), requestTo(new URI(destination + "/api/telemetry"))).andExpect(method(HttpMethod.POST)) | ||
.andExpect(request -> assertThat(request.getBody().toString()).doesNotContain("adminName")) | ||
.andRespond(withStatus(HttpStatus.OK).contentType(MediaType.APPLICATION_JSON).body(mapper.writeValueAsString("Success!"))); | ||
telemetryServiceSpy.sendTelemetry(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid Code Duplication in Test Setup
The setup code in testSendTelemetry_TelemetryEnabledWithoutPersonalData()
is similar to other test methods. To promote maintainability and reduce duplication, consider refactoring the common setup code into a separate method or using parameterized tests.
Extract common mock server expectations into a helper method:
private void setupMockServerExpectations(boolean includeAdminName) throws URISyntaxException {
mockServer.expect(ExpectedCount.once(), requestTo(new URI(eurekaRequestUrl)))
.andExpect(method(HttpMethod.GET))
.andExpect(header(HttpHeaders.AUTHORIZATION, "Basic YWRtaW46YWRtaW4="))
.andRespond(withStatus(HttpStatus.OK)
.contentType(MediaType.APPLICATION_JSON)
.body(applicationsBody));
mockServer.expect(ExpectedCount.once(), requestTo(new URI(destination + "/api/telemetry")))
.andExpect(method(HttpMethod.POST))
.andExpect(request -> {
if (includeAdminName) {
assertThat(request.getBody().toString()).contains("adminName");
} else {
assertThat(request.getBody().toString()).doesNotContain("adminName");
}
})
.andRespond(withStatus(HttpStatus.OK)
.contentType(MediaType.APPLICATION_JSON)
.body(mapper.writeValueAsString("Success!")));
}
Then call this method in your tests:
@Test
void testSendTelemetry_TelemetryEnabled() throws Exception {
setupMockServerExpectations(true);
telemetryServiceSpy.sendTelemetry();
await().atMost(2, SECONDS).untilAsserted(() -> mockServer.verify());
}
@Test
void testSendTelemetry_TelemetryEnabledWithoutPersonalData() throws Exception {
telemetryServiceSpy.sendAdminDetails = false;
setupMockServerExpectations(false);
telemetryServiceSpy.sendTelemetry();
await().atMost(2, SECONDS).untilAsserted(() -> mockServer.verify());
}
import com.fasterxml.jackson.databind.ObjectMapper; | ||
|
||
import de.tum.cit.aet.artemis.AbstractSpringIntegrationIndependentTest; | ||
import de.tum.cit.aet.artemis.core.service.TelemetryService; | ||
import de.tum.cit.aet.artemis.core.service.telemetry.TelemetryService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Organize Imports According to Project Standards
Ensure that the import statements are organized as per the project's coding guidelines. Import statements should be grouped logically, and wildcards should be avoided unless necessary.
mockServer.expect(ExpectedCount.once(), requestTo(new URI(eurekaRequestUrl))).andExpect(method(HttpMethod.GET)) | ||
.andRespond(withStatus(HttpStatus.OK).contentType(MediaType.APPLICATION_JSON).body(applicationsBody)); | ||
mockServer.expect(ExpectedCount.once(), requestTo(new URI(destination + "/api/telemetry"))).andExpect(method(HttpMethod.POST)) | ||
.andRespond(withServerError().body(mapper.writeValueAsString("Failure!"))); | ||
|
||
telemetryServiceSpy.sendTelemetry(); | ||
await().atMost(1, SECONDS).untilAsserted(() -> mockServer.verify()); | ||
await().atMost(2, SECONDS).untilAsserted(() -> mockServer.verify()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle Server Errors Appropriately in Exception Handling Test
In testSendTelemetry_ExceptionHandling()
, ensure that the test not only verifies that an exception doesn't crash the application but also that it behaves as expected in case of server errors. Consider asserting the state after the error is handled.
Add assertions to verify the application's state post-error handling:
// After sending telemetry and simulating server error
telemetryServiceSpy.sendTelemetry();
// Verify that the application handled the error gracefully
// For example, check logs or internal flags if applicable
@Value("${eureka.client.service-url.defaultZone}") | ||
private String defaultZoneUrl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Descriptive Variable Names for Clarity
Consider renaming defaultZoneUrl
to eurekaDefaultZoneUrl
to make it clear that this URL is specific to Eureka's default zone. This enhances readability and helps other developers understand the context of the variable.
Apply this diff to rename the variable:
- @Value("${eureka.client.service-url.defaultZone}")
- private String defaultZoneUrl;
+ @Value("${eureka.client.service-url.defaultZone}")
+ private String eurekaDefaultZoneUrl;
Also, update all references to defaultZoneUrl
accordingly.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
@Value("${eureka.client.service-url.defaultZone}") | |
private String defaultZoneUrl; | |
@Value("${eureka.client.service-url.defaultZone}") | |
private String eurekaDefaultZoneUrl; |
.andExpect(request -> assertThat(request.getBody().toString()).doesNotContain("adminName")) | ||
.andRespond(withStatus(HttpStatus.OK).contentType(MediaType.APPLICATION_JSON).body(mapper.writeValueAsString("Success!"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expand Verification of Personal Data Exclusion
Currently, the test only checks that the request body does not contain "adminName"
. Consider verifying that other personal data fields are also excluded when sendAdminDetails
is set to false
.
Update the test assertion to check for additional personal data fields:
.andExpect(request -> {
String requestBody = request.getBody().toString();
assertThat(requestBody).doesNotContain("adminName");
+ assertThat(requestBody).doesNotContain("adminEmail");
+ assertThat(requestBody).doesNotContain("adminPassword");
})
Committable suggestion was skipped due to low confidence.
@Value("${spring.datasource.url}") | ||
private String datasourceUrl; | ||
|
||
@Value("${artemis.continuous-integration.concurrent-build-size}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value may not be defined when not using the localci
or buildagent
profiles and then result in a failure to start Artemis. The suggestion sets a default value in such cases.
@Value("${artemis.continuous-integration.concurrent-build-size}") | |
@Value("${artemis.continuous-integration.concurrent-build-size:0}") |
@Service | ||
public class EurekaClientService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add the @Profile
annotation here?
Rule 'classes that are annotated with @Service or are annotated with @Component or are annotated with @Configuration and do not belong to any of [de.tum.cit.aet.artemis.core.config.ApplicationConfiguration, de.tum.cit.aet.artemis.core.config.ConditionalMetricsExclusionConfiguration] should be annotated with @Profile, because we want to be able to exclude these classes from application startup by specifying profiles' was violated
/** | ||
* Retrieves the number of Artemis application instances registered with the Eureka server. | ||
* <p> | ||
* This method makes an HTTP GET request to the Eureka server's `/api/eureka/applications` endpoint to | ||
* retrieve a list of all registered applications. It then filters out the Artemis application | ||
* and returns the number of instances associated with it. If any error occurs during the request | ||
* (e.g., network issues, parsing issues, invalid URI), the method returns 1 as the default value. | ||
* | ||
* @return the number of Artemis application instances, or 1 if an error occurs. | ||
*/ | ||
public long getNumberOfReplicas() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How quickly do the Eureka instances register? Could the telemetry data be sent before all nodes have been started?
/** | ||
* Assembles the telemetry data, and sends it to the external telemetry server. | ||
* | ||
* @throws Exception if the writing the telemetry data to a json format fails, or the connection to the telemetry server fails | ||
*/ | ||
@Async | ||
public void sendTelemetryByPostRequest() throws Exception { | ||
List<String> activeProfiles = Arrays.asList(env.getActiveProfiles()); | ||
public void sendTelemetryByPostRequest(boolean eurekaEnabled, boolean sendAdminDetails) throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the javadocs as well
Checklist
General
Server
Motivation and Context
We already have a Telemetry service, which collects valuable data from Artemis instances of other universities. To gain more insights, we want to extend it by additional fields.
Description
This PR adds fields to the telemetry data json:
isProductionInstance
: boolean flag if running instance is a production instancedataSource
: if the datasource is mysql or postgresnumberOfNodes
: how many nodes are used, if Artemis runs in a multi node environment (1 if single node)buildAgentCount
: the number of employed build agentsSteps for Testing
Prerequisites:
dev
profile locally, make sure to manually comment out the check that disables the telemetry service by default on this profileTestserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
EurekaClientService
for interacting with the Eureka server to retrieve the number of registered instances.TelemetrySendingService
to gather and send more comprehensive telemetry data, including new fields likeisProductionInstance
anddataSource
.TelemetryService
to include new parameters for telemetry data sending.Bug Fixes
Refactor
Tests
TelemetryService
.