Duansg commented on code in PR #3994:
URL: https://github.com/apache/hertzbeat/pull/3994#discussion_r2712813165
##########
web-app/src/app/routes/monitor/monitor-data-chart/monitor-data-chart.component.ts:
##########
@@ -264,7 +266,7 @@ export class MonitorDataChartComponent implements OnInit,
OnDestroy {
// load historical metrics data
this.loading =
`${this.i18nSvc.fanyi('monitor.detail.chart.data-loading')}`;
let metricData$ = this.monitorSvc
- .getMonitorMetricHistoryData(this.instance, this.app, this.metrics,
this.metric, this.timePeriod, isInterval)
+ .getMonitorMetricHistoryData(this.instance, this.monitorName, this.app,
this.metrics, this.metric, this.timePeriod, isInterval)
Review Comment:
Can we just directly convert the app here? That way, we won't need to adjust
the REST API.
##########
hertzbeat-ai/src/main/java/org/apache/hertzbeat/ai/tools/impl/MetricsToolsImpl.java:
##########
@@ -151,13 +151,14 @@ public String getRealtimeMetrics(
Ask user to provide the filters for labels, history and interval
aggregation
""")
public String getHistoricalMetrics(
- @ToolParam(description = "Instance identifier (e.g., 'ip:port',
'ip', or 'domain')") String instance,
- @ToolParam(description = "Monitor type (e.g., 'linux', 'mysql',
'http')", required = true) String app,
- @ToolParam(description = "Metrics name (e.g., 'target', 'cpu',
'memory')", required = true) String metrics,
- @ToolParam(description = "Field Parameter (e.g., 'usage', 'used',
'available')", required = false) String fieldParameter,
- @ToolParam(description = "Label filter for specific instances",
required = false) String label,
- @ToolParam(description = "Time range (e.g., '1h', '6h', '24h',
'7d')", required = false) String history,
- @ToolParam(description = "Whether to aggregate data with
intervals", required = false) Boolean interval) {
+ @ToolParam(description = "Instance identifier (e.g., 'ip:port', 'ip',
or 'domain')") String instance,
+ @ToolParam(description = "Monitor name for querying historical
metrics") String monitorName,
Review Comment:
Should monitorName be exposed to the Tool? I believe this is an unstable
parameter and doesn't need to be exposed externally. It should remain within
the internal normalization layer. I recommend performing app conversion
internally.
--
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]
