[GitHub] [systemds] Baunsgaard commented on a diff in pull request #1641: [SYSTEMDS-3385] Integration of frontend with monitoring backend

[GitBox <gi...@apache.org>]

Baunsgaard commented on code in PR #1641:
URL: https://github.com/apache/systemds/pull/1641#discussion_r905098797


##########
src/main/java/org/apache/sysds/runtime/controlprogram/federated/monitoring/models/NodeEntityModel.java:
##########
@@ -68,13 +83,43 @@ public void setStats(final List<BaseEntityModel> stats) {
 		_stats = stats;
 	}
 
+	public double getJitCompileTime() {
+		return _jitCompileTime;
+	}
+
+	public void setJitCompileTime(final double jitCompileTime) {
+		_jitCompileTime = jitCompileTime;
+	}
+
+	public String getRequestTypeCount() {
+		List<String> requestTypeCountStrArr = new ArrayList<>();
+
+		if (_requestTypeCount != null) {
+			for(var entry : _requestTypeCount) {
+				requestTypeCountStrArr.add(String.format("{" +

Review Comment:
   this is not nice, make the formatting string outside the for loop (i do not trust the java compiler to fix this.)



##########
src/main/java/org/apache/sysds/runtime/controlprogram/federated/monitoring/models/NodeEntityModel.java:
##########
@@ -68,13 +83,43 @@ public void setStats(final List<BaseEntityModel> stats) {
 		_stats = stats;
 	}
 
+	public double getJitCompileTime() {
+		return _jitCompileTime;
+	}
+
+	public void setJitCompileTime(final double jitCompileTime) {
+		_jitCompileTime = jitCompileTime;
+	}
+
+	public String getRequestTypeCount() {
+		List<String> requestTypeCountStrArr = new ArrayList<>();
+
+		if (_requestTypeCount != null) {
+			for(var entry : _requestTypeCount) {
+				requestTypeCountStrArr.add(String.format("{" +
+						"\"type\": \"%s\"," +
+						"\"count\": %d" +
+						"}", entry.getLeft().toString(), entry.getRight()));
+			}
+		}
+
+		return String.join(",", requestTypeCountStrArr);
+	}
+
+	public void setRequestTypeCount(final List<Pair<FederatedRequest.RequestType, Long>> requestTypeCount) {
+		_requestTypeCount = requestTypeCount;
+	}
+
 	@Override
 	public String toString() {
 		return String.format("{" +
 				"\"id\": %d," +
 				"\"name\": \"%s\"," +
 				"\"address\": \"%s\"," +
+				"\"isOnline\": %b," +
+				"\"jitCompileTime\": %.2f," +

Review Comment:
   same here, make a static string or something different.



##########
src/main/java/org/apache/sysds/runtime/controlprogram/federated/monitoring/models/StatsEntityModel.java:
##########
@@ -103,21 +134,22 @@ public void setHeavyHitterInstructions(final String heavyHitterInstructionsJsonS
 	}
 
 	public String getTransferredBytes() {
-		if (_transferredBytes.isEmpty() || _transferredBytes.isBlank()) {
-			StringBuilder sb = new StringBuilder();
+		if ((_transferredBytes.isEmpty() || _transferredBytes.isBlank()) && _transferredBytesObj != null) {
+
+			List<String> transferredBytesStrArr = new ArrayList<>();
 
-			sb.append("{");
 			for (var entry: _transferredBytesObj) {
-				sb.append(String.format("{" +
-					"\"datetime\": %s," +
+				transferredBytesStrArr.add(String.format("{" +
+					"\"datetime\": \"%s\"," +

Review Comment:
   format string outside for loop



##########
src/main/java/org/apache/sysds/runtime/controlprogram/federated/monitoring/models/StatsEntityModel.java:
##########
@@ -74,25 +97,33 @@ public double getMemoryUsage() {
 	public void setMemoryUsage(final double memoryUsage) {
 		_memoryUsage = memoryUsage;
 	}
+	public double getJitCompileTime() {
+		return _jitCompileTime;
+	}
+
+	public List<Pair<FederatedRequest.RequestType, Long>> getRequestTypeCount() {
+		return _requestTypeCount;
+	}
 
 	public String getHeavyHitterInstructions() {
-		if (_heavyHitterInstructions.isEmpty() || _heavyHitterInstructions.isBlank()) {
-			StringBuilder sb = new StringBuilder();
+		if ((_heavyHitterInstructions.isEmpty() || _heavyHitterInstructions.isBlank()) && _heavyHitterInstructionsObj != null) {
+
+			List<String> heavyHittersStrArr = new ArrayList<>();
 
-			sb.append("{");
 			for(Map.Entry<String, Pair<Long, Double>> entry : _heavyHitterInstructionsObj.entrySet()) {
 				String instruction = entry.getKey();
 				Long count = entry.getValue().getLeft();
 				double duration = entry.getValue().getRight();
-				sb.append(String.format("{" +
-					"\"instruction\": %s," +
-					"\"count\": \"%d\"," +
-					"\"duration\": \"%.2f\"," +
-					"},", instruction, count, duration));
+				heavyHittersStrArr.add(String.format("{" +
+					"\"instruction\": \"%s\"," +

Review Comment:
   make format string outside loop



##########
src/main/java/org/apache/sysds/runtime/controlprogram/federated/monitoring/repositories/DerbyRepository.java:
##########
@@ -43,17 +43,21 @@ public class DerbyRepository implements IRepository {
 	private static final String ENTITY_SCHEMA_CREATE_STATS_STMT = "CREATE TABLE %s " +
 			"(id INTEGER PRIMARY KEY GENERATED ALWAYS AS IDENTITY (START WITH 1, INCREMENT BY 1), " +
 			"%s INTEGER, " +

Review Comment:
   this is a good example of pre-computing your format string



##########
src/main/java/org/apache/sysds/runtime/controlprogram/federated/monitoring/services/StatsService.java:
##########
@@ -45,13 +50,20 @@ public static BaseEntityModel getWorkerStatistics(Long id, String address) {
 					aggFedStats.aggregate((FederatedStatistics.FedStatsCollection)tmp[0]);
 
 				parsedStats = new StatsEntityModel(
-					id, aggFedStats.cpuUsage, aggFedStats.memoryUsage,
-					aggFedStats.heavyHitters, aggFedStats.coordinatorsTrafficBytes);
+						id,
+						new Timestamp(System.currentTimeMillis()),
+						aggFedStats.cpuUsage,
+						aggFedStats.memoryUsage,
+						aggFedStats.jitCompileTime,
+						aggFedStats.heavyHitters,
+						aggFedStats.coordinatorsTrafficBytes,
+						aggFedStats.requestTypeCount);
 			}
-		} catch(DMLRuntimeException dre) {
+		} catch (DMLRuntimeException dre) {
 			// silently ignore -> caused by offline federated workers
+			log.error("Worker offline: " + dre.getMessage());
 		} catch (Exception e) {
-			throw new RuntimeException(e);
+			log.error("Error: " + e.getMessage());

Review Comment:
   this is super dangerous. 
   Please verify that this is the actual behavior you want, 
   can you limit the exception caught to something else?
   And if it really is the functionality you want add a comment saying that we do not handle the exception but just log it, and that it is fine.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org