Skip to content
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

Add new Datadog scaler #2354

Merged
merged 16 commits into from
Jan 12, 2022
Merged

Add new Datadog scaler #2354

merged 16 commits into from
Jan 12, 2022

Conversation

arapulido
Copy link
Contributor

@arapulido arapulido commented Nov 26, 2021

This PR adds a new Datadog scaler. Right now, it uses Datadog metrics to drive HPA, both on a target value and a target average value. In the future, it could grow to also allow to scale based on events, monitor statuses, etc.

Related docs PR: kedacore/keda-docs#602

This is an example of what the TriggerAuthentication and the ScaledObject definitions would look like:

apiVersion: v1
kind: Secret
metadata:
  name: datadog-secrets
  namespace: my-project
type: Opaque
data:
  apiKey: # Required: base64 encoded value of Datadog apiKey
  appKey: # Required: base64 encoded value of Datadog appKey
  datadogSite: # Optional: base64 encoded value of Datadog site
---
apiVersion: keda.sh/v1alpha1
kind: TriggerAuthentication
metadata:
  name: keda-trigger-auth-datadog-secret
  namespace: my-project
spec:
  secretTargetRef:
    # Required: API key for your Datadog account
  - parameter: apiKey
    name: datadog-secrets
    key: apiKey
    # Required: APP key for your Datadog account
  - parameter: appKey
    name: datadog-secrets
    key: appKey
    # Optional: Datadog site. Default: "datadoghq.com"
  - parameter: datadogSite
    name: datadog-secrets
    key: datadogSite
---
apiVersion: keda.sh/v1alpha1
kind: ScaledObject
metadata:
  name: datadog-scaledobject
  namespace: my-project
spec:
  scaleTargetRef:
    name: worker
  triggers:
  - type: datadog
    metadata:
      # Required: datadog metric query
      query: "sum:trace.redis.command.hits{env:none,service:redis}.as_count()"
      # Required: according to the number of query result, to scale the TargetRef
      queryValue: "7"
      # Optional: (global or average). Whether the target value is global or average per pod. Default: average
      type: "global"
      # Optional: The time window (in seconds) to retrieve metrics from Datadog. Default: 90
      age: "60"
    authenticationRef:
      name: keda-trigger-auth-datadog-secret

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Changelog has been updated

Relates to #1002

@arapulido arapulido requested a review from a team as a code owner November 26, 2021 11:11
@tomkerkhove
Copy link
Member

Thank you for your PR! Would you mind opening a PR for docs as well please?
https://github.com/kedacore/keda-docs

@zroubalik
Copy link
Member

Thank you for your PR! Would you mind opening a PR for docs as well please? https://github.com/kedacore/keda-docs

And e2e tests please :) https://github.com/kedacore/keda/tree/main/tests

@arapulido
Copy link
Contributor Author

Thank you for your PR! Would you mind opening a PR for docs as well please? https://github.com/kedacore/keda-docs

PR for docs: kedacore/keda-docs#602

@arapulido
Copy link
Contributor Author

Thank you for your PR! Would you mind opening a PR for docs as well please? https://github.com/kedacore/keda-docs

And e2e tests please :) https://github.com/kedacore/keda/tree/main/tests

I will work on those. Thanks!

@arapulido
Copy link
Contributor Author

Thanks both for the early feedback. I will be off next week. I will work on the e2e tests once I get back. Thanks again!

@tomkerkhove
Copy link
Member

Thanks and have a good week off!

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your effort! ❤️
Apart from the e2e test, 2 small nits

pkg/scalers/datadog_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/datadog_scaler.go Outdated Show resolved Hide resolved
@JorTurFer JorTurFer requested a review from a team November 29, 2021 07:23
pkg/scalers/datadog_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/datadog_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/datadog_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/datadog_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/datadog_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/datadog_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/datadog_scaler.go Show resolved Hide resolved
@zroubalik zroubalik mentioned this pull request Dec 9, 2021
5 tasks
@arapulido
Copy link
Contributor Author

Hello all! Thanks again for the reviews! I think I have addressed all comments and I have pushed the E2E tests. Let me know how we can do it to add the needed secrets to your pipeline.

@JorTurFer
Copy link
Member

Hello all! Thanks again for the reviews! I think I have addressed all comments and I have pushed the E2E tests. Let me know how we can do it to add the needed secrets to your pipeline.

Hi @arapulido ,
Thanks for this!!!
I guess that we have to request any free access account for datadog, let me know what secrets are needed and I will request the account and add the secrets to the pipeline (sorry, I have 0 exprtise with datadog)

@JorTurFer JorTurFer mentioned this pull request Dec 21, 2021
5 tasks
@JorTurFer
Copy link
Member

JorTurFer commented Dec 21, 2021

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

@JorTurFer JorTurFer self-requested a review December 22, 2021 13:03
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looking good, I have 2 questions:

  1. I can see that you are using datadog-api-client-go/api/v1, though it seems like there is v2 as well. Is there a particular reason to not use v2?
  2. Do you think you can use KEDA http client? (it allows setting a global timeout), seems like the datadog client might allow this: https://github.com/DataDog/datadog-api-client-go/blob/4cb420c458f3e0259afd11a0a918108ebd8cbd32/api/v1/datadog/client.go#L118-L121
    Example usage of KEDA http client:
    httpClient := kedautil.CreateHTTPClient(config.GlobalHTTPTimeout, false)

@zroubalik zroubalik added this to the v2.6.0 milestone Jan 3, 2022
@JorTurFer
Copy link
Member

JorTurFer commented Jan 3, 2022

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

@JorTurFer
Copy link
Member

JorTurFer commented Jan 3, 2022

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

@JorTurFer
Copy link
Member

Hi @arapulido
I have tried again the e2e tests after downgrade to free plan and they fail, I think that is not related with Datadog plan but maybe you could take a look.
Thanks in advance ❤️

@arapulido
Copy link
Contributor Author

arapulido commented Jan 10, 2022

Hi @arapulido I have tried again the e2e tests after downgrade to free plan and they fail, I think that is not related with Datadog plan but maybe you could take a look. Thanks in advance ❤️

I cannot reproduce this failure in my environment. It is failing creating a secret, but not sure why. What would be the best way to debug this in the CI environment, @JorTurFer ?

The line that fails the only thing it does is the following:

kubectl create secret generic datadog-secrets --from-literal=apiKey=${datadogApiKey} \
      --from-literal=appKey=${datadogAppKey} --from-literal=datadogSite=${datadogSite} --namespace ${testNamespace}

can you make sure that the env variables that populate the API and APP keys are not empty? Thanks!

@JorTurFer
Copy link
Member

JorTurFer commented Jan 10, 2022

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

@JorTurFer
Copy link
Member

Hi @arapulido I have tried again the e2e tests after downgrade to free plan and they fail, I think that is not related with Datadog plan but maybe you could take a look. Thanks in advance ❤️

I cannot reproduce this failure in my environment. It is failing creating a secret, but not sure why. What would be the best way to debug this in the CI environment, @JorTurFer ?

The line that fails the only thing it does is the following:

kubectl create secret generic datadog-secrets --from-literal=apiKey=${datadogApiKey} \
      --from-literal=appKey=${datadogAppKey} --from-literal=datadogSite=${datadogSite} --namespace ${testNamespace}

can you make sure that the env variables that populate the API and APP keys are not empty? Thanks!

The second one is noise because the namespace was terminating and that was why it failed. I think that the problem could be in the creation order, before the SO and then the secret.

@arapulido
Copy link
Contributor Author

Generally looking good, I have 2 questions:

1. I can see that you are using `datadog-api-client-go/api/v1`, though it seems like there is `v2` as well. Is there a particular reason to not use `v2`?

I use an endpoint (QueryMetrics) that is not yet available in v2.

2. Do you think you can use KEDA http client? (it allows setting a global timeout), seems like the datadog client might allow this: https://github.com/DataDog/datadog-api-client-go/blob/4cb420c458f3e0259afd11a0a918108ebd8cbd32/api/v1/datadog/client.go#L118-L121
   Example usage of KEDA http client: https://github.com/kedacore/keda/blob/eeefb100ad9418a3add72b481d2495feb0318d99/pkg/scalers/metrics_api_scaler.go#L75

Sure, I have now pushed a commit with this. Thanks!

@JorTurFer
Copy link
Member

JorTurFer commented Jan 11, 2022

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

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Thanks a ton for the effort that you have done doing this PR ❤️ ❤️ ❤️
There is a conflict in CHANGELOG.md, could you solve it?

Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>
Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>
Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>
Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>
Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>
Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>
Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>
Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>
Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>
Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>
Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>
Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>
Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>
Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>
Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>
@arapulido
Copy link
Contributor Author

LGTM! Thanks a ton for the effort that you have done doing this PR ❤️ ❤️ ❤️ There is a conflict in CHANGELOG.md, could you solve it?

Done! I have rebased with main and resolved the conflict. Thanks a lot for the reviews and the help!

@JorTurFer
Copy link
Member

JorTurFer commented Jan 11, 2022

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

pkg/scalers/datadog_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/datadog_scaler.go Show resolved Hide resolved
pkg/scalers/datadog_scaler.go Show resolved Hide resolved
Signed-off-by: Ara Pulido <ara.pulido@datadoghq.com>
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Great job :)

@zroubalik zroubalik merged commit aa20943 into kedacore:main Jan 12, 2022
@bmpandrade
Copy link

So good to see this being merged... thanks to all who contributed to make this happen 🙌

Is there a possibility to release this sooner, or v2.6.0 is only due to feb 24 as the milestone states?

@JorTurFer
Copy link
Member

Hi @bmpandrade
You could use main tag if you'd like to try this scaler before the release.

@zroubalik
Copy link
Member

@bmpandrade we might actually do this release a little bit sooner, maybe end of this month. Be careful, consuming KEDA from main is obviously not stable and not for production :)

@bmpandrade
Copy link

Thank you both :)

At this point, would be mostly for experimentation so we would be fine with ref main for now until we have a release out.

@arschles
Copy link
Contributor

@bmpandrade just an idea: for now, until the release, you could pull main down, tag it, and push it to your own image repository. that would ensure that you only have this change and not anything more that gets merged between now and the release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants