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

Cache metric names provided by KEDA Metrics Server #2279

Merged
merged 7 commits into from
Nov 23, 2021

Conversation

zroubalik
Copy link
Member

@zroubalik zroubalik commented Nov 11, 2021

Signed-off-by: Zbynek Roubalik zroubali@redhat.com

Cache metrics provided by KEDA Metrics Server instead of requesting list of all ScaledObjects in the cluster, this should reduce the load on k8s server and Metrics Server should respond much faster.

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Changelog has been updated

Fixes #2156

Potential fix for: #1937

Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
@zroubalik
Copy link
Member Author

/run-e2e

@zroubalik zroubalik added this to the v2.5.0 milestone Nov 11, 2021
@zroubalik zroubalik changed the title Cache metrics provided by KEDA Metrics Server Cache metric names provided by KEDA Metrics Server Nov 11, 2021
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
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!

@JorTurFer
Copy link
Member

Should we add a note in the docs explaining this?
I mean, maybe in the KEDA metric server section where we explain how to query metric directly, we could write a short note. We are caching the metrics, IDK if in any case we could have any worng metric exposed there.

@zroubalik
Copy link
Member Author

Should we add a note in the docs explaining this? I mean, maybe in the KEDA metric server section where we explain how to query metric directly, we could write a short note. We are caching the metrics, IDK if in any case we could have any worng metric exposed there.

This change is just introducing a caching of the metric names that are provided by KEDA Metrics Server, not the actual metrics (values). It is internal change, so I am not sure that it is worth documenting. But I am open to do that if we all agree.

@JorTurFer
Copy link
Member

JorTurFer commented Nov 12, 2021

I don't have a strong opinion about it, I was just sharing my thoughts :)

Copy link
Contributor

@coderanger coderanger left a comment

Choose a reason for hiding this comment

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

Looks good to me overall, just a few micro-optimization thoughts because I'm a terrible person :)

controllers/keda/metrics_adapter_controller.go Outdated Show resolved Hide resolved
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
@zroubalik
Copy link
Member Author

/run-e2e

Zbynek Roubalik added 2 commits November 12, 2021 15:21
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
@zroubalik
Copy link
Member Author

/run-e2e

adapter/main.go Show resolved Hide resolved
pkg/provider/provider.go Outdated Show resolved Hide resolved
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
@zroubalik
Copy link
Member Author

@ahmelsayed PTAL

Copy link
Contributor

@arschles arschles left a comment

Choose a reason for hiding this comment

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

@zroubalik LGTM. do you think it's worth wrapping the slice pointer and its lock into a separate struct? might make for easier-to-follow code later on, and reduce the likelihood of concurrency bugs down the line. I'd be happy to do that in a follow-up if you like.

@zroubalik
Copy link
Member Author

@zroubalik LGTM. do you think it's worth wrapping the slice pointer and its lock into a separate struct? might make for easier-to-follow code later on, and reduce the likelihood of concurrency bugs down the line. I'd be happy to do that in a follow-up if you like.

@arschles I was thinking about similar approach, but not sure if it doesn't introduce additional complexity. But I am no strongly opposed to this.

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.

Cache metric names provided by KEDA Metrics Server
4 participants