Skip to content

[refactor] Add monitorName parameter to history data query for consistent app handling#3994

Closed
Saramanda9988 wants to merge 4 commits intoapache:masterfrom
Saramanda9988:issue-3983
Closed

[refactor] Add monitorName parameter to history data query for consistent app handling#3994
Saramanda9988 wants to merge 4 commits intoapache:masterfrom
Saramanda9988:issue-3983

Conversation

@Saramanda9988
Copy link
Copy Markdown
Contributor

@Saramanda9988 Saramanda9988 commented Jan 20, 2026

What's changed?

please refer to #3983

In the method of querying historical data, add the monitorName parameter. When the app is prometheus, automatically concatenate the prefix prometheus{monitorName} to ensure that the query and storage use the same app value

  • HistoryDataReader - add the monitorName parameter to the interface method (marked @ Deprecated)
  • MetricsDataController - API path changed from/api/monitor/{instance}/metric/{metricFull} to/ -api/monitor/{instance}/metric/{monitorName}/{metricFull}
  • MetricsDataService - add the monitorName parameter to the service method

IotDb

a00ea231bfb24cfde6cd24f5306229c1

Influxdb

5c84338dd3c5ff522b639242114431a4

QuestDb

c9b31a715e736a313bb88396ba779e93

TDengine

870f4df499ee3de29d6d2efb3f1f86ad

Checklist

  • I have read the Contributing Guide
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

Add or update API

  • I have added the necessary e2e tests and all cases have passed.

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)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just directly convert the app here? That way, we won't need to adjust the REST API.

Copy link
Copy Markdown
Contributor Author

@Saramanda9988 Saramanda9988 Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi,thanks for your cr.
Do you mean we should modify the app parameter directly in the fe? Maybe that would require adjusting greptimedb and vm to align with other tsdbs.
Just concerned this adds complexity to these two storages that we'll support long-term. What do you think?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your reply.

  1. VictoriaMetricsClusterDataStorage And VictoriaMetricsDataStorage
// saveData() method
if (metricsData.getApp().startsWith(CommonConstants.PROMETHEUS_APP_PREFIX)) {
    isPrometheusAuto = true;
}
// getHistoryMetricData()
String labelName = metrics + SPILT + metric;
if (CommonConstants.PROMETHEUS.equals(app)) { // app.startsWith(CommonConstants.PROMETHEUS_APP_PREFIX)
    labelName = metrics;
}
String timeSeriesSelector = Stream.of(
    LABEL_KEY_NAME + "=\"" + labelName + "\"",
    LABEL_KEY_INSTANCE + "=\"" + instance + "\"",
    CommonConstants.PROMETHEUS.equals(app) ? null : MONITOR_METRIC_KEY + "=\"" + metric + "\"" // app.startsWith(CommonConstants.PROMETHEUS_APP_PREFIX)
).filter(Objects::nonNull).collect(Collectors.joining(","));
  1. GreptimeDbDataStorage: getHistoryMetricData() and getHistoryIntervalMetricData()
if (!CommonConstants.PROMETHEUS.equals(app)) { // default :  String timeSeriesSelector = LABEL_KEY_NAME + "=\"" + name + "\"" + "," + LABEL_KEY_INSTANCE + "=\"" + instance + "\"";
    timeSeriesSelector = timeSeriesSelector + "," + LABEL_KEY_FIELD + "=\"" + metric + "\"";
}

Theoretically, only the VM needs to be modified, right? And since the VM is saved through its mapping, maintaining consistency shouldn't be an issue. What do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Duansg , hi thanks for your reply

Yes, you're right. Let's do it this way.
I'll close this PR and submit a new one following your suggestion; it will be much simpler.

@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,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants