Improve the Datadog scaler, including a new optional parameter `metricUnavailableValue` to fill data when no Datadog metric was returned #2694

arapulido · 2022-03-01T13:42:59Z

Several non-breaking fixes for the Datadog scaler. Fixes #2657

The problem described in #2657 is partially caused by the selection of a time window that is too small (15 seconds). Datadog doesn't have that metric yet in many cases, and returns an empty value, that logs as a warning in the HPA.

To avoid this from happening often, the user should select a bigger window, and also added some retries before giving up. I also changed the documentation, to discourage reducing the time window: kedacore/keda-docs#682

To avoid this, we should remove the "age" parameter going forward, but as this is a breaking change, I will create a different PR for 2.7.

Other improvements in this PR:

If more than one point is returned, return the last one, instead of the first one, for more recent results
More parsing of the initial query, and some potential fixes to a incomplete query: default to avg aggregator, and only add a rollup if one wasn't added by the user
Moved to MilliQuantities and floats for query results for more granularity
Return a specific error if Datadog didn't return a metric value due to reaching rate limits for the organization
IsActive doesn't try to get a metric value, to reduce rate limiting

Checklist

Commits are signed with Developer Certificate of Origin (DCO - learn more)
Tests have been added
A PR is opened to update the documentation on (repo) (if applicable)
Changelog has been updated and is aligned with our changelog requirements

Fixes #2657

JorTurFer

Thanks for your contribution ❤️ ❤️
I have left some comments

pkg/scalers/datadog_scaler.go

arapulido · 2022-03-04T15:13:45Z

@tomkerkhove I implemented here the unavailableMetricValue optional parameter.

@JorTurFer I addressed your comments

Once this is approved, I will create a PR with the updated documentation

The missing thing is IsActive. I am not sure what needs implementing here. Anything that requires contacting the API will accelerate rate limiting, and it is not reliable. Any suggestions?

JorTurFer · 2022-03-07T08:09:11Z

@tomkerkhove I implemented here the unavailableMetricValue optional parameter.

@JorTurFer I addressed your comments

Once this is approved, I will create a PR with the updated documentation

The missing thing is IsActive. I am not sure what needs implementing here. Anything that requires contacting the API will accelerate rate limiting, and it is not reliable. Any suggestions?

The point here is that KEDA uses that method for getting if the scaler is active or not and that triggers things like scale from/to zero. If we return true always, KEDA thinks that the scaler is always active and will not scale to zero

arapulido · 2022-03-07T11:42:02Z

The point here is that KEDA uses that method for getting if the scaler is active or not and that triggers things like scale from/to zero. If we return true always, KEDA thinks that the scaler is always active and will not scale to zero

My question is: what do we mean by "a scaler is active"?

JorTurFer · 2022-03-07T12:04:20Z

I'm AFK but this is part of the info (from docs)

KEDA polls ScaledObject object according to the pollingInterval configured in the ScaledObject; it checks the last time it was polled, it checks if the number of replicas is greater than 0, and if the scaler itself is active. So if the scaler returns false for IsActive, and if current number of replicas is greater than 0, and there is no configured minimum pods, then KEDA scales down to 0.

JorTurFer · 2022-03-07T12:09:20Z

So it could be like:
IsActive=true: KEDA should scale from zero and let the HPA works
IsActive=false: KEDA should scale to zero (HPA doesn't work)

Of course, min replicas should be 0 for scaling to/from zero

zroubalik · 2022-03-07T12:39:31Z

The point here is that KEDA uses that method for getting if the scaler is active or not and that triggers things like scale from/to zero. If we return true always, KEDA thinks that the scaler is always active and will not scale to zero

My question is: what do we mean by "a scaler is active"?

As I mentioned above: #2694 (comment)

Active is when the target metric is above threshold. In this case a the value returned by query is above queryValue.

arapulido · 2022-03-08T11:36:24Z

@JorTurFer @zroubalik Thanks both! I have finally understood this :) I have added a commit for this, and I have also opened a PR to the Datadog scaler docs to improve the explanation related to polling intervals configuration, as it is fairly important to understand it, as it is directly affected by rate limits: kedacore/keda-docs#702

JorTurFer · 2022-03-08T12:39:23Z

/run-e2e datadog*
Update: You can check the progres here

JorTurFer

LGTM!
Thanks for these improvements!
(I'll wait to merge till e2e passes and docs PR is ready to merge too)

JorTurFer · 2022-03-08T13:14:27Z

e2e test has failed and also the workflow has failed because a needed rebase (rebasing main is not important at this moment if you don't want to do it, e2e test runs and only fails after it trying to clean up all namespaces).
This is the link to datadog e2e logs

arapulido · 2022-03-08T14:44:16Z

e2e test has failed and also the workflow has failed because a needed rebase (rebasing main is not important at this moment if you don't want to do it, e2e test runs and only fails after it trying to clean up all namespaces). This is the link to datadog e2e logs

@JorTurFer Added a couple of changes to make the tests more resilient. Can we rerun, please?

JorTurFer · 2022-03-10T11:40:25Z

/run-e2e datadog*
Update: You can check the progres here

JorTurFer · 2022-03-10T12:31:35Z

They have worked (again, ignore the error, it's because a rebase is needed for fixing it)
Have you got any other thing to add in this PR?
Could you update the PR with the issue please? Then I can merge this

As always, thanks a ton for you nice work ❤️ ❤️ ❤️ ❤️

arapulido · 2022-03-10T15:11:22Z

@JorTurFer Thanks! I don't have plans to add anything for now, but I need to know whether this is going to be targeted for 2.6 or 2.7, as I need to add changes to the documentation related to the new metricUnavailableValue parameter

JorTurFer · 2022-03-10T15:41:15Z

This change will be shipped as part of 2.7

arapulido · 2022-03-11T09:59:17Z

This change will be shipped as part of 2.7

Cool! The corresponding docs PR is here: kedacore/keda-docs#710

zroubalik

LGTM

zroubalik · 2022-03-11T11:20:20Z

@arapulido could you please update the Changelog? then we can merge this one. thanks!

zroubalik

Could you please rebase, there's a conflict in Changelog. Thanks!

CHANGELOG.md

pkg/scalers/datadog_scaler.go

… limiting Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

…ng a null metric, without blocking Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

…eturn that value as metric if the metric is not available for the time period selected Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

zroubalik

LGTM, thanks!

zroubalik · 2022-03-14T12:32:46Z

/run-e2e datadog*
Update: You can check the progres here

…vailableValue` to fill data when no Datadog metric was returned (kedacore#2694) Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com> Signed-off-by: Ram Cohen <ram.cohen@gmail.com>

arapulido requested a review from a team as a code owner March 1, 2022 13:42

arapulido changed the title ~~Fixes datadog~~ Improve the Datadog scaler with non-breaking changes Mar 1, 2022

JorTurFer requested changes Mar 1, 2022

View reviewed changes

JorTurFer approved these changes Mar 8, 2022

View reviewed changes

tomkerkhove mentioned this pull request Mar 11, 2022

Add new metricUnavailableValue kedacore/keda-docs#710

Merged

1 task

zroubalik approved these changes Mar 11, 2022

View reviewed changes

zroubalik requested changes Mar 11, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

arapulido force-pushed the fixes_datadog branch from f571306 to 2a5b681 Compare March 11, 2022 14:13

JorTurFer reviewed Mar 11, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

zroubalik reviewed Mar 14, 2022

View reviewed changes

pkg/scalers/datadog_scaler.go Show resolved Hide resolved

arapulido added 4 commits March 14, 2022 11:13

Do not query Datadog to check if the metric is active, to reduce rate…

2a588c2

… limiting Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

Return a specific error message when reaching rate limits

51361da

Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

Retry getting a metric up to 5 times, needed for short time windows

5c0c912

Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

Allow for more granularity from points returned by Datadog

1a99cd8

Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

arapulido added 14 commits March 14, 2022 11:13

Parse the query better and add rollup and aggregator only if missing

db1d5b9

Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

Return the value of the last point in the series

817842d

Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

Linting code

c3d60ab

Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

Move function closer to context

1cd870c

Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

Improve how to get metrics from Datadog to reduce the chance of getti…

90a816e

…ng a null metric, without blocking Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

Add new metricUnavailableValue optional parameter that if set, will r…

8a3417a

…eturn that value as metric if the metric is not available for the time period selected Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

Fix IsActive to allow this scaler to scale to zero if needed

44c10bf

Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

Avoid race condition when deleting the test namespace

2b7c298

Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

Wait a bit longer in tests to scale down

1f7b807

Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

Modify changelog to add new datadog scaler parameter

089e78b

Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

Point to issue not PR in changelog

7a86aff

Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

Order changelog alphabetically

44dfd5d

Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

Improve Datadog query parsing error catching

80db12a

Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

Add tests for the parseAndTransformDatadogQuery function

9a5a747

Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>

arapulido force-pushed the fixes_datadog branch from 2a5b681 to 9a5a747 Compare March 14, 2022 11:10

zroubalik approved these changes Mar 14, 2022

View reviewed changes

zroubalik mentioned this pull request Mar 14, 2022

Datadog: add more information related to polling intervals that may affect rate limiting kedacore/keda-docs#702

Merged

1 task

zroubalik merged commit 877d0f6 into kedacore:main Mar 14, 2022

zroubalik changed the title ~~Improve the Datadog scaler with non-breaking changes~~ Improve the Datadog scaler, including a new optional parameter metricUnavailableValue to fill data when no Datadog metric was returned Mar 14, 2022

dalgibbard mentioned this pull request Mar 15, 2022

Datadog query with functions never returns healthy #2761

Closed

JorTurFer mentioned this pull request Mar 16, 2022

Datadog scaler doesn't scale correctly to 0 #2759

Closed

arapulido deleted the fixes_datadog branch March 17, 2022 16:55

zroubalik mentioned this pull request Mar 22, 2022

Datadog: isActive() should react on num > 0 #2798

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the Datadog scaler, including a new optional parameter `metricUnavailableValue` to fill data when no Datadog metric was returned #2694

Improve the Datadog scaler, including a new optional parameter `metricUnavailableValue` to fill data when no Datadog metric was returned #2694

arapulido commented Mar 1, 2022 •

edited by zroubalik

Loading

JorTurFer left a comment

arapulido commented Mar 4, 2022

JorTurFer commented Mar 7, 2022

arapulido commented Mar 7, 2022

JorTurFer commented Mar 7, 2022

JorTurFer commented Mar 7, 2022 •

edited

Loading

zroubalik commented Mar 7, 2022

arapulido commented Mar 8, 2022

JorTurFer commented Mar 8, 2022 •

edited by github-actions bot

Loading

JorTurFer left a comment

JorTurFer commented Mar 8, 2022

arapulido commented Mar 8, 2022

JorTurFer commented Mar 10, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Mar 10, 2022

arapulido commented Mar 10, 2022

JorTurFer commented Mar 10, 2022

arapulido commented Mar 11, 2022

zroubalik left a comment

zroubalik commented Mar 11, 2022

zroubalik left a comment

zroubalik left a comment

zroubalik commented Mar 14, 2022 •

edited by github-actions bot

Loading

Improve the Datadog scaler, including a new optional parameter metricUnavailableValue to fill data when no Datadog metric was returned #2694

Improve the Datadog scaler, including a new optional parameter metricUnavailableValue to fill data when no Datadog metric was returned #2694

Conversation

arapulido commented Mar 1, 2022 • edited by zroubalik Loading

Checklist

JorTurFer left a comment

Choose a reason for hiding this comment

arapulido commented Mar 4, 2022

JorTurFer commented Mar 7, 2022

arapulido commented Mar 7, 2022

JorTurFer commented Mar 7, 2022

JorTurFer commented Mar 7, 2022 • edited Loading

zroubalik commented Mar 7, 2022

arapulido commented Mar 8, 2022

JorTurFer commented Mar 8, 2022 • edited by github-actions bot Loading

JorTurFer left a comment

Choose a reason for hiding this comment

JorTurFer commented Mar 8, 2022

arapulido commented Mar 8, 2022

JorTurFer commented Mar 10, 2022 • edited by github-actions bot Loading

JorTurFer commented Mar 10, 2022

arapulido commented Mar 10, 2022

JorTurFer commented Mar 10, 2022

arapulido commented Mar 11, 2022

zroubalik left a comment

Choose a reason for hiding this comment

zroubalik commented Mar 11, 2022

zroubalik left a comment

Choose a reason for hiding this comment

zroubalik left a comment

Choose a reason for hiding this comment

zroubalik commented Mar 14, 2022 • edited by github-actions bot Loading

Improve the Datadog scaler, including a new optional parameter `metricUnavailableValue` to fill data when no Datadog metric was returned #2694

Improve the Datadog scaler, including a new optional parameter `metricUnavailableValue` to fill data when no Datadog metric was returned #2694

arapulido commented Mar 1, 2022 •

edited by zroubalik

Loading

JorTurFer commented Mar 7, 2022 •

edited

Loading

JorTurFer commented Mar 8, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Mar 10, 2022 •

edited by github-actions bot

Loading

zroubalik commented Mar 14, 2022 •

edited by github-actions bot

Loading