Copilot commented on code in PR #3863:
URL: https://github.com/apache/hertzbeat/pull/3863#discussion_r2569150404
##########
hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/store/history/tsdb/iotdb/IotDbDataStorage.java:
##########
@@ -427,23 +411,37 @@ private List<String> queryAllDevices(String deviceId) {
}
/**
- * use ${group}.${app}.${metrics}.${monitor}.${labels} to get device id if
there is a way to get instanceId
+ * use ${group}.${app}.${metrics}.${monitor}.${metric_labels} to get
device id if labels exist
* otherwise use ${group}.${app}.${metrics}.${monitor}
* Use ${group}.${app}.${metrics}.${monitor}.* to get all instance data
when you tend to query
*/
- private String getDeviceId(String app, String metrics, Long monitorId,
String labels, boolean useQuote) {
+ private String getDeviceId(String app, String metrics, String instance,
String labels, boolean useQuote) {
+ if (instance.contains(".")) {
+ instance = instance.replace(".", "-");
+ }
String deviceId = STORAGE_GROUP + "."
+ (useQuote ? addQuote(app) : app) + "."
+ (useQuote ? addQuote(metrics) : metrics) + "."
- + addQuote(monitorId.toString());
- if (labels != null && !labels.isEmpty() &&
!labels.equals(CommonConstants.NULL_VALUE)) {
+ + addQuote(instance);
+ if (labels != null && !labels.isEmpty() &&
!labels.equals(CommonConstants.NULL_VALUE) && !"{}".equals(labels)) {
deviceId += "." + addQuote(labels);
}
return deviceId;
}
/**
- * add quote，prevents keyword errors during queries(eg: nodes)
+ * Extract labels from device path
+ */
+ private String extractLabelsFromDevice(String device, String baseDeviceId)
{
+ if (device.length() > baseDeviceId.length() + 1) {
+ String labelsPart = device.substring(baseDeviceId.length() + 1);
+ return labelsPart.replace("`", "");
+ }
+ return "";
+ }
+
+ /**
+ * add quote,prevents keyword errors during queries(eg: nodes)
Review Comment:
Missing space after comma in the comment.
##########
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/NoticeConfigServiceImpl.java:
##########
@@ -305,7 +305,7 @@ public boolean sendTestMsg(NoticeReceiver noticeReceiver) {
Map<String, String> labels = new HashMap<>(8);
labels.put(CommonConstants.LABEL_INSTANCE, "1000000");
labels.put(CommonConstants.LABEL_ALERT_NAME, "CPU Usage Alert");
- labels.put(CommonConstants.LABEL_INSTANCE_HOST, "127.0.0.1");
+ labels.put(CommonConstants.LABEL_INSTANCE, "127.0.0.1");
Review Comment:
Duplicate label key usage. The same label key
`CommonConstants.LABEL_INSTANCE` is being used twice with different values
(lines 306 and 308). Line 306 sets it to "1000000" and line 308 sets it to
"127.0.0.1", which will overwrite the first value.
The original code had:
- Line 306: `LABEL_INSTANCE` -> "1000000" (monitor ID)
- Line 308: `LABEL_INSTANCE_HOST` -> "127.0.0.1" (host)
After the refactoring, you need to keep different keys or reconsider what
values should be in the labels. Consider keeping line 306 as monitor ID with a
different key if needed, or remove it if only the instance (host:port) is
needed.
```suggestion
labels.put(CommonConstants.LABEL_INSTANCE_HOST, "127.0.0.1");
```
##########
hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/store/history/tsdb/questdb/QuestdbDataStorage.java:
##########
@@ -359,8 +355,8 @@ private String getDateAdd(String history) {
return String.format("dateadd('%s', %d, now())", unit, -count);
}
- private String generateTable(String app, String metrics, Long monitorId) {
- return app + "_" + metrics + "_" + monitorId;
+ private String generateTable(String app, String metrics, String instance) {
+ return app + "_" + metrics + "_" + instance;
}
Review Comment:
The `generateTable` method uses `instance` directly in the table name
without sanitization. Since `instance` can contain special characters like
colons (e.g., "127.0.0.1:8080") or periods, this could create invalid table
names in QuestDB. You should sanitize the instance string:
```java
private String generateTable(String app, String metrics, String instance) {
if (instance.contains(":") || instance.contains(".")) {
instance = instance.replace(":", "-").replace(".", "-");
}
return app + "_" + metrics + "_" + instance;
}
```
##########
hertzbeat-startup/src/main/resources/db/migration/mysql/V180__update_column.sql:
##########
@@ -41,4 +41,86 @@ DELIMITER ;
CALL UpdateAlertDefineColumns();
DROP PROCEDURE IF EXISTS UpdateAlertDefineColumns;
+
+-- Rename host to instance
+DELIMITER //
+CREATE PROCEDURE RenameHostToInstance()
+BEGIN
+ DECLARE instance_exists INT;
+ DECLARE host_exists INT;
+ SELECT COUNT(*) INTO instance_exists FROM INFORMATION_SCHEMA.COLUMNS WHERE
TABLE_SCHEMA = DATABASE() AND TABLE_NAME = 'hzb_monitor' AND COLUMN_NAME =
'instance';
+ SELECT COUNT(*) INTO host_exists FROM INFORMATION_SCHEMA.COLUMNS WHERE
TABLE_SCHEMA = DATABASE() AND TABLE_NAME = 'hzb_monitor' AND COLUMN_NAME =
'host';
+ IF instance_exists > 0 THEN
+ IF host_exists > 0 THEN
+ SET @sql_update = 'UPDATE hzb_monitor SET instance = host WHERE
instance IS NULL';
+ PREPARE stmt_update FROM @sql_update;
+ EXECUTE stmt_update;
+ DEALLOCATE PREPARE stmt_update;
+
+ SET @sql_drop = 'ALTER TABLE hzb_monitor DROP COLUMN host';
+ PREPARE stmt_drop FROM @sql_drop;
+ EXECUTE stmt_drop;
+ DEALLOCATE PREPARE stmt_drop;
+ END IF;
+ ELSE
+ IF host_exists > 0 THEN
+ SET @sql_change = 'ALTER TABLE hzb_monitor CHANGE host instance
VARCHAR(100)';
+ PREPARE stmt_change FROM @sql_change;
+ EXECUTE stmt_change;
+ DEALLOCATE PREPARE stmt_change;
+ END IF;
+ END IF;
+END //
+DELIMITER ;
+CALL RenameHostToInstance();
+DROP PROCEDURE IF EXISTS RenameHostToInstance;
+
+-- Update instance with port
+UPDATE hzb_monitor m
+INNER JOIN hzb_param p ON m.id = p.monitor_id AND p.field = 'port'
+SET m.instance = CONCAT(m.instance, ':', p.param_value)
+WHERE m.instance IS NOT NULL;
Review Comment:
The update query to concatenate instance with port has a potential issue: it
doesn't check if `m.instance` already contains a port. If the migration is run
multiple times or if some instances already have ports, this will result in
duplicated port values (e.g., "127.0.0.1:8080:8080").
Add a condition to check if the instance doesn't already contain the port:
```sql
UPDATE hzb_monitor m
INNER JOIN hzb_param p ON m.id = p.monitor_id AND p.field = 'port'
SET m.instance = CONCAT(m.instance, ':', p.param_value)
WHERE m.instance IS NOT NULL
AND m.instance NOT LIKE CONCAT('%:', p.param_value);
```
```suggestion
WHERE m.instance IS NOT NULL
AND m.instance NOT LIKE CONCAT('%:', p.param_value);
```
##########
hertzbeat-startup/src/main/resources/db/migration/postgresql/V180__update_column.sql:
##########
@@ -27,4 +27,48 @@ UPDATE HZB_ALERT_DEFINE
SET type = 'periodic_metric'
WHERE type = 'periodic';
+-- Rename host to instance
+DO $$
+BEGIN
+ IF EXISTS(SELECT * FROM information_schema.columns WHERE table_name =
'hzb_monitor' AND column_name = 'instance') THEN
+ IF EXISTS(SELECT * FROM information_schema.columns WHERE table_name =
'hzb_monitor' AND column_name = 'host') THEN
+ EXECUTE 'UPDATE HZB_MONITOR SET instance = host WHERE instance IS
NULL';
+ EXECUTE 'ALTER TABLE HZB_MONITOR DROP COLUMN host';
+ END IF;
+ ELSE
+ IF EXISTS(SELECT * FROM information_schema.columns WHERE table_name =
'hzb_monitor' AND column_name = 'host') THEN
+ EXECUTE 'ALTER TABLE HZB_MONITOR RENAME COLUMN host TO instance';
+ END IF;
+ END IF;
+END $$;
+
+-- Update instance with port
+UPDATE HZB_MONITOR m
+SET instance = m.instance || ':' || p.param_value
+FROM HZB_PARAM p
+WHERE m.id = p.monitor_id AND p.field = 'port';
Review Comment:
The update query to concatenate instance with port has a potential issue: it
doesn't check if `m.instance` already contains a port. If the migration is run
multiple times or if some instances already have ports, this will result in
duplicated port values (e.g., "127.0.0.1:8080:8080").
Add a condition to check if the instance doesn't already contain the port:
```sql
UPDATE HZB_MONITOR m
SET instance = m.instance || ':' || p.param_value
FROM HZB_PARAM p
WHERE m.id = p.monitor_id
AND p.field = 'port'
AND m.instance NOT LIKE '%:' || p.param_value;
```
```suggestion
WHERE m.id = p.monitor_id AND p.field = 'port' AND m.instance NOT LIKE '%:'
|| p.param_value;
```
##########
hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/store/history/tsdb/tdengine/TdEngineDataStorage.java:
##########
@@ -384,48 +391,43 @@ public Map<String, List<Value>> getHistoryMetricData(Long
monitorId, String app,
}
@Override
- public Map<String, List<Value>> getHistoryIntervalMetricData(Long
monitorId, String app, String metrics,
- String
metric, String label, String history) {
+ public Map<String, List<Value>> getHistoryIntervalMetricData(String
instance, String app, String metrics,
+ String
metric, String history) {
if (!serverAvailable) {
INSTANCE_EXCEPTION_PRINT.run();
return Collections.emptyMap();
}
- String table = app + "_" + metrics + "_" + monitorId;
+ String table = getTable(app, metrics, instance);
List<String> instances = new LinkedList<>();
- if (label != null) {
- instances.add(label);
- }
- if (instances.isEmpty()) {
- // need to confirm that how many instances of current metrics one
week ago
- String queryInstanceSql = String.format(QUERY_INSTANCE_SQL, table);
- Connection connection = null;
- try {
- connection = hikariDataSource.getConnection();
- Statement statement = connection.createStatement();
- ResultSet resultSet = statement.executeQuery(queryInstanceSql);
- while (resultSet.next()) {
- String instanceValue = resultSet.getString(1);
- if (instanceValue == null ||
StringUtils.isBlank(instanceValue)) {
- instances.add("''");
- } else {
- instances.add(instanceValue);
- }
+ // query all metric_labels from the table
+ String queryInstanceSql = String.format(QUERY_INSTANCE_SQL, table);
+ Connection connection = null;
+ try {
+ connection = hikariDataSource.getConnection();
+ Statement statement = connection.createStatement();
+ ResultSet resultSet = statement.executeQuery(queryInstanceSql);
Review Comment:
This ResultSet is not always closed on method exit.
##########
hertzbeat-startup/src/main/resources/db/migration/h2/V180__update_column.sql:
##########
@@ -26,3 +26,68 @@ WHERE type = 'realtime';
UPDATE HZB_ALERT_DEFINE
SET type = 'periodic_metric'
WHERE type = 'periodic';
+
+-- Rename host to instance
+CREATE ALIAS RENAME_HOST_TO_INSTANCE AS $$
+void renameHostToInstance(java.sql.Connection conn) throws
java.sql.SQLException {
+ boolean instanceExists = false;
+ boolean hostExists = false;
+ try (java.sql.ResultSet rs = conn.getMetaData().getColumns(null, null,
"HZB_MONITOR", "INSTANCE")) {
+ if (rs.next()) instanceExists = true;
+ }
+ try (java.sql.ResultSet rs = conn.getMetaData().getColumns(null, null,
"HZB_MONITOR", "HOST")) {
+ if (rs.next()) hostExists = true;
+ }
+ try (java.sql.Statement stmt = conn.createStatement()) {
+ if (instanceExists) {
+ if (hostExists) {
+ stmt.execute("UPDATE HZB_MONITOR SET instance = host WHERE
instance IS NULL");
+ stmt.execute("ALTER TABLE HZB_MONITOR DROP COLUMN host");
+ }
+ } else {
+ if (hostExists) {
+ stmt.execute("ALTER TABLE HZB_MONITOR ALTER COLUMN host RENAME
TO instance");
+ }
+ }
+ }
+}
+$$;
+CALL RENAME_HOST_TO_INSTANCE();
+DROP ALIAS RENAME_HOST_TO_INSTANCE;
+
+-- Update instance with port
+UPDATE HZB_MONITOR m
+SET instance = CONCAT(instance, ':', (SELECT param_value FROM HZB_PARAM p
WHERE p.monitor_id = m.id AND p.field = 'port'))
+WHERE EXISTS (SELECT 1 FROM HZB_PARAM p WHERE p.monitor_id = m.id AND p.field
= 'port');
Review Comment:
The update query to concatenate instance with port has a potential issue: it
doesn't check if the instance already contains a port. If the migration is run
multiple times or if some instances already have ports, this will result in
duplicated port values (e.g., "127.0.0.1:8080:8080").
Modify the WHERE clause to check if the port isn't already present:
```sql
UPDATE HZB_MONITOR m
SET instance = CONCAT(instance, ':', (SELECT param_value FROM HZB_PARAM p
WHERE p.monitor_id = m.id AND p.field = 'port'))
WHERE EXISTS (SELECT 1 FROM HZB_PARAM p WHERE p.monitor_id = m.id AND
p.field = 'port')
AND instance NOT LIKE '%:' || (SELECT param_value FROM HZB_PARAM p WHERE
p.monitor_id = m.id AND p.field = 'port');
```
```suggestion
WHERE EXISTS (SELECT 1 FROM HZB_PARAM p WHERE p.monitor_id = m.id AND
p.field = 'port')
AND instance NOT LIKE '%:' || (SELECT param_value FROM HZB_PARAM p WHERE
p.monitor_id = m.id AND p.field = 'port');
```
##########
hertzbeat-ai/src/main/java/org/apache/hertzbeat/ai/tools/impl/MetricsToolsImpl.java:
##########
@@ -170,15 +170,15 @@ public String getHistoricalMetrics(
interval = true;
}
- MetricsHistoryData historyData =
metricsDataService.getMetricHistoryData(
- monitorId, app, metrics, fieldParameter, label, history,
interval);
+ MetricsHistoryData historyData =
metricsDataService.getMetricHistoryData(instance,
+ app, metrics, fieldParameter, history, interval);
if (historyData == null) {
- return String.format("No historical metrics data found for
monitor ID %d and metrics '%s'", monitorId, metrics);
+ return String.format("No historical metrics data found for
monitor %d and metrics '%s'", instance, metrics);
Review Comment:
String format mismatch: the format string expects a numeric value (`%d`) but
`instance` is now a String. Change `%d` to `%s`:
```java
return String.format("No historical metrics data found for monitor %s and
metrics '%s'", instance, metrics);
```
```suggestion
return String.format("No historical metrics data found for
monitor %s and metrics '%s'", instance, metrics);
```
##########
hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/store/history/tsdb/tdengine/TdEngineDataStorage.java:
##########
@@ -314,6 +317,11 @@ public void saveData(CollectRep.MetricsData metricsData) {
}
+ @NotNull
+ private static String getTable(String app, String metrics, String
instance) {
+ return app + "_" + metrics + "_" + instance;
+ }
Review Comment:
The helper method `getTable` receives `instance` as a parameter but uses it
directly in the table name concatenation. Since `instance` can now contain
special characters like colons (e.g., "127.0.0.1:8080") or periods, this could
create invalid table names in TDengine. You should sanitize the instance string
before using it in the table name, similar to what is done in IotDbDataStorage
at line 419-421:
```java
private static String getTable(String app, String metrics, String instance) {
if (instance.contains(":") || instance.contains(".")) {
instance = instance.replace(":", "-").replace(".", "-");
}
return app + "_" + metrics + "_" + instance;
}
```
##########
hertzbeat-common/src/main/java/org/apache/hertzbeat/common/entity/dto/MetricsHistoryData.java:
##########
@@ -35,8 +35,8 @@
@Schema(description = "Metric History Range Query Data")
public class MetricsHistoryData {
- @Schema(title = "Monitoring Task ID")
- private Long id;
+ @Schema(title = "Monitor ID")
Review Comment:
The Schema description is misleading. It says "Monitor ID" but the field is
now `instance` which is not an ID but an instance identifier (e.g.,
"127.0.0.1:8080"). Update the description:
```java
@Schema(title = "Monitor Instance (e.g., ip:port or domain)")
```
```suggestion
@Schema(title = "Monitor Instance (e.g., ip:port or domain)")
```
##########
hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/store/history/tsdb/influxdb/InfluxdbDataStorage.java:
##########
@@ -309,8 +300,8 @@ public Map<String, List<Value>>
getHistoryIntervalMetricData(Long monitorId, Str
return instanceValueMap;
}
- private String generateTable(String app, String metrics, Long monitorId) {
- return app + "_" + metrics + "_" + monitorId;
+ private String generateTable(String app, String metrics, String instance) {
+ return app + "_" + metrics + "_" + instance;
}
Review Comment:
The `generateTable` method uses `instance` directly in the table name
without sanitization. Since `instance` can contain special characters like
colons (e.g., "127.0.0.1:8080") or periods, this could create invalid table
names in InfluxDB. You should sanitize the instance string:
```java
private String generateTable(String app, String metrics, String instance) {
if (instance.contains(":") || instance.contains(".")) {
instance = instance.replace(":", "-").replace(".", "-");
}
return app + "_" + metrics + "_" + instance;
}
```
##########
hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/controller/MetricsDataController.java:
##########
@@ -75,15 +75,13 @@ public ResponseEntity<Message<MetricsData>> getMetricsData(
return ResponseEntity.ok(Message.success(metricsData));
}
- @GetMapping("/api/monitor/{monitorId}/metric/{metricFull}")
+ @GetMapping("/api/monitor/{instance}/metric/{metricFull}")
@Operation(summary = "Queries historical data for a specified metric for
monitoring", description = "Queries historical data for a specified metric
under monitoring")
public ResponseEntity<Message<MetricsHistoryData>> getMetricHistoryData(
- @Parameter(description = "monitor the task ID", example =
"343254354")
- @PathVariable Long monitorId,
+ @Parameter(description = "monitor instance", example = "343254354")
Review Comment:
The parameter description example is incorrect. It says "monitor instance"
but the example value "343254354" looks like a numeric monitor ID, not an
instance identifier. Update the example to match an instance format:
```java
@Parameter(description = "monitor instance", example = "127.0.0.1:8080")
```
```suggestion
@Parameter(description = "monitor instance", example =
"127.0.0.1:8080")
```
##########
hertzbeat-common/src/main/java/org/apache/hertzbeat/common/support/valid/HostParamValidator.java:
##########
@@ -44,7 +44,21 @@ public boolean isValid(String value,
ConstraintValidatorContext context) {
value = value.replaceFirst(PATTERN_HTTPS, BLANK);
}
- return IpDomainUtil.validateIpDomain(value);
+ String hostPart = value;
+
+ if (value.contains(":")) {
+ // if contains multiple ":", it may be IPv6 with port
+ if (value.lastIndexOf(":") > value.indexOf(":") &&
value.contains("[")) {
+ int portIndex = value.lastIndexOf(":");
+ hostPart = value.substring(0, portIndex);
Review Comment:
The IPv6 detection logic is incorrect. The condition `value.lastIndexOf(":")
> value.indexOf(":")` will be true for any string with more than one colon, but
this doesn't necessarily mean it's an IPv6 address with a port. For example,
"fe80::1:2:3" is a valid IPv6 without port, but this code would incorrectly try
to parse it.
A more robust approach would be:
```java
if (value.startsWith("[") && value.contains("]:")) {
// IPv6 with port: [fe80::1]:8080
int portIndex = value.lastIndexOf("]:");
hostPart = value.substring(0, portIndex + 1);
}
```
```suggestion
// IPv6 with port: [address]:port
if (value.startsWith("[") && value.contains("]:")) {
int portIndex = value.lastIndexOf("]:");
hostPart = value.substring(0, portIndex + 1);
```
##########
hertzbeat-manager/src/main/java/org/apache/hertzbeat/manager/component/sd/ServiceDiscoveryWorker.java:
##########
@@ -123,13 +124,14 @@ public void run() {
.filter(p -> !p.isEmpty())
.orElse(defaultPort);
final String keyStr = host + ":" + port;
+ final String instance = Objects.equals(port, "") ?
host : host + ":" + port;
Review Comment:
The instance construction logic checks for empty string, but it should also
handle the case when `port` is `null`. Consider using:
```java
final String instance = port == null || port.isEmpty() ? host : host + ":" +
port;
```
This is more explicit and handles the null case properly.
```suggestion
final String instance = (port == null ||
port.isEmpty()) ? host : host + ":" + port;
```
##########
hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/store/history/tsdb/tdengine/TdEngineDataStorage.java:
##########
@@ -384,48 +391,43 @@ public Map<String, List<Value>> getHistoryMetricData(Long
monitorId, String app,
}
@Override
- public Map<String, List<Value>> getHistoryIntervalMetricData(Long
monitorId, String app, String metrics,
- String
metric, String label, String history) {
+ public Map<String, List<Value>> getHistoryIntervalMetricData(String
instance, String app, String metrics,
+ String
metric, String history) {
if (!serverAvailable) {
INSTANCE_EXCEPTION_PRINT.run();
return Collections.emptyMap();
}
- String table = app + "_" + metrics + "_" + monitorId;
+ String table = getTable(app, metrics, instance);
List<String> instances = new LinkedList<>();
- if (label != null) {
- instances.add(label);
- }
- if (instances.isEmpty()) {
- // need to confirm that how many instances of current metrics one
week ago
- String queryInstanceSql = String.format(QUERY_INSTANCE_SQL, table);
- Connection connection = null;
- try {
- connection = hikariDataSource.getConnection();
- Statement statement = connection.createStatement();
- ResultSet resultSet = statement.executeQuery(queryInstanceSql);
- while (resultSet.next()) {
- String instanceValue = resultSet.getString(1);
- if (instanceValue == null ||
StringUtils.isBlank(instanceValue)) {
- instances.add("''");
- } else {
- instances.add(instanceValue);
- }
+ // query all metric_labels from the table
+ String queryInstanceSql = String.format(QUERY_INSTANCE_SQL, table);
+ Connection connection = null;
+ try {
+ connection = hikariDataSource.getConnection();
+ Statement statement = connection.createStatement();
Review Comment:
This Statement is not always closed on method exit.
##########
hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/store/history/tsdb/tdengine/TdEngineDataStorage.java:
##########
@@ -441,10 +443,10 @@ public Map<String, List<Value>>
getHistoryIntervalMetricData(Long monitorId, Str
}
List<Value> values =
instanceValuesMap.computeIfAbsent(instanceValue, k -> new LinkedList<>());
- Connection connection = null;
+ Connection conn = null;
try {
- connection = hikariDataSource.getConnection();
- Statement statement = connection.createStatement();
+ conn = hikariDataSource.getConnection();
+ Statement statement = conn.createStatement();
Review Comment:
This Statement is not always closed on method exit.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
