Copilot commented on code in PR #3985:
URL: https://github.com/apache/hertzbeat/pull/3985#discussion_r2707516837
##########
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/DataSourceServiceImpl.java:
##########
@@ -168,4 +168,34 @@ private CommonTokenStream createTokenStream(String expr) {
AlertExpressionLexer lexer = new
AlertExpressionLexer(CharStreams.fromString(expr));
return new CommonTokenStream(lexer);
}
+
+ @Override
+ public Map<String, Object> getAvailableExecutors() {
+ boolean hasPromqlExecutor = false;
+ boolean hasSqlExecutor = false;
+ java.util.Set<String> availableExecutors = new java.util.HashSet<>();
+
+ if (executors != null) {
+ for (QueryExecutor executor : executors) {
+ String datasource = executor.getDatasource();
+ availableExecutors.add(datasource);
+
+ // Check if executor supports promql
+ if (executor.support(WarehouseConstants.PROMQL)) {
+ hasPromqlExecutor = true;
+ }
+ // Check if executor supports sql
+ if (executor.support(WarehouseConstants.SQL)) {
+ hasSqlExecutor = true;
+ }
+ }
+ }
+
+ Map<String, Object> result = new java.util.HashMap<>(8);
+ result.put("hasPromqlExecutor", hasPromqlExecutor);
+ result.put("hasSqlExecutor", hasSqlExecutor);
+ result.put("availableExecutors", availableExecutors);
+
+ return result;
+ }
Review Comment:
The new method `getAvailableExecutors()` in DataSourceServiceImpl lacks test
coverage. Since test coverage exists for DataSourceServiceImpl in
DataSourceServiceTest.java, this new functionality should also be tested to
ensure it correctly identifies available executors and handles edge cases
(e.g., null executors, executors that don't support certain query languages).
##########
web-app/src/app/routes/alert/alert-setting/alert-setting.component.ts:
##########
@@ -321,7 +330,40 @@ export class AlertSettingComponent implements OnInit {
this.isSelectTypeModalVisible = true;
}
+ loadDatasourceStatus() {
+ this.alertDefineSvc.getDatasourceStatus().subscribe({
+ next: res => {
+ if (res.code === 0 && res.data) {
+ this.datasourceStatus = {
+ hasPromqlExecutor: res.data.hasPromqlExecutor || false,
+ hasSqlExecutor: res.data.hasSqlExecutor || false,
+ loaded: true
+ };
+ // Check if periodic alert is enabled (requires at least one
executor)
+ this.isPeriodicAlertEnabled =
this.datasourceStatus.hasPromqlExecutor || this.datasourceStatus.hasSqlExecutor;
+ }
+ },
+ error: err => {
+ console.warn('Failed to load datasource status:', err);
+ // If failed to load, still enable periodic alerts
+ this.isPeriodicAlertEnabled = true;
+ this.datasourceStatus.loaded = true;
+ }
+ });
+ }
+
+ onDatasourceChange() {
+ // Update placeholder when datasource changes
+ // Placeholder is already set in template with dynamic binding
Review Comment:
The `onDatasourceChange()` method at lines 355-358 is empty with only a
comment explaining that placeholder updates are handled in the template. If
this method serves no functional purpose, it should be removed to reduce code
clutter. If it's a placeholder for future functionality, add a TODO comment to
clarify the intent.
```suggestion
// TODO: Add any additional logic needed when the datasource changes.
// Currently, placeholder text is updated via template dynamic binding.
```
##########
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/controller/AlertDefineController.java:
##########
@@ -112,4 +116,12 @@ public ResponseEntity<Message<List<Map<String, Object>>>>
getDefinePreview(
}
}
+ @GetMapping(path = "/datasource/status")
+ @Operation(summary = "Get available datasource executors status",
+ description = "Get status of available datasource executors for
periodic alerts")
+ public ResponseEntity<Message<Map<String, Object>>> getDatasourceStatus() {
+ Map<String, Object> status = dataSourceService.getAvailableExecutors();
+ return ResponseEntity.ok(Message.successWithData(status));
+ }
Review Comment:
The new endpoint `/datasource/status` in AlertDefineController lacks test
coverage. Since test coverage exists for AlertDefineController in
AlertDefineControllerTest.java, this new endpoint should also be tested to
ensure it correctly returns datasource executor status and handles service
interactions properly.
##########
web-app/src/assets/i18n/en-US.json:
##########
@@ -362,6 +362,20 @@
"alert.setting.type.realtime.desc": "Calculate data in real-time with
instant alert on threshold breach",
"alert.setting.type.realtime.log": "Log RealTime",
"alert.setting.type.realtime.metric": "Metric RealTime",
+ "alert.setting.query.language": "Query Language",
+ "alert.setting.query.language.promql": "PromQL",
+ "alert.setting.query.language.sql": "SQL",
+ "alert.setting.query.language.sql.not.support.metric": "SQL queries are not
supported for periodic metric alerts yet",
+ "alert.setting.query.language.promql.not.support.log": "PromQL queries are
not supported for periodic log alerts yet",
+ "alert.setting.promql.description": "PromQL Query
Language<br/><br/>Examples:<br/>• <code>cpu_usage > 80</code><br/>•
<code>cpu_usage{instance='server1'} > 80</code><br/>•
<code>rate(http_requests_total[5m]) > 100</code><br/>• <code>cpu > 80 and
memory > 70</code>",
+ "alert.setting.sql.description": "SQL Query
Language<br/><br/>Examples:<br/>• <code>SELECT COUNT(*) FROM hertzbeat_logs
WHERE severity_text = 'ERROR'</code><br/>• <code>SELECT service_name, COUNT(*)
FROM hertzbeat_logs GROUP BY service_name</code><br/>• <code>SELECT * FROM
hertzbeat_logs WHERE time_unix_nano >= NOW() - INTERVAL '5 minute'</code>",
+ "alert.setting.type.realtime": "Real-time Alert",
+ "alert.setting.type.periodic": "Periodic Alert",
+ "alert.setting.type.realtime.desc": "Trigger alerts immediately upon data
collection",
+ "alert.setting.type.periodic.desc": "Execute PromQL/SQL queries periodically
to check alert conditions",
Review Comment:
Duplicate translation keys detected. Lines 357-358 and 373 both define
"alert.setting.type.periodic", and lines 361-362 and 372 both define
"alert.setting.type.realtime". The second definitions (lines 372-373) override
the first ones, which may cause unintended behavior. The descriptions at lines
362 and 374, as well as lines 358 and 375, also differ. Review and consolidate
these duplicate keys to ensure only one definition exists for each key.
##########
web-app/src/app/routes/alert/alert-setting/alert-setting.component.html:
##########
@@ -717,16 +736,35 @@
</nz-form-item>
<nz-form-item *ngIf="define.type == 'periodic_log'" [ngStyle]="{
marginBottom: '5px' }">
<nz-form-label [nzSpan]="7" nzRequired="true" nzFor="periodic_expr"
[nzTooltipTitle]="'alert.setting.log.query.tip' | i18n">
- {{ 'alert.setting.log.query' | i18n }}
+ {{ 'alert.setting.query.language' | i18n }}
</nz-form-label>
<nz-form-control [nzSpan]="12" [nzErrorTip]="'validation.required' |
i18n">
- <ng-container *ngIf="cascadeValues[1] !== 'availability'">
- <nz-radio-group [(ngModel)]="define.datasource"
nzButtonStyle="solid" name="datasource" id="datasource">
- <label nz-radio-button [nzValue]="'sql'">
- {{ 'SQL' | i18n }}
- </label>
- </nz-radio-group>
- </ng-container>
+ <nz-radio-group
+ [(ngModel)]="define.datasource"
+ (ngModelChange)="onDatasourceChange()"
+ nzButtonStyle="solid"
+ name="datasource"
+ id="datasource"
+ >
+ <label
+ nz-radio-button
+ [nzValue]="'promql'"
+ [nzDisabled]="true"
+ nz-tooltip
+
[nzTooltipTitle]="'alert.setting.query.language.promql.not.support.log' | i18n"
+ >
+ {{ 'alert.setting.query.language.promql' | i18n }}
+ </label>
+ <label nz-radio-button [nzValue]="'sql'" [nzDisabled]="false">
+ {{ 'alert.setting.query.language.sql' | i18n }}
+ </label>
+ </nz-radio-group>
+ <div class="datasource-tip" *ngIf="define.datasource === 'promql'">
+ <small [innerHTML]="'alert.setting.promql.description' |
i18n"></small>
+ </div>
+ <div class="datasource-tip" *ngIf="define.datasource === 'sql'">
+ <small [innerHTML]="'alert.setting.sql.description' |
i18n"></small>
Review Comment:
The use of `[innerHTML]` binding with i18n strings at lines 763 and 766
could pose a security risk if the translation strings contain untrusted
content. While the current strings appear to be static HTML examples, ensure
that these translation strings are properly sanitized and controlled. Consider
using Angular's DomSanitizer if dynamic content is ever introduced, or use
safer alternatives like structural directives for rendering formatted content.
```suggestion
<small>{{ 'alert.setting.promql.description' | i18n }}</small>
</div>
<div class="datasource-tip" *ngIf="define.datasource === 'sql'">
<small>{{ 'alert.setting.sql.description' | i18n }}</small>
```
##########
web-app/src/app/routes/alert/alert-setting/alert-setting.component.html:
##########
@@ -656,16 +656,35 @@
</nz-form-item>
<nz-form-item *ngIf="define.type == 'periodic_metric'" [ngStyle]="{
marginBottom: '5px' }">
<nz-form-label [nzSpan]="7" nzFor="datasource" nzRequired="true"
[nzTooltipTitle]="'alert.setting.rule.label' | i18n">
- {{ 'alert.setting.rule' | i18n }}
+ {{ 'alert.setting.query.language' | i18n }}
</nz-form-label>
<nz-form-control [nzSpan]="12" [nzErrorTip]="'validation.required' |
i18n">
- <ng-container *ngIf="cascadeValues[1] !== 'availability'">
- <nz-radio-group [(ngModel)]="define.datasource"
nzButtonStyle="solid" name="datasource" id="datasource">
- <label nz-radio-button [nzValue]="'promql'">
- {{ 'PromQL' | i18n }}
- </label>
- </nz-radio-group>
- </ng-container>
+ <nz-radio-group
+ [(ngModel)]="define.datasource"
+ (ngModelChange)="onDatasourceChange()"
+ nzButtonStyle="solid"
+ name="datasource"
+ id="datasource"
+ >
+ <label nz-radio-button [nzValue]="'promql'" [nzDisabled]="false">
+ {{ 'alert.setting.query.language.promql' | i18n }}
+ </label>
+ <label
+ nz-radio-button
+ [nzValue]="'sql'"
+ [nzDisabled]="true"
+ nz-tooltip
+
[nzTooltipTitle]="'alert.setting.query.language.sql.not.support.metric' | i18n"
+ >
+ {{ 'alert.setting.query.language.sql' | i18n }}
+ </label>
+ </nz-radio-group>
+ <div class="datasource-tip" *ngIf="define.datasource === 'promql'">
+ <small [innerHTML]="'alert.setting.promql.description' |
i18n"></small>
+ </div>
+ <div class="datasource-tip" *ngIf="define.datasource === 'sql'">
+ <small [innerHTML]="'alert.setting.sql.description' |
i18n"></small>
Review Comment:
The use of `[innerHTML]` binding with i18n strings at lines 683 and 686
could pose a security risk if the translation strings contain untrusted
content. While the current strings appear to be static HTML examples, ensure
that these translation strings are properly sanitized and controlled. Consider
using Angular's DomSanitizer if dynamic content is ever introduced, or use
safer alternatives like structural directives for rendering formatted content.
```suggestion
<small>{{ 'alert.setting.promql.description' | i18n }}</small>
</div>
<div class="datasource-tip" *ngIf="define.datasource === 'sql'">
<small>{{ 'alert.setting.sql.description' | i18n }}</small>
```
##########
web-app/src/app/routes/alert/alert-setting/alert-setting.component.ts:
##########
@@ -1460,7 +1505,12 @@ export class AlertSettingComponent implements OnInit {
}
}
- this.userExpr = `${before} ${insertText} ${after}`;
+ // Update the appropriate expression variable
+ if (this.define.type === 'realtime_log') {
+ this.userExpr = `${before} ${insertText} ${after}`;
+ } else {
+ this.userExpr = `${before} ${insertText} ${after}`;
+ }
Review Comment:
The conditional logic at lines 1509-1513 is redundant. Both branches of the
if-else statement execute the same assignment: `this.userExpr = '${before}
${insertText} ${after}'`. This suggests either the condition is unnecessary, or
one of the branches should update a different variable. If the logic is
intentionally the same for both cases, the conditional should be removed for
clarity and maintainability.
```suggestion
// Update the expression variable
this.userExpr = `${before} ${insertText} ${after}`;
```
##########
web-app/src/assets/i18n/zh-CN.json:
##########
@@ -365,6 +365,20 @@
"alert.setting.window": "计算窗口",
"alert.setting.window.tip": "实时阈值计算窗口时间，单位秒",
"alert.setting.window.placeholder": "请输入计算窗口时间",
+ "alert.setting.query.language": "查询语言",
+ "alert.setting.query.language.promql": "PromQL",
+ "alert.setting.query.language.sql": "SQL",
+ "alert.setting.query.language.sql.not.support.metric": "周期性监控指标暂不支持 SQL 查询",
+ "alert.setting.query.language.promql.not.support.log": "周期性日志暂不支持 PromQL 查询",
+ "alert.setting.promql.description": "PromQL 查询语言<br/><br/>示例：<br/>•
<code>cpu_usage > 80</code><br/>• <code>cpu_usage{instance='server1'} >
80</code><br/>• <code>rate(http_requests_total[5m]) > 100</code><br/>•
<code>cpu > 80 and memory > 70</code>",
+ "alert.setting.sql.description": "SQL 查询语言<br/><br/>示例：<br/>• <code>SELECT
COUNT(*) FROM hertzbeat_logs WHERE severity_text = 'ERROR'</code><br/>•
<code>SELECT service_name, COUNT(*) FROM hertzbeat_logs GROUP BY
service_name</code><br/>• <code>SELECT * FROM hertzbeat_logs WHERE
time_unix_nano >= NOW() - INTERVAL '5 minute'</code>",
+ "alert.setting.type.realtime": "实时告警",
+ "alert.setting.type.periodic": "周期告警",
+ "alert.setting.type.realtime.desc": "实时监控数据采集即触发告警",
+ "alert.setting.type.periodic.desc": "按周期执行 PromQL/SQL 查询判断告警",
Review Comment:
Duplicate translation keys detected. Lines 357-358 and 376 both define
"alert.setting.type.periodic", and lines 361-362 and 375 both define
"alert.setting.type.realtime". The second definitions (lines 375-376) override
the first ones, which may cause unintended behavior. The descriptions at lines
362 and 378, as well as 377, also differ. Review and consolidate these
duplicate keys to ensure only one definition exists for each key.
##########
web-app/src/app/routes/alert/alert-setting/alert-setting.component.ts:
##########
@@ -321,7 +330,40 @@ export class AlertSettingComponent implements OnInit {
this.isSelectTypeModalVisible = true;
}
+ loadDatasourceStatus() {
+ this.alertDefineSvc.getDatasourceStatus().subscribe({
+ next: res => {
+ if (res.code === 0 && res.data) {
+ this.datasourceStatus = {
+ hasPromqlExecutor: res.data.hasPromqlExecutor || false,
+ hasSqlExecutor: res.data.hasSqlExecutor || false,
+ loaded: true
+ };
+ // Check if periodic alert is enabled (requires at least one
executor)
+ this.isPeriodicAlertEnabled =
this.datasourceStatus.hasPromqlExecutor || this.datasourceStatus.hasSqlExecutor;
+ }
+ },
+ error: err => {
+ console.warn('Failed to load datasource status:', err);
+ // If failed to load, still enable periodic alerts
+ this.isPeriodicAlertEnabled = true;
+ this.datasourceStatus.loaded = true;
Review Comment:
The error handling fallback at lines 348-349 sets `isPeriodicAlertEnabled =
true` when the datasource status API fails. This could lead to a poor user
experience if the backend is unavailable - users might attempt to create
periodic alerts that will fail. Consider defaulting to `false` on error, or
showing a specific warning message that the status couldn't be determined.
```suggestion
// If failed to load, disable periodic alerts and use a safe
fallback status
this.isPeriodicAlertEnabled = false;
this.datasourceStatus = {
hasPromqlExecutor: false,
hasSqlExecutor: false,
loaded: true
};
this.notifySvc.warning('Failed to load datasource status. Periodic
alerts are disabled.', '');
```
--
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]
