-
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?
Changes from 3 commits
848eb38
3a5d86f
3829a34
e5c690a
7c972b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
package de.tum.cit.aet.artemis.core.service.telemetry; | ||
|
||
import java.net.URI; | ||
import java.net.URISyntaxException; | ||
import java.nio.charset.StandardCharsets; | ||
import java.util.Base64; | ||
import java.util.Collections; | ||
|
||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
import org.springframework.beans.factory.annotation.Value; | ||
import org.springframework.http.HttpEntity; | ||
import org.springframework.http.HttpHeaders; | ||
import org.springframework.http.HttpMethod; | ||
import org.springframework.http.MediaType; | ||
import org.springframework.http.ResponseEntity; | ||
import org.springframework.stereotype.Service; | ||
import org.springframework.web.client.RestClientException; | ||
import org.springframework.web.client.RestTemplate; | ||
|
||
import com.fasterxml.jackson.core.JacksonException; | ||
import com.fasterxml.jackson.databind.JsonNode; | ||
import com.fasterxml.jackson.databind.ObjectMapper; | ||
|
||
@Service | ||
public class EurekaClientService { | ||
|
||
private static final Logger log = LoggerFactory.getLogger(EurekaClientService.class); | ||
|
||
@Value("${eureka.client.service-url.defaultZone}") | ||
private String eurekaServiceUrl; | ||
|
||
private final RestTemplate restTemplate; | ||
|
||
public EurekaClientService(RestTemplate restTemplate) { | ||
this.restTemplate = restTemplate; | ||
} | ||
|
||
/** | ||
* 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() { | ||
Comment on lines
+39
to
+49
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
try { | ||
var eurekaURI = new URI(eurekaServiceUrl); | ||
HttpHeaders headers = createHeaders(eurekaURI.getUserInfo()); | ||
HttpEntity<String> request = new HttpEntity<>(headers); | ||
var requestUrl = eurekaURI.getScheme() + "://" + eurekaURI.getAuthority() + "/api/eureka/applications"; | ||
|
||
ResponseEntity<String> response = restTemplate.exchange(requestUrl, HttpMethod.GET, request, String.class); | ||
|
||
ObjectMapper objectMapper = new ObjectMapper(); | ||
JsonNode rootNode = objectMapper.readTree(response.getBody()); | ||
JsonNode applicationsNode = rootNode.get("applications"); | ||
for (JsonNode application : applicationsNode) { | ||
if (application.get("name").asText().equals("ARTEMIS")) { | ||
JsonNode instancesNode = application.get("instances"); | ||
return instancesNode.size(); | ||
} | ||
} | ||
} | ||
catch (RestClientException | JacksonException | URISyntaxException e) { | ||
log.warn("Error while trying to retrieve number of replicas."); | ||
} | ||
|
||
return 1; | ||
} | ||
|
||
/** | ||
* Creates HTTP headers with Basic Authentication and JSON content type. | ||
* | ||
* @param auth the user credentials in the format "username:password" to be encoded and included in the Authorization header. | ||
* @return HttpHeaders with Basic Authentication and JSON content types. | ||
*/ | ||
private HttpHeaders createHeaders(String auth) { | ||
HttpHeaders headers = new HttpHeaders(); | ||
headers.setContentType(MediaType.APPLICATION_JSON); | ||
headers.setAccept(Collections.singletonList(MediaType.APPLICATION_JSON)); | ||
|
||
byte[] encodedAuth = Base64.getEncoder().encode(auth.getBytes(StandardCharsets.US_ASCII)); | ||
String authHeader = "Basic " + new String(encodedAuth); | ||
|
||
headers.set("Authorization", authHeader); | ||
return headers; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,4 +1,4 @@ | ||||||
package de.tum.cit.aet.artemis.core.service; | ||||||
package de.tum.cit.aet.artemis.core.service.telemetry; | ||||||
|
||||||
import static de.tum.cit.aet.artemis.core.config.Constants.PROFILE_SCHEDULING; | ||||||
|
||||||
|
@@ -21,23 +21,32 @@ | |||||
import com.fasterxml.jackson.databind.ObjectMapper; | ||||||
import com.fasterxml.jackson.databind.ObjectWriter; | ||||||
|
||||||
import de.tum.cit.aet.artemis.core.service.ProfileService; | ||||||
|
||||||
@Service | ||||||
@Profile(PROFILE_SCHEDULING) | ||||||
public class TelemetrySendingService { | ||||||
|
||||||
private static final Logger log = LoggerFactory.getLogger(TelemetrySendingService.class); | ||||||
|
||||||
@JsonInclude(JsonInclude.Include.NON_EMPTY) | ||||||
public record TelemetryData(String version, String serverUrl, String operator, String contact, List<String> profiles, String adminName) { | ||||||
public record TelemetryData(String version, String serverUrl, String operator, List<String> profiles, boolean isProductionInstance, String dataSource, long numberOfNodes, | ||||||
long buildAgentCount, String contact, String adminName) { | ||||||
} | ||||||
Comment on lines
+33
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure compliance with data privacy regulations when transmitting PII The |
||||||
|
||||||
private final Environment env; | ||||||
|
||||||
private final RestTemplate restTemplate; | ||||||
|
||||||
public TelemetrySendingService(Environment env, RestTemplate restTemplate) { | ||||||
private final EurekaClientService eurekaClientService; | ||||||
|
||||||
private final ProfileService profileService; | ||||||
|
||||||
public TelemetrySendingService(Environment env, RestTemplate restTemplate, EurekaClientService eurekaClientService, ProfileService profileService) { | ||||||
this.env = env; | ||||||
this.restTemplate = restTemplate; | ||||||
this.eurekaClientService = eurekaClientService; | ||||||
this.profileService = profileService; | ||||||
} | ||||||
|
||||||
@Value("${artemis.version}") | ||||||
|
@@ -61,6 +70,15 @@ public TelemetrySendingService(Environment env, RestTemplate restTemplate) { | |||||
@Value("${artemis.telemetry.destination}") | ||||||
private String destination; | ||||||
|
||||||
@Value("${spring.datasource.url}") | ||||||
private String datasourceUrl; | ||||||
|
||||||
Comment on lines
+70
to
+71
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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 Analysis chainValidate the datasource URL before usage While retrieving the Please run the following script to check for null or empty Scripts executedThe 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 |
||||||
@Value("${eureka.client.enabled}") | ||||||
private boolean eurekaEnabled; | ||||||
|
||||||
@Value("${artemis.continuous-integration.concurrent-build-size}") | ||||||
private long buildAgentCount; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This value may not be defined when not using the
Suggested change
|
||||||
|
||||||
/** | ||||||
* Assembles the telemetry data, and sends it to the external telemetry server. | ||||||
* | ||||||
|
@@ -70,11 +88,19 @@ 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"; | ||||||
long numberOfInstances = 1; | ||||||
SimonEntholzer marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
if (eurekaEnabled) { | ||||||
numberOfInstances = eurekaClientService.getNumberOfReplicas(); | ||||||
} | ||||||
|
||||||
if (sendAdminDetails) { | ||||||
telemetryData = new TelemetryData(version, serverUrl, operator, contact, activeProfiles, operatorAdminName); | ||||||
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); | ||||||
} | ||||||
SimonEntholzer marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
HttpHeaders headers = new HttpHeaders(); | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2,31 +2,36 @@ | |||||||||
|
||||||||||
import static java.util.concurrent.TimeUnit.SECONDS; | ||||||||||
import static org.mockito.Mockito.spy; | ||||||||||
import static org.springframework.test.web.client.match.MockRestRequestMatchers.header; | ||||||||||
import static org.springframework.test.web.client.match.MockRestRequestMatchers.method; | ||||||||||
import static org.springframework.test.web.client.match.MockRestRequestMatchers.requestTo; | ||||||||||
import static org.springframework.test.web.client.response.MockRestResponseCreators.withServerError; | ||||||||||
import static org.springframework.test.web.client.response.MockRestResponseCreators.withStatus; | ||||||||||
import static org.testcontainers.shaded.org.awaitility.Awaitility.await; | ||||||||||
|
||||||||||
import java.net.URI; | ||||||||||
import java.util.List; | ||||||||||
import java.util.Map; | ||||||||||
|
||||||||||
import org.junit.jupiter.api.BeforeEach; | ||||||||||
import org.junit.jupiter.api.Test; | ||||||||||
import org.junit.jupiter.api.extension.ExtendWith; | ||||||||||
import org.mockito.junit.jupiter.MockitoExtension; | ||||||||||
import org.springframework.beans.factory.annotation.Autowired; | ||||||||||
import org.springframework.beans.factory.annotation.Value; | ||||||||||
import org.springframework.http.HttpHeaders; | ||||||||||
import org.springframework.http.HttpMethod; | ||||||||||
import org.springframework.http.HttpStatus; | ||||||||||
import org.springframework.http.MediaType; | ||||||||||
import org.springframework.test.web.client.ExpectedCount; | ||||||||||
import org.springframework.test.web.client.MockRestServiceServer; | ||||||||||
import org.springframework.web.client.RestTemplate; | ||||||||||
|
||||||||||
import com.fasterxml.jackson.core.JsonProcessingException; | ||||||||||
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 commentThe 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. |
||||||||||
|
||||||||||
@ExtendWith(MockitoExtension.class) | ||||||||||
class TelemetryServiceTest extends AbstractSpringIntegrationIndependentTest { | ||||||||||
|
@@ -46,19 +51,45 @@ class TelemetryServiceTest extends AbstractSpringIntegrationIndependentTest { | |||||||||
@Value("${artemis.telemetry.destination}") | ||||||||||
private String destination; | ||||||||||
|
||||||||||
@Value("${eureka.client.service-url.defaultZone}") | ||||||||||
private String defaultZoneUrl; | ||||||||||
Comment on lines
+55
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use Descriptive Variable Names for Clarity Consider renaming 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 Committable suggestion
Suggested change
|
||||||||||
|
||||||||||
private String eurekaRequestUrl; | ||||||||||
|
||||||||||
private byte[] appliciationsBody; | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 |
||||||||||
|
||||||||||
@BeforeEach | ||||||||||
void init() { | ||||||||||
void init() throws JsonProcessingException { | ||||||||||
try { | ||||||||||
var eurekaURI = new URI(defaultZoneUrl); | ||||||||||
eurekaRequestUrl = eurekaURI.getScheme() + "://" + eurekaURI.getAuthority() + "/api/eureka/applications"; | ||||||||||
|
||||||||||
} | ||||||||||
catch (Exception ignored) { | ||||||||||
} | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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
}
|
||||||||||
|
||||||||||
telemetryServiceSpy = spy(telemetryService); | ||||||||||
mockServer = MockRestServiceServer.createServer(restTemplate); | ||||||||||
|
||||||||||
appliciationsBody = mapper.writeValueAsBytes(Map.of("applications", List.of(Map.of("name", "ARTEMIS", "instances", List.of(Map.of()) // Mocking an instance object | ||||||||||
)))); | ||||||||||
MockRestServiceServer.MockRestServiceServerBuilder builder = MockRestServiceServer.bindTo(restTemplate); | ||||||||||
builder.ignoreExpectOrder(true); | ||||||||||
mockServer = builder.build(); | ||||||||||
|
||||||||||
telemetryServiceSpy.useTelemetry = true; | ||||||||||
} | ||||||||||
|
||||||||||
@Test | ||||||||||
void testSendTelemetry_TelemetryEnabled() throws Exception { | ||||||||||
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(appliciationsBody)); | ||||||||||
|
||||||||||
mockServer.expect(ExpectedCount.once(), requestTo(new URI(destination + "/api/telemetry"))).andExpect(method(HttpMethod.POST)) | ||||||||||
.andRespond(withStatus(HttpStatus.OK).contentType(MediaType.APPLICATION_JSON).body(mapper.writeValueAsString("Success!"))); | ||||||||||
telemetryServiceSpy.sendTelemetry(); | ||||||||||
await().atMost(1, SECONDS).untilAsserted(() -> mockServer.verify()); | ||||||||||
|
||||||||||
await().atMost(2, SECONDS).untilAsserted(() -> mockServer.verify()); | ||||||||||
} | ||||||||||
|
||||||||||
@Test | ||||||||||
|
@@ -72,9 +103,12 @@ void testSendTelemetry_TelemetryDisabled() throws Exception { | |||||||||
|
||||||||||
@Test | ||||||||||
void testSendTelemetry_ExceptionHandling() throws Exception { | ||||||||||
mockServer.expect(ExpectedCount.once(), requestTo(new URI(eurekaRequestUrl))).andExpect(method(HttpMethod.GET)) | ||||||||||
.andRespond(withStatus(HttpStatus.OK).contentType(MediaType.APPLICATION_JSON).body(appliciationsBody)); | ||||||||||
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.
Could you add the
@Profile
annotation here?