[Feature] Add Support for Historical Data Points #986

luismy · 2023-06-11T20:52:05Z

Initial implementation to support historical data points by having a new flag addHistoricalMetrics at the job config level

Related to #985

Pending tasks

Build docker image and test to see if the Prometheus graphs are as accurate as the ones provided by AWS CloudWatch
Go over the code to change the implementation and use a hashmap to store the "template" metric
Make the new flag configurable at the metric level to keep it consistent with the other flags!? Check how easy/difficult is going to be
Add tests
Linting

Initial implementation to support historical data points by having a new flag `addHistoricalMetrics` at the job config level

luismy · 2023-06-11T20:53:09Z

pkg/promutil/migrate.go

+ // 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())


Is it safe to include the timestamp to remove "real" duplicates?

luismy · 2023-06-11T20:55:22Z

pkg/job/discovery.go

+ 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


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)

luismy · 2023-06-11T20:57:37Z

pkg/config/config.go

@@ -33,6 +33,7 @@ type JobLevelMetricFields struct {
 Delay int64 `yaml:"delay"`
 NilToZero *bool `yaml:"nilToZero"`
 AddCloudwatchTimestamp *bool `yaml:"addCloudwatchTimestamp"`
+ AddHistoricalMetrics *bool `yaml:"addHistoricalMetrics"`


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.

luismy · 2023-06-11T21:01:58Z

I'm planning to make more changes but marking this PR as ready for review to get early feedback

thomaspeitz · 2023-06-26T19:14:02Z

Love the idea of it :)

gavin-jeong · 2023-07-25T06:27:56Z

I'm using this from our environment. Works nice

ecktom · 2023-11-03T17:18:45Z

This would also drastically reduce costs for people who are currently looking to get metrics in a 1 minute resolution (scrape-interval=60s / period=60s / length=60s), right? @gavin-jeong you've been using this for a while without issues?

gavin-jeong · 2023-11-24T06:02:14Z

@ecktom no any issue yet. seems it works well.

luismy · 2024-03-03T20:36:55Z

Apologies for the delays on this one. I'm planning to spend some time next weekend refreshing this with the latest from master.

I hope the rebase is not too painful 😢

…oints_issue_985 # Conflicts: # pkg/clients/cloudwatch/client.go # pkg/clients/cloudwatch/v1/client.go # pkg/clients/cloudwatch/v2/client.go # pkg/clients/v2/cache_test.go # pkg/job/discovery.go # pkg/model/model.go

luismy · 2024-04-14T20:38:32Z

I completed the rebase locally a while back but I can see that there are more changes lol. Let's see if can get to a "safe" place this week 😭

ecktom · 2024-04-15T13:23:02Z

@luismy would be highly appreciated if you can give it another shot. We also tried to get it rebased but were stuck in some segfaults which we couldn't get right

kgeckhart · 2024-04-21T12:37:21Z

👋 as the main person who's been driving a lot of the conflicts here I'm sorry @luismy. I honestly didn't realize this existed for so long but I'm "done" enough with the getmetricdata refactoring work I set out to do to enable #1094 that I think further conflicts should be rather minor. ~~I will leave a comment with a suggestion that I think could cleanup the implementation~~ I think with the latest refactors there's a chance to have addHistoricalDatapoints be handled completely by https://github.com/nerdswords/yet-another-cloudwatch-exporter/blob/master/pkg/promutil/migrate.go#L79 simplifying the change and making it much more testable.

The latest refactors split a lot of CloudwatchData in to more independent structs specifically these three, https://github.com/nerdswords/yet-another-cloudwatch-exporter/blob/master/pkg/model/model.go#L165-L172.

The addHistoricalDatapoints config could be added to MetricMigrationParams.
Allow ScrapeAwsData to be unaware of addHistoricalDatapoints
- GetMetricDataResult(https://github.com/nerdswords/yet-another-cloudwatch-exporter/blob/master/pkg/model/model.go#L201-L205) is only aware of a single datapoint + timestamp but AWS returns multiple
- Change ^^ to include a slice of Datapoint + Timestamp
- Allow the Cloudwatch Client result struct to do the same https://github.com/nerdswords/yet-another-cloudwatch-exporter/blob/master/pkg/clients/cloudwatch/client.go#L45-L50
- Now all datapoints AWS returned are available on GetMetricDataResult allowing migration.go's BuildMetrics to conditionally map the latest or all the datapoints based on the addHistoricalDatapoints setting (this is very similar to how GetMetricStatisticsResult's are handled today https://github.com/nerdswords/yet-another-cloudwatch-exporter/blob/master/pkg/promutil/migrate.go#L155-L186)

Let me know what you think and if you could use a hand with conflict resolution.

First draft to support historical data points

7172252

Initial implementation to support historical data points by having a new flag `addHistoricalMetrics` at the job config level

luismy commented Jun 11, 2023

View reviewed changes

luismy marked this pull request as ready for review June 11, 2023 21:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Add Support for Historical Data Points #986

[Feature] Add Support for Historical Data Points #986

luismy commented Jun 11, 2023 •

edited

luismy Jun 11, 2023

luismy Jun 11, 2023

luismy Jun 11, 2023

luismy commented Jun 11, 2023

thomaspeitz commented Jun 26, 2023

gavin-jeong commented Jul 25, 2023

ecktom commented Nov 3, 2023

gavin-jeong commented Nov 24, 2023

luismy commented Mar 3, 2024

luismy commented Apr 14, 2024

ecktom commented Apr 15, 2024

kgeckhart commented Apr 21, 2024 •

edited

[Feature] Add Support for Historical Data Points #986

Are you sure you want to change the base?

[Feature] Add Support for Historical Data Points #986

Conversation

luismy commented Jun 11, 2023 • edited

Pending tasks

luismy Jun 11, 2023

Choose a reason for hiding this comment

luismy Jun 11, 2023

Choose a reason for hiding this comment

luismy Jun 11, 2023

Choose a reason for hiding this comment

luismy commented Jun 11, 2023

thomaspeitz commented Jun 26, 2023

gavin-jeong commented Jul 25, 2023

ecktom commented Nov 3, 2023

gavin-jeong commented Nov 24, 2023

luismy commented Mar 3, 2024

luismy commented Apr 14, 2024

ecktom commented Apr 15, 2024

kgeckhart commented Apr 21, 2024 • edited

luismy commented Jun 11, 2023 •

edited

kgeckhart commented Apr 21, 2024 •

edited