-
-
Notifications
You must be signed in to change notification settings - Fork 342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feature] Add Support for Historical Data Points #986
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,6 +68,11 @@ func runDiscoveryJob( | |
getMetricDataOutput := make([][]*cloudwatch.MetricDataResult, partition) | ||
count := 0 | ||
|
||
var addHistoricalMetrics bool | ||
if job.AddHistoricalMetrics != nil { | ||
addHistoricalMetrics = *job.AddHistoricalMetrics | ||
} | ||
|
||
for i := 0; i < metricDataLength; i += maxMetricCount { | ||
go func(i, n int) { | ||
defer wg.Done() | ||
|
@@ -76,7 +81,7 @@ func runDiscoveryJob( | |
end = metricDataLength | ||
} | ||
input := getMetricDatas[i:end] | ||
data := clientCloudwatch.GetMetricData(ctx, logger, input, svc.Namespace, length, job.Delay, job.RoundingPeriod) | ||
data := clientCloudwatch.GetMetricData(ctx, logger, input, svc.Namespace, length, job.Delay, job.RoundingPeriod, addHistoricalMetrics) | ||
if data != nil { | ||
getMetricDataOutput[n] = data | ||
} else { | ||
|
@@ -98,15 +103,34 @@ func runDiscoveryJob( | |
if data == nil { | ||
continue | ||
} | ||
previousIdx := -1 | ||
previousID := "" | ||
for _, metricDataResult := range data { | ||
idx := findGetMetricDataByID(getMetricDatas, *metricDataResult.ID) | ||
// TODO: This logic needs to be guarded by a feature flag! Also, remember to add compatibility in the client v2 | ||
if idx == -1 { | ||
logger.Warn("GetMetricData returned unknown metric ID", "metric_id", *metricDataResult.ID) | ||
if addHistoricalMetrics { | ||
// Use the previousIdx to make a copy | ||
if previousIdx != -1 && previousID == *metricDataResult.ID { | ||
// Create a new CloudwatchData object | ||
newData := *getMetricDatas[previousIdx] | ||
newData.GetMetricDataPoint = metricDataResult.Datapoint | ||
newData.GetMetricDataTimestamps = metricDataResult.Timestamp | ||
|
||
getMetricDatas = append(getMetricDatas, &newData) | ||
} else { | ||
logger.Warn("GetMetricData returned unknown metric ID", "metric_id", *metricDataResult.ID) | ||
} | ||
} else { | ||
logger.Warn("GetMetricData returned unknown metric ID", "metric_id", *metricDataResult.ID) | ||
} | ||
continue | ||
} | ||
getMetricDatas[idx].GetMetricDataPoint = metricDataResult.Datapoint | ||
getMetricDatas[idx].GetMetricDataTimestamps = metricDataResult.Timestamp | ||
getMetricDatas[idx].MetricID = nil // mark as processed | ||
previousIdx = idx | ||
previousID = *metricDataResult.ID | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the first attempt but I'm not keen on using the "order" in which the data is retrieved to achieve this. I'm planning to go over this section again and use a hashmap/dictionary to make this logic independent from the order the getMetricDatas has (I believe timestamp descending if I recall correctly) |
||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -233,7 +233,9 @@ func EnsureLabelConsistencyAndRemoveDuplicates(metrics []*PrometheusMetric, obse | |
} | ||
} | ||
|
||
metricKey := fmt.Sprintf("%s-%d", *metric.Name, prom_model.LabelsToSignature(metric.Labels)) | ||
// Include the timestamp to avoid genuine duplicates!? At this point we have all the metrics to be exposed under the `/metrics` endpoint so | ||
// we aren't able to tell if some of the metrics are present because the `addHistoricalMetrics` is set to `true`!? | ||
metricKey := fmt.Sprintf("%s-%d-%d", *metric.Name, prom_model.LabelsToSignature(metric.Labels), metric.Timestamp.Unix()) | ||
Comment on lines
+253
to
+255
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it safe to include the timestamp to remove "real" duplicates? |
||
if _, exists := metricKeys[metricKey]; !exists { | ||
metricKeys[metricKey] = struct{}{} | ||
output = append(output, metric) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new flag to allow this behaviour is only supported at the job level. I'll do some testing first but it might be a good idea to support it even at the metric level.