Copilot commented on code in PR #3776:
URL: https://github.com/apache/hertzbeat/pull/3776#discussion_r2379395502
##########
hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/store/history/tsdb/greptime/GreptimeDbDataStorage.java:
##########
@@ -202,60 +208,148 @@ public void saveData(CollectRep.MetricsData metricsData)
{
@Override
public Map<String, List<Value>> getHistoryMetricData(Long monitorId,
String app, String metrics, String metric,
String label, String
history) {
+ Map<String, Long> timeRange = getTimeRange(history);
+ Long start = timeRange.get(LABEL_KEY_START_TIME);
+ Long end = timeRange.get(LABEL_KEY_END_TIME);
+
+ String step = getTimeStep(start, end);
+
+ return getHistoryData(start, end, step, monitorId, app, metrics,
metric);
+ }
+
+ private String getTableName(String metrics) {
+ return metrics;
+ }
+
+ @Override
+ public Map<String, List<Value>> getHistoryIntervalMetricData(Long
monitorId, String app, String metrics,
+ String
metric, String label, String history) {
+ Map<String, Long> timeRange = getTimeRange(history);
+ Long start = timeRange.get(LABEL_KEY_START_TIME);
+ Long end = timeRange.get(LABEL_KEY_END_TIME);
+
+ String step = getTimeStep(start, end);
+
+ Map<String, List<Value>> instanceValuesMap = getHistoryData(start,
end, step, monitorId, app, metrics, metric);
+
+ // Queries below this point may yield inconsistent results due to
exceeding the valid data range.
+ // Therefore, we restrict the valid range by obtaining the post-query
timeframe.
+ // Since `gretime`'s `end` excludes the specified time, we add 4 hours.
+ List<Value> values =
instanceValuesMap.get(instanceValuesMap.keySet().stream().toList().get(0));
Review Comment:
This code will throw an IndexOutOfBoundsException if instanceValuesMap is
empty. Add a null check and ensure the map contains at least one entry before
accessing the first key.
##########
hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/store/history/tsdb/greptime/GreptimeDbDataStorage.java:
##########
@@ -202,60 +208,148 @@ public void saveData(CollectRep.MetricsData metricsData)
{
@Override
public Map<String, List<Value>> getHistoryMetricData(Long monitorId,
String app, String metrics, String metric,
String label, String
history) {
+ Map<String, Long> timeRange = getTimeRange(history);
+ Long start = timeRange.get(LABEL_KEY_START_TIME);
+ Long end = timeRange.get(LABEL_KEY_END_TIME);
+
+ String step = getTimeStep(start, end);
+
+ return getHistoryData(start, end, step, monitorId, app, metrics,
metric);
+ }
+
+ private String getTableName(String metrics) {
+ return metrics;
+ }
+
+ @Override
+ public Map<String, List<Value>> getHistoryIntervalMetricData(Long
monitorId, String app, String metrics,
+ String
metric, String label, String history) {
+ Map<String, Long> timeRange = getTimeRange(history);
+ Long start = timeRange.get(LABEL_KEY_START_TIME);
+ Long end = timeRange.get(LABEL_KEY_END_TIME);
+
+ String step = getTimeStep(start, end);
+
+ Map<String, List<Value>> instanceValuesMap = getHistoryData(start,
end, step, monitorId, app, metrics, metric);
+
+ // Queries below this point may yield inconsistent results due to
exceeding the valid data range.
+ // Therefore, we restrict the valid range by obtaining the post-query
timeframe.
+ // Since `gretime`'s `end` excludes the specified time, we add 4 hours.
+ List<Value> values =
instanceValuesMap.get(instanceValuesMap.keySet().stream().toList().get(0));
+ // effective time
+ long effectiveStart = values.get(0).getTime() / 1000;
+ long effectiveEnd = values.get(values.size() - 1).getTime() / 1000 +
Duration.ofHours(4).getSeconds();
+
+ String name = getTableName(metrics);
+ String timeSeriesSelector = name + "{" + LABEL_KEY_INSTANCE + "=\"" +
monitorId + "\"";
+ if (!CommonConstants.PROMETHEUS.equals(app)) {
+ timeSeriesSelector = timeSeriesSelector + "," + LABEL_KEY_FIELD +
"=\"" + metric + "\"";
+ }
+ timeSeriesSelector = timeSeriesSelector + "}";
+
+ try {
+ // max
+ String finalTimeSeriesSelector = timeSeriesSelector;
+ URI uri = getUri(effectiveStart, effectiveEnd, step, uriComponents
-> "max_over_time(" + finalTimeSeriesSelector + "[" + step + "])");
+ requestIntervalMetricAndPutValue(uri, instanceValuesMap,
Value::setMax);
+ // min
+ uri = getUri(effectiveStart, effectiveEnd, step, uriComponents ->
"min_over_time(" + finalTimeSeriesSelector + "[" + step + "])");
+ requestIntervalMetricAndPutValue(uri, instanceValuesMap,
Value::setMin);
+ // avg
+ uri = getUri(effectiveStart, effectiveEnd, step, uriComponents ->
"avg_over_time(" + finalTimeSeriesSelector + "[" + step + "])");
+ requestIntervalMetricAndPutValue(uri, instanceValuesMap,
Value::setMean);
+ } catch (Exception e) {
+ log.error("query interval metrics data from greptime error. {}",
e.getMessage(), e);
+ }
+
+ return instanceValuesMap;
+ }
+
+ /**
+ * Get time range
+ *
+ * @param history history range
+ * @return time range
+ */
+ private Map<String, Long> getTimeRange(String history) {
+ // Build start and end times
+ Instant now = Instant.now();
+ long start;
+ try {
+ if (NumberUtils.isParsable(history)) {
+ start = NumberUtils.toLong(history);
+ start = (ZonedDateTime.now().toEpochSecond() - start);
+ } else {
+ TemporalAmount temporalAmount =
TimePeriodUtil.parseTokenTime(history);
+ assert temporalAmount != null;
+ Instant dateTime = now.minus(temporalAmount);
+ start = dateTime.getEpochSecond();
+ }
+ } catch (Exception e) {
+ log.error("history time error: {}. use default: 6h",
e.getMessage());
+ start = now.minus(6, ChronoUnit.HOURS).getEpochSecond();
+ }
+ long end = now.getEpochSecond();
+ return Map.of("start", start, "end", end);
Review Comment:
Using hardcoded string literals 'start' and 'end' here is inconsistent with
the defined constants LABEL_KEY_START_TIME and LABEL_KEY_END_TIME. Use the
constants for consistency.
```suggestion
return Map.of(MetricDataConstants.LABEL_KEY_START_TIME, start,
MetricDataConstants.LABEL_KEY_END_TIME, end);
```
##########
hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/store/history/tsdb/greptime/GreptimeDbDataStorage.java:
##########
@@ -202,60 +208,148 @@ public void saveData(CollectRep.MetricsData metricsData)
{
@Override
public Map<String, List<Value>> getHistoryMetricData(Long monitorId,
String app, String metrics, String metric,
String label, String
history) {
+ Map<String, Long> timeRange = getTimeRange(history);
+ Long start = timeRange.get(LABEL_KEY_START_TIME);
+ Long end = timeRange.get(LABEL_KEY_END_TIME);
+
+ String step = getTimeStep(start, end);
+
+ return getHistoryData(start, end, step, monitorId, app, metrics,
metric);
+ }
+
+ private String getTableName(String metrics) {
+ return metrics;
+ }
+
+ @Override
+ public Map<String, List<Value>> getHistoryIntervalMetricData(Long
monitorId, String app, String metrics,
+ String
metric, String label, String history) {
+ Map<String, Long> timeRange = getTimeRange(history);
+ Long start = timeRange.get(LABEL_KEY_START_TIME);
+ Long end = timeRange.get(LABEL_KEY_END_TIME);
+
+ String step = getTimeStep(start, end);
+
+ Map<String, List<Value>> instanceValuesMap = getHistoryData(start,
end, step, monitorId, app, metrics, metric);
+
+ // Queries below this point may yield inconsistent results due to
exceeding the valid data range.
+ // Therefore, we restrict the valid range by obtaining the post-query
timeframe.
+ // Since `gretime`'s `end` excludes the specified time, we add 4 hours.
+ List<Value> values =
instanceValuesMap.get(instanceValuesMap.keySet().stream().toList().get(0));
+ // effective time
+ long effectiveStart = values.get(0).getTime() / 1000;
+ long effectiveEnd = values.get(values.size() - 1).getTime() / 1000 +
Duration.ofHours(4).getSeconds();
+
+ String name = getTableName(metrics);
+ String timeSeriesSelector = name + "{" + LABEL_KEY_INSTANCE + "=\"" +
monitorId + "\"";
+ if (!CommonConstants.PROMETHEUS.equals(app)) {
+ timeSeriesSelector = timeSeriesSelector + "," + LABEL_KEY_FIELD +
"=\"" + metric + "\"";
+ }
+ timeSeriesSelector = timeSeriesSelector + "}";
+
+ try {
+ // max
+ String finalTimeSeriesSelector = timeSeriesSelector;
+ URI uri = getUri(effectiveStart, effectiveEnd, step, uriComponents
-> "max_over_time(" + finalTimeSeriesSelector + "[" + step + "])");
+ requestIntervalMetricAndPutValue(uri, instanceValuesMap,
Value::setMax);
+ // min
+ uri = getUri(effectiveStart, effectiveEnd, step, uriComponents ->
"min_over_time(" + finalTimeSeriesSelector + "[" + step + "])");
+ requestIntervalMetricAndPutValue(uri, instanceValuesMap,
Value::setMin);
+ // avg
+ uri = getUri(effectiveStart, effectiveEnd, step, uriComponents ->
"avg_over_time(" + finalTimeSeriesSelector + "[" + step + "])");
+ requestIntervalMetricAndPutValue(uri, instanceValuesMap,
Value::setMean);
+ } catch (Exception e) {
+ log.error("query interval metrics data from greptime error. {}",
e.getMessage(), e);
+ }
+
+ return instanceValuesMap;
+ }
+
+ /**
+ * Get time range
+ *
+ * @param history history range
+ * @return time range
+ */
+ private Map<String, Long> getTimeRange(String history) {
+ // Build start and end times
+ Instant now = Instant.now();
+ long start;
+ try {
+ if (NumberUtils.isParsable(history)) {
+ start = NumberUtils.toLong(history);
+ start = (ZonedDateTime.now().toEpochSecond() - start);
+ } else {
+ TemporalAmount temporalAmount =
TimePeriodUtil.parseTokenTime(history);
+ assert temporalAmount != null;
+ Instant dateTime = now.minus(temporalAmount);
+ start = dateTime.getEpochSecond();
+ }
+ } catch (Exception e) {
+ log.error("history time error: {}. use default: 6h",
e.getMessage());
+ start = now.minus(6, ChronoUnit.HOURS).getEpochSecond();
+ }
+ long end = now.getEpochSecond();
+ return Map.of("start", start, "end", end);
+ }
+
+ /**
+ * Get time step
+ *
+ * @param start start time
+ * @param end end time
+ * @return step
+ */
+ private String getTimeStep(long start, long end) {
+ // get step
+ String step = "60s";
+ if (end - start < Duration.ofDays(7).getSeconds() && end - start >
Duration.ofDays(1).getSeconds()) {
+ step = "1h";
+ } else if (end - start >= Duration.ofDays(7).getSeconds()) {
+ step = "4h";
+ }
+ return step;
+ }
+
+ /**
+ * Get history metric data
+ *
+ * @param start start time
+ * @param end end time
+ * @param step step
+ * @param monitorId monitor id
+ * @param app monitor type
+ * @param metrics metrics
+ * @param metric metric
+ * @return history metric data
+ */
+ private Map<String, List<Value>> getHistoryData(long start, long end,
String step, Long monitorId, String app, String metrics, String metric) {
String name = getTableName(metrics);
String timeSeriesSelector = LABEL_KEY_NAME + "=\"" + name + "\""
+ "," + LABEL_KEY_INSTANCE + "=\"" + monitorId + "\"";
if (!CommonConstants.PROMETHEUS.equals(app)) {
timeSeriesSelector = timeSeriesSelector + "," + LABEL_KEY_FIELD +
"=\"" + metric + "\"";
}
+
Map<String, List<Value>> instanceValuesMap = new HashMap<>(8);
try {
- HttpHeaders headers = new HttpHeaders();
- headers.setContentType(MediaType.APPLICATION_JSON);
- headers.setAccept(List.of(MediaType.APPLICATION_JSON));
- if (StringUtils.hasText(greptimeProperties.username())
- && StringUtils.hasText(greptimeProperties.password())) {
- String authStr = greptimeProperties.username() + ":" +
greptimeProperties.password();
- String encodedAuth = Base64Util.encode(authStr);
- headers.add(HttpHeaders.AUTHORIZATION, BASIC + " " +
encodedAuth);
- }
- Instant now = Instant.now();
- long start;
- try {
- if (NumberUtils.isParsable(history)) {
- start = NumberUtils.toLong(history);
- start = (ZonedDateTime.now().toEpochSecond() - start);
- } else {
- TemporalAmount temporalAmount =
TimePeriodUtil.parseTokenTime(history);
- assert temporalAmount != null;
- Instant dateTime = now.minus(temporalAmount);
- start = dateTime.getEpochSecond();
+ HttpEntity<Void> httpEntity = getHttpEntity();
+
+ String finalTimeSeriesSelector = timeSeriesSelector;
+ URI uri = getUri(start, end, step, uriComponents -> {
+ MultiValueMap<String, String> queryParams =
uriComponents.getQueryParams();
+ if (!queryParams.isEmpty()) {
+ return "{" + finalTimeSeriesSelector + "}";
}
- } catch (Exception e) {
- log.error("history time error: {}. use default: 6h",
e.getMessage());
- start = now.minus(6, ChronoUnit.HOURS).getEpochSecond();
- }
+ return null;
+ });
- long end = now.getEpochSecond();
- String step = "60s";
- if (end - start < Duration.ofDays(7).getSeconds() && end - start >
Duration.ofDays(1).getSeconds()) {
- step = "1h";
- } else if (end - start >= Duration.ofDays(7).getSeconds()) {
- step = "4h";
+ ResponseEntity<PromQlQueryContent> responseEntity = null;
+ if (uri != null) {
+ responseEntity = restTemplate.exchange(uri,
+ HttpMethod.GET, httpEntity, PromQlQueryContent.class);
}
Review Comment:
The null initialization and subsequent null check is unnecessary. Consider
returning early from getHistoryData when uri is null, or restructure to avoid
the null assignment.
--
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]
