Restore default behaviour of returning nil/absent metrics as NaN #1288

nhinds · 2024-01-30T03:00:20Z

The default value for MetricDataResult.Datapoint was 0, so all metrics without datapoints were being returned as if they had a datapoint with the value 0, regardless of the nilToZero flag.

Making this field a pointer allows the v1 and v2 clients to return nil when there is no data, which eventually gets passed through to the existing code for handling nil datapoints and the nilToZero flag.

Fixes #1242

Might be related to #1138

benbridts · 2024-01-30T09:07:18Z

I tested this to see if it fixes #1138, and it seems like it does:

I am using this config:

---
apiVersion: v1alpha1
discovery:
  jobs:
    - type: AWS/DDoSProtection
      metrics:
        - name: DDoSDetected
          statistics: [ SampleCount ]
      addCloudwatchTimestamp: true
      period: 300
      length: 300
      regions:
        - eu-west-1

    - type: AWS/ApiGateway
      metrics:
        - name: 5XXError
          statistics: [ SampleCount ]
      addCloudwatchTimestamp: true
      period: 300
      length: 300
      regions:
        - eu-west-1

On the master branch, this give me (trimmed for clarity):

aws_apigateway_5_xxerror_sample_count{...} 0 -62135596800000
aws_ddosprotection_ddo_sdetected_sample_count{...} 0 -62135596800000
aws_apigateway_info{...} 0
aws_ddosprotection_info{...} 0

with this branch, I get:

aws_apigateway_info{...} 0
aws_ddosprotection_info{...} 0

which is the same as with previous versions like v0.49.0 and v0.50.0.

If I remove addCloudwatchTimestamp: true, I get on master:

aws_ddosprotection_ddo_sdetected_sample_count{...} 0
aws_apigateway_5_xxerror_sample_count{...} 0
aws_apigateway_info{...} 0
aws_ddosprotection_info{...} 0

and on this branch:

aws_ddosprotection_ddo_sdetected_sample_count{...} NaN
aws_apigateway_5_xxerror_sample_count{...} NaN
aws_apigateway_info{...} 0
aws_ddosprotection_info{...} 0

which is the same as with previous versions like v0.49.0 I also got the NaN behaviour, so this PR looks fine as a fix for #1138 (in previous versions theaws_ddosprotection_ddo_sdetected_sample_count is missing, but that is unrelated to this PR.

nhinds · 2024-02-07T22:22:27Z

This will be solved differently in #1289, closing

nhinds · 2024-02-09T21:23:46Z

Reopening per discussion in #1289 (#1289 (comment)). Will aim to get the bugfix and tests merged and handle the refactor in a later PR.

CC @cristiangreco

nhinds · 2024-02-09T21:44:22Z

Linting failures seem to be new between golangci-lint 1.56.0 and 1.56.1 - docker run --rm -v $(pwd):/app -w /app golangci/golangci-lint:v1.56.0 golangci-lint run -v succeeds, while docker run --rm -v $(pwd):/app -w /app golangci/golangci-lint:v1.56.1 golangci-lint run -v fails. The same code also passed linting 2 weeks ago on version 1.55.2 (https://github.com/nerdswords/yet-another-cloudwatch-exporter/actions/runs/7705664092/job/20999963536). I assume the CI is floating on the latest version of golangci-lint as I don't see any commits that would bump this.

The failures seem to be in files I haven't touched (pkg/clients/tagging/*, pkg/clients/v1/*, pkg/job/compact_test.go, cmd/yace/*)

Makefile

cristiangreco · 2024-02-12T11:44:31Z

The failures seem to be in files I haven't touched (pkg/clients/tagging/*, pkg/clients/v1/*, pkg/job/compact_test.go, cmd/yace/*)

Can you please rebase? The linter errors have been fixed in #1295

The `$` in the Makefile was being interpreted by `make`, and running the incorrect command `go test -v -bench=^-race -count=1 ./...`

cristiangreco

LGTM, left just some comments around tests.

pkg/clients/cloudwatch/v2/client_test.go

pkg/clients/cloudwatch/v1/client_test.go

…structs

The default value for MetricDataResult.Datapoint was 0, so all metrics without datapoints were being returned as if they had a datapoint with the value 0, regardless of the nilToZero flag. Making this field a pointer allows the v1 and v2 clients to return nil when there is no data.

cristiangreco

Great work! 🚀

…metheus-community#1288) Co-authored-by: Cristian Greco <[email protected]>

* Bump github.com/aws/aws-sdk-go from 1.49.16 to 1.49.17 (prometheus-community#1267) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Add IPAM metrics support (prometheus-community#1168) Co-authored-by: Cristian Greco <[email protected]> * Bump github.com/aws/aws-sdk-go from 1.49.17 to 1.49.19 (prometheus-community#1275) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump the aws-sdk-v2 group with 1 update (prometheus-community#1274) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Cristian Greco <[email protected]> * Prepare changelog for v0.56.0 (prometheus-community#1273) * Bump github.com/aws/aws-sdk-go from 1.49.19 to 1.49.21 (prometheus-community#1277) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump github.com/prometheus/common from 0.45.0 to 0.46.0 (prometheus-community#1276) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Optionally include context labels (account, region, customTags) on info metrics (prometheus-community#1249) Co-authored-by: Cristian Greco <[email protected]> * Simplify associator usage (prometheus-community#1280) Co-authored-by: Cristian Greco <[email protected]> * Fix race condition in scraper registry usage (prometheus-community#1281) * Refactor dimensions regexp usage for discovery jobs (prometheus-community#1279) * Bump the aws-sdk-v2 group with 1 update (prometheus-community#1282) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Cristian Greco <[email protected]> * Bump github.com/aws/aws-sdk-go from 1.49.21 to 1.50.0 (prometheus-community#1283) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Cristian Greco <[email protected]> * Improve usability and performance of searchTags (prometheus-community#1270) Co-authored-by: Cristian Greco <[email protected]> * Fix lint issues for golangci-lint 1.56.1 (prometheus-community#1295) * Bump alpine from 3.19.0 to 3.19.1 (prometheus-community#1287) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Cristian Greco <[email protected]> * Add serverless ElastiCache support (prometheus-community#1254) Co-authored-by: Cristian Greco <[email protected]> * Fix elasticache serverless test after merge (prometheus-community#1299) * Restore default behaviour of returning nil/absent metrics as NaN (prometheus-community#1288) Co-authored-by: Cristian Greco <[email protected]> * Bump the aws-sdk-v2 group with 5 updates (prometheus-community#1297) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Cristian Greco <[email protected]> * Bump github.com/aws/aws-sdk-go from 1.50.0 to 1.50.10 (prometheus-community#1291) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Cristian Greco <[email protected]> * Update build tools and CI to go 1.22 (prometheus-community#1301) * chore: update version * chore: update test to support historical data * chore: fix return * chore: fix git missing for test * chore: cgo for test * chore: don't use cgo --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: pkubicsek-sb <[email protected]> Co-authored-by: Cristian Greco <[email protected]> Co-authored-by: kgeckhart <[email protected]> Co-authored-by: Nicholas Hinds <[email protected]>

bogdankatishev mentioned this pull request Feb 1, 2024

[BUG] Invalid timestamps on metrics with addCloudwatchTimestamp after upgrading from 0.53.0 to 0.54.1 #1138

Closed

1 task

nhinds mentioned this pull request Feb 7, 2024

Restructure fields on CloudwatchData #1289

Merged

nhinds closed this Feb 7, 2024

nhinds reopened this Feb 9, 2024

nhinds force-pushed the allow-nil-nan-metrics branch from 54d2d80 to 245d834 Compare February 9, 2024 21:23

cristiangreco reviewed Feb 12, 2024

View reviewed changes

Makefile Show resolved Hide resolved

nhinds force-pushed the allow-nil-nan-metrics branch from 245d834 to 4f565b7 Compare February 12, 2024 21:10

Escape $ in Makefile

77bc5f2

The `$` in the Makefile was being interpreted by `make`, and running the incorrect command `go test -v -bench=^-race -count=1 ./...`

nhinds force-pushed the allow-nil-nan-metrics branch from 4f565b7 to 3310336 Compare February 12, 2024 21:11

cristiangreco reviewed Feb 13, 2024

View reviewed changes

pkg/clients/cloudwatch/v2/client_test.go Outdated Show resolved Hide resolved

pkg/clients/cloudwatch/v2/client_test.go Outdated Show resolved Hide resolved

pkg/clients/cloudwatch/v1/client_test.go Show resolved Hide resolved

nhinds added 3 commits February 14, 2024 11:51

Test migrating nilToZero and addCloudwatchTimestamp metrics

a1290e2

Add test for mapping GetMetricData responses to CloudwatchData model …

a4da5f6

…structs

nhinds force-pushed the allow-nil-nan-metrics branch from 3310336 to dd03081 Compare February 13, 2024 22:53

Merge branch 'master' into allow-nil-nan-metrics

0969cf5

cristiangreco approved these changes Feb 14, 2024

View reviewed changes

cristiangreco merged commit 340e810 into prometheus-community:master Feb 14, 2024
3 checks passed

TanZng pushed a commit to grid-x/yet-another-cloudwatch-exporter that referenced this pull request Feb 14, 2024

Restore default behaviour of returning nil/absent metrics as NaN (pro…

93aa3fa

…metheus-community#1288) Co-authored-by: Cristian Greco <[email protected]>

TanZng pushed a commit to grid-x/yet-another-cloudwatch-exporter that referenced this pull request Feb 14, 2024

Restore default behaviour of returning nil/absent metrics as NaN (pro…

e580c45

…metheus-community#1288) Co-authored-by: Cristian Greco <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore default behaviour of returning nil/absent metrics as NaN #1288

Restore default behaviour of returning nil/absent metrics as NaN #1288

nhinds commented Jan 30, 2024

benbridts commented Jan 30, 2024

nhinds commented Feb 7, 2024

nhinds commented Feb 9, 2024

nhinds commented Feb 9, 2024 •

edited

Loading

cristiangreco commented Feb 12, 2024

cristiangreco left a comment

cristiangreco left a comment

Restore default behaviour of returning nil/absent metrics as NaN #1288

Restore default behaviour of returning nil/absent metrics as NaN #1288

Conversation

nhinds commented Jan 30, 2024

benbridts commented Jan 30, 2024

nhinds commented Feb 7, 2024

nhinds commented Feb 9, 2024

nhinds commented Feb 9, 2024 • edited Loading

cristiangreco commented Feb 12, 2024

cristiangreco left a comment

Choose a reason for hiding this comment

cristiangreco left a comment

Choose a reason for hiding this comment

nhinds commented Feb 9, 2024 •

edited

Loading