Development: Add more data for Telemetry
#9345
Conversation
github-actions
bot
added
tests
server
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.ymlis excluded by!**/*.ymlsrc/test/resources/config/application.ymlis 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
TelemetryServiceclass to a dedicatedtelemetrysub-package aligns with the single responsibility principle and improves code organization.
15-15: LGTM!The import statement for
ProfileServicefollows 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
TelemetryServicereflects the restructuring of the package hierarchy. Moving the class to a dedicatedtelemetrysubpackage 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.telemetrybetter organizes telemetry-related services and enhances code maintainability.
24-24: ImportingProfileServicefor profile managementThe import of
ProfileServiceis appropriate for accessing profile-related information within the telemetry service.
33-34: Expansion ofTelemetryDatarecord aligns with data requirementsAdding new fields to
TelemetryDataallows 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
EurekaClientServiceandProfileServiceasprivate finalcomplies with the least access principle and supports immutability.
45-49: Constructor updated for proper dependency injectionIncluding
EurekaClientServiceandProfileServicein the constructor aligns with constructor injection best practices, promoting better testability and adherence to the single responsibility principle.
73-75: Injection ofdatasourceUrlusing@ValueannotationUsing
@Value("${spring.datasource.url}")to injectdatasourceUrlis appropriate and consistent with configuration management practices.
76-78: Injection ofeurekaEnabledflagThe use of
@Value("${eureka.client.enabled}")to inject theeurekaEnabledproperty correctly retrieves the configuration value.
79-81: Injection ofbuildAgentCountconfigurationInjecting
buildAgentCountwith@Value("${artemis.continuous-integration.concurrent-build-size}")appropriately accesses the configuration setting.
| @@ -70,11 +88,21 @@ public TelemetrySendingService(Environment env, RestTemplate restTemplate) { | |||
| public void sendTelemetryByPostRequest() throws Exception { | |||
| List<String> activeProfiles = Arrays.asList(env.getActiveProfiles()); | |||
| TelemetryData telemetryData; | |||
| var dataSource = datasourceUrl.startsWith("jdbc:mysql") ? "mysql" : "postgresql"; | |||
There was a problem hiding this comment.
Ensure robust detection of data source type
The current logic assumes only MySQL or defaults to PostgreSQL, potentially misclassifying other data sources. Consider handling additional data source types or adding validation to prevent incorrect identification.
Apply this diff to improve robustness:
-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);
+}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.
| 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); | |
| } |
| telemetryData = new TelemetryData(version, serverUrl, operator, activeProfiles, profileService.isProductionActive(), dataSource, numberOfInstances, buildAgentCount, | ||
| contact, operatorAdminName); | ||
| } | ||
| else { | ||
| telemetryData = new TelemetryData(version, serverUrl, operator, null, activeProfiles, null); | ||
| telemetryData = new TelemetryData(version, serverUrl, operator, activeProfiles, profileService.isProductionActive(), dataSource, numberOfInstances, buildAgentCount, | ||
| null, null); |
There was a problem hiding this comment.
Refactor to eliminate duplication when creating TelemetryData
Duplicated code in creating TelemetryData instances can be simplified to improve readability and maintainability.
Apply this diff to streamline the code:
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);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.
| telemetryData = new TelemetryData(version, serverUrl, operator, activeProfiles, profileService.isProductionActive(), dataSource, numberOfInstances, buildAgentCount, | |
| contact, operatorAdminName); | |
| } | |
| else { | |
| telemetryData = new TelemetryData(version, serverUrl, operator, null, activeProfiles, null); | |
| telemetryData = new TelemetryData(version, serverUrl, operator, activeProfiles, profileService.isProductionActive(), dataSource, numberOfInstances, buildAgentCount, | |
| null, null); | |
| String contactInfo = null; | |
| String adminName = null; | |
| if (sendAdminDetails) { | |
| contactInfo = contact; | |
| adminName = operatorAdminName; | |
| } | |
| telemetryData = new TelemetryData(version, serverUrl, operator, activeProfiles, profileService.isProductionActive(), dataSource, numberOfInstances, buildAgentCount, | |
| contactInfo, adminName); |
There was a problem hiding this comment.
nice suggestion
There was a problem hiding this comment.
@SimonEntholzer, glad you found the suggestion helpful! ((^.^)/)
| // Wait one minute to give other nodes time to connect to the Eureka registry | ||
| Thread.sleep(60000); |
There was a problem hiding this comment.
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.
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.ymlis 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
TelemetrySendingServiceto thetelemetrypackage improves code organization and aligns the class with its functionality.
33-34: ExpandedTelemetryDatarecord captures detailed metricsAdding new fields to
TelemetryDataallows for more comprehensive telemetry reporting, fulfilling the PR objectives of collecting richer data.
98-103: Refactor to eliminate duplication when creatingTelemetryDataDuplicating the creation of
TelemetryDatainstances 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.
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.
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.
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.ymlis 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
sendTelemetryByPostRequesthas been correctly updated with the new parameterseurekaEnabledandsendAdminDetails.
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 toTelemetryServicethroughout the codebase have been updated to reflect this change to preventClassNotFoundExceptionor 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.telemetryenhances modularity by grouping telemetry-related classes together.
24-24: Necessary import ofProfileServiceImporting
ProfileServiceis appropriate, as it's used to check if the production profile is active.
41-49: Good use of constructor injection for dependenciesInjecting
EurekaClientServiceandProfileServicevia the constructor adheres to the dependency injection principle, enhancing testability and modularity.
73-74: Confirm the accuracy ofbuildAgentCountvalueEnsure that the
buildAgentCountretrieved 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 creatingTelemetryDataThere's duplicated code when setting up
TelemetryDatainstances. 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 istruein 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
sendAdminDetailsin theTelemetryServiceclass 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
awaitassertion 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.Maporcom.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.gradlefile 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.gradleusesJavaVersion.VERSION_21- Some template
build.gradlefiles 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.
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.
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.
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.
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
datasourceUrlbefore 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
DataSourcePropertiesorEnvironmentfor 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.
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.
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.
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.
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.
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.
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}") |
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:
devprofile 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
EurekaClientServicefor interacting with the Eureka server to retrieve the number of registered instances.TelemetrySendingServiceto gather and send more comprehensive telemetry data, including new fields likeisProductionInstanceanddataSource.TelemetryServiceto include new parameters for telemetry data sending.Bug Fixes
Refactor
Tests
TelemetryService.