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

feat: Dynatrace scaler #5685

Merged
merged 30 commits into from
Jul 30, 2024
Merged

feat: Dynatrace scaler #5685

merged 30 commits into from
Jul 30, 2024

Conversation

cyrilico
Copy link
Contributor

@cyrilico cyrilico commented Apr 11, 2024

Add new scaler for interacting with Dynatrace and its Get Metric Data Points API

Checklist

Closes #2411

Relates to kedacore/keda-docs#1360

Signed-off-by: cyrilico <[email protected]>
@cyrilico cyrilico marked this pull request as ready for review April 24, 2024 14:40
@cyrilico cyrilico requested a review from a team as a code owner April 24, 2024 14:40
@zroubalik
Copy link
Member

zroubalik commented Apr 24, 2024

/run-e2e dynatrace
Update: You can check the progress here

@JorTurFer
Copy link
Member

Please, rebase your branch @cyrilico , the e2e issue was related with some breaking changes in otel-collector which are already fixed in main branch (your tests hasn't been executed yet indeed)

@JorTurFer
Copy link
Member

JorTurFer commented Apr 28, 2024

/run-e2e dynatrace
Update: You can check the progress here

tests/scalers/dynatrace/dynatrace_test.go Outdated Show resolved Hide resolved
tests/scalers/dynatrace/dynatrace_test.go Outdated Show resolved Hide resolved
tests/scalers/dynatrace/dynatrace_test.go Outdated Show resolved Hide resolved
cyrilico and others added 2 commits April 28, 2024 14:48
Co-authored-by: Jorge Turrado Ferrero <[email protected]>
Signed-off-by: cyrilico <[email protected]>
@cyrilico cyrilico requested a review from JorTurFer April 28, 2024 14:03
@JorTurFer
Copy link
Member

JorTurFer commented Apr 28, 2024

/run-e2e dynatrace
Update: You can check the progress here

@cyrilico
Copy link
Contributor Author

new changes, please rerun

@JorTurFer
Copy link
Member

JorTurFer commented Apr 28, 2024

/run-e2e dynatrace
Update: You can check the progress here

Signed-off-by: Jorge Turrado Ferrero <[email protected]>
@JorTurFer
Copy link
Member

JorTurFer commented Apr 28, 2024

/run-e2e dynatrace
Update: You can check the progress here

@JorTurFer
Copy link
Member

@cyrilico I've merged 2 fixes to the e2e to trigger them again (you'll need to pull the changes :) )

@JorTurFer
Copy link
Member

Hello,
I've been testing this locally and I found some things:

  • The dynatrace operator fails because the token doesn't have enough permissions for it
  • The job generators aren't generating the load because the values are overridden
    image

For the dynatrace operator issue, I've created a new secret 'DYNATRACE_OPERATOR_TOKEN', please use it for the operator and the other one for metrics (including for reading them by KEDA)

After making these changes, I see that dynatrace operator has created more pods:
image

But the metric isn't still available:
image

I think that the issue can be in something related with the e2e test configuration because I can see that KEDA is querying the value and getting an empty response with status 200 (so from KEDA side seems that works)
image

@cyrilico
Copy link
Contributor Author

Appreciate the help @JorTurFer 🙏 What metrics are available in that page? The one I used is supposed to be a built-in, i.e., supported OOTB, so I'm curious as to what exactly is available in that dynatrace environment

@cyrilico
Copy link
Contributor Author

Any news @JorTurFer ? 🙏

@JorTurFer
Copy link
Member

Any news @JorTurFer ? 🙏

I have to apologize for not answering before, I've been involved on a huge working peak :(
I'll try to review this ASAP. I guess that the issue with the metrics was that they needed more time to be consolidated, maybe we can use an approach like the Azure Monitor one, just querying about any available metric as default and updating the ScaledObject to trigger the scaling. e.g: Imagine that you can return an arbitrary number and you use it for scaling, at the end of the day, we don't need to check that the vendor calculates correctly the metric but just the connectivity

@cyrilico
Copy link
Contributor Author

Thanks, I have noticed in the corporate instance I have access to that Dynatrace does take a minute (sometimes literally) to ingest a data point, so I believe that might be an interesting approach. I'll take a look at the Azure Monitor scaler and try to get some inspiration

@JorTurFer
Copy link
Member

For instance, influx does it too: https://github.com/kedacore/keda/blob/main/tests/scalers/influxdb/influxdb_test.go#L106-L117

It sets the expected value within the query and just updating the ScaledObject you change the received value.

And this is the Azure log analytics one (I said azure monitor worngly): https://github.com/kedacore/keda/blob/main/tests/scalers/azure/azure_log_analytics/azure_log_analytics_test.go#L113-L123

@cyrilico
Copy link
Contributor Author

@JorTurFer couldn't find an exact map to replicate that mechanism in Dynatrace (other than potentially mocking the API calls, which would invalidate the connection testing part), so I've tried a configurable default value filter available in their query language spec; can we have another e2e test run?

@JorTurFer
Copy link
Member

JorTurFer commented Jul 25, 2024

/run-e2e dynatrace
Update: You can check the progress here

@JorTurFer
Copy link
Member

Hello,
I'm so sorry due to the long silent from my side and I'd like to apologize, my life's been complicated during last months. During this week and next week I'll be focus on this PR (and others) aiming to include them within next week release.
Thanks for your contribution :)

@JorTurFer
Copy link
Member

JorTurFer commented Jul 25, 2024

/run-e2e dynatrace
Update: You can check the progress here

@JorTurFer
Copy link
Member

JorTurFer commented Jul 25, 2024

@cyrilico , I've updated the e2e test to use custom metrics. We have the full control over them and they don't need the agent or any other service, just POST to ingest and KEDA will read them 😄
It's the same approach that we use for Azure Application Insights

@JorTurFer
Copy link
Member

JorTurFer commented Jul 25, 2024

/run-e2e dynatrace
Update: You can check the progress 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 for the contribution! ❤️
PTAL @zroubalik

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.

This is really great, there is one minor thing, do you think you can refactor the scaler metadata parsing (parseDynatraceMetadata())? We introduced a new simplified way of parsing, it would be great to include this change in this scaler. You can see details at #5797

@cyrilico cyrilico requested a review from zroubalik July 30, 2024 10:29
@cyrilico
Copy link
Contributor Author

done @zroubalik 👌

@JorTurFer
Copy link
Member

JorTurFer commented Jul 30, 2024

/run-e2e dynatrace
Update: You can check the progress here

@JorTurFer JorTurFer merged commit 12a529d into kedacore:main Jul 30, 2024
18 checks passed
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.

thanks @cyrilico !

@satrox28
Copy link

satrox28 commented Aug 10, 2024

Thank you, makes life easier now. Don't have to deploy Prometheus server additionally on each cluster.

JorTurFer added a commit to JorTurFer/keda that referenced this pull request Oct 7, 2024
* Add first scaler version

Signed-off-by: cyrilico <[email protected]>

* small refactor for response validation

Signed-off-by: cyrilico <[email protected]>

* Add 'from' property, rename host/token

Signed-off-by: cyrilico <[email protected]>

* Add parsing tests

Signed-off-by: cyrilico <[email protected]>

* update changelog

Signed-off-by: cyrilico <[email protected]>

* Update CHANGELOG.md

Signed-off-by: damas <[email protected]>

* Update values type to float64

Signed-off-by: damas <[email protected]>

* Remove unnecessary conversion

Signed-off-by: damas <[email protected]>

* e2e tests

Signed-off-by: cyrilico <[email protected]>

* Apply suggestions from code review

Co-authored-by: Jorge Turrado Ferrero <[email protected]>
Signed-off-by: cyrilico <[email protected]>

* Update dynatrace_test.go

Signed-off-by: cyrilico <[email protected]>

* Fix bad templating for e2e tests

Signed-off-by: cyrilico <[email protected]>

* Revert unnecessary (?) template variable change

Signed-off-by: cyrilico <[email protected]>

* Apply suggestions from code review

Signed-off-by: Jorge Turrado Ferrero <[email protected]>

* Update tests/scalers/dynatrace/dynatrace_test.go

Signed-off-by: Jorge Turrado Ferrero <[email protected]>

* Do not allow token to be passed in scaledobject trigger

Signed-off-by: cyrilico <[email protected]>

* Remove bad secret, tweak dynakube test config

Signed-off-by: cyrilico <[email protected]>

* Rename property in response parsing

Signed-off-by: cyrilico <[email protected]>

* Update tests/scalers/dynatrace/dynatrace_test.go

Signed-off-by: Jorge Turrado Ferrero <[email protected]>

* use new operator secret, update template variable naming

Signed-off-by: cyrilico <[email protected]>

* forgotten correct variable definition

Signed-off-by: cyrilico <[email protected]>

* try default value in query for e2e tests

Signed-off-by: cyrilico <[email protected]>

* fix missing closing parenthesis, bad indenting

Signed-off-by: cyrilico <[email protected]>

* Update e2e test to use custom metrics

Signed-off-by: Jorge Turrado <[email protected]>

* Close the body to fix static checks

Signed-off-by: Jorge Turrado <[email protected]>

* use declarative scaler config

Signed-off-by: cyrilico <[email protected]>

---------

Signed-off-by: cyrilico <[email protected]>
Signed-off-by: damas <[email protected]>
Signed-off-by: Jorge Turrado Ferrero <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
Co-authored-by: Jorge Turrado Ferrero <[email protected]>
Co-authored-by: Jorge Turrado <[email protected]>
Signed-off-by: Jorge Turrado <[email protected]>
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.

Provide scaler for Dynatrace metrics
4 participants