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.

Sign up for GitHub

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

Jump to bottom

feat: Provide Azure Application Insights Scaler #2506

Merged
merged 6 commits into from
Jan 30, 2022
Merged

feat: Provide Azure Application Insights Scaler #2506

merged 6 commits into from
Jan 30, 2022

Conversation

markrzasa
Copy link
Contributor

@markrzasa markrzasa commented Jan 23, 2022 •
edited
Loading

Signed-off-by: Mark Rzasa [email protected]

This pull request adds an Azure Application Insights scaler. This is related to this issue:

#1965

This pull request is marked as a draft because an app insights scaler may not be necessary to resolve issue 1965. Alternatively, the Azure Log Analytics scaler could be used to read app insights metrics from the AppMetrics table.

Here is the keda-docs PR: kedacore/keda-docs#638

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

Fixes #1965

@markrzasa markrzasa mentioned this pull request Jan 23, 2022
Closed
@tomkerkhove
Copy link
Member

@markrzasa We're preparing a release for tomorrow. Do you think you can complete the PR before tomorrow in the late morning CET?

@tomkerkhove tomkerkhove changed the title Azure Application Insights Scaler feat: Provide Azure Application Insights Scaler Jan 26, 2022
@zroubalik
Copy link
Member

And I think that we need to have these env values defined in the GH secrets.

TEST_APP_INSIGHTS_APP_ID
TEST_APP_INSIGHTS_INSTRUMENTATION_KEY

@tomkerkhove
Copy link
Member

And I think that we need to have these env values defined in the GH secrets.

TEST_APP_INSIGHTS_APP_ID
TEST_APP_INSIGHTS_INSTRUMENTATION_KEY

Adding now in GitHub as secrets

@tomkerkhove
Copy link
Member

Before I do so, what identity is it using? The same as other Azure resources? If so, then I need @ahmelsayed to create an AI instance

@markrzasa
Copy link
Contributor Author

The code should match the documentation now.

JorTurFer
JorTurFer reviewed Jan 26, 2022
View reviewed changes
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.

Nice job with this scaler! 😃
Please, add a unit test (like this) to check that the metric names are generated correctly
Apart from that, some small nits

pkg/scalers/azure/azure_app_insights.go Show resolved Hide resolved
pkg/scalers/azure/azure_app_insights.go Outdated Show resolved Hide resolved
pkg/scalers/azure/azure_app_insights_test.go Outdated Show resolved Hide resolved
@markrzasa
Copy link
Contributor Author

should be all set now

@markrzasa
Copy link
Contributor Author

@markrzasa We're preparing a release for tomorrow. Do you think you can complete the PR before tomorrow in the late morning CET?

I believe all comments have been addressed. If everything looks good, please feel free to complete this PR. Otherwise, I'm not in a big rush to get this merged. I can wait until the next release.

@tomkerkhove tomkerkhove marked this pull request as ready for review January 27, 2022 06:41
@tomkerkhove tomkerkhove requested a review from a team as a code owner January 27, 2022 06:41
@tomkerkhove
Copy link
Member

Thank you for the PR, very smooth!

Before I do so, what identity is it using? The same as other Azure resources? If so, then I need @ahmelsayed to create an AI instance

In terms of this, can you elaborate for your e2e tests @markrzasa? We'll need to add a test resource but want to understand what identity it is using now.

Can you fix the DCO issues please? You can learn more in our contribution guide.

@tomkerkhove
Copy link
Member

We are pushing the release to next Monday so that should give us more time if we want

@markrzasa
Copy link
Contributor Author

Thank you for the PR, very smooth!

Before I do so, what identity is it using? The same as other Azure resources? If so, then I need @ahmelsayed to create an AI instance

In order to run the end-to-end app insights test, you'll need a service principal assigned the 'Monitoring Reader' role scoped to your application insights resource. This setup is likely similar to what is in place for the log analytics scaler.

In terms of this, can you elaborate for your e2e tests @markrzasa? We'll need to add a test resource but want to understand what identity it is using now.

Can you fix the DCO issues please? You can learn more in our contribution guide.

Yes. I'll fix that.

@tomkerkhove
Copy link
Member

And I think that we need to have these env values defined in the GH secrets.

TEST_APP_INSIGHTS_APP_ID
TEST_APP_INSIGHTS_INSTRUMENTATION_KEY

Added to GitHub repo's secrets.

@JorTurFer
Copy link
Member

And I think that we need to have these env values defined in the GH secrets.

TEST_APP_INSIGHTS_APP_ID
TEST_APP_INSIGHTS_INSTRUMENTATION_KEY

Added to GitHub repo's secrets.

Did you add them also to the main branch?

tomkerkhove added a commit that referenced this pull request Jan 27, 2022
@tomkerkhove
chore: Provide e2e secrets for Azure Application Insights scaler
cc3f1f2 
Provide e2e secrets for Azure Application Insights scaler.

Relates to #2506

Signed-off-by: Tom Kerkhove <[email protected]>
tomkerkhove added a commit that referenced this pull request Jan 27, 2022
@tomkerkhove
Update nightly-e2e.yml
94f84d7 
Provide e2e secrets for Azure Application Insights scaler.

Relates to #2506

Signed-off-by: Tom Kerkhove <[email protected]>
tomkerkhove added a commit that referenced this pull request Jan 27, 2022
@tomkerkhove
Update main-build.yml
d862282 
Provide e2e secrets for Azure Application Insights scaler.

Relates to #2506

Signed-off-by: Tom Kerkhove <[email protected]>
@tomkerkhove tomkerkhove mentioned this pull request Jan 27, 2022
Merged
1 task
@tomkerkhove
Copy link
Member

See #2568, ignore PredictKube as I created branch off of the last commit when I added them (sorry)

@JorTurFer
Copy link
Member

JorTurFer commented Jan 27, 2022 •
edited by github-actions bot
Loading

/run-e2e azure-app-insights.*
Update: You can check the progres here

@zroubalik
Copy link
Member

@markrzasa could you please rebase your PR, so it contains only relevant commits, it is hard to do a review :)

@markrzasa
Copy link
Contributor Author

@markrzasa could you please rebase your PR, so it contains only relevant commits, it is hard to do a review :)

I don't mind doing that, but I could use a little help. Is there a specific set of steps or some doc that you would recommend to help me rebase?

@markrzasa
Copy link
Contributor Author

@markrzasa could you please rebase your PR, so it contains only relevant commits, it is hard to do a review :)

I don't mind doing that, but I could use a little help. Is there a specific set of steps or some doc that you would recommend to help me rebase?

I rebased the PR. Hopefully, I handled it correctly.

@markrzasa
Copy link
Contributor Author

/run-e2e azure-app-insights.* Update: You can check the progres here

It looks like the test may need a little more time to scale. I've updated the test to wait a little longer.

markrzasa and others added 2 commits January 27, 2022 16:29
@markrzasa
parent ba37a17
cb478c3 
author Mark Rzasa <[email protected]> 1642970493 -0500
committer Mark Rzasa <[email protected]> 1643317483 -0500

app insights scaler

Signed-off-by: Mark Rzasa <[email protected]>

Add PredictKube scaler (kedacore#2418)

Signed-off-by: Daniel Yavorovych <[email protected]>
Signed-off-by: alex60217101990 <[email protected]>

bump deps (kedacore#2563)

Signed-off-by: Zbynek Roubalik <[email protected]>

Clear scalers cache correctly both in Operator and Metrics Server (kedacore#2564)

Signed-off-by: Zbynek Roubalik <[email protected]>

test: Provide e2e secrets for Azure Application Insights scaler (kedacore#2568)

Signed-off-by: Tom Kerkhove <[email protected]>
Signed-off-by: Mark Rzasa <[email protected]>
@markrzasa
give app insights scaler test a little more time to scale
4bd4907 
Signed-off-by: Mark Rzasa <[email protected]>
@markrzasa
Copy link
Contributor Author

I think I've botched this PR. I see a lot of changes in files that have nothing to do with app insights. Should I discard the PR and start a new one?

zroubalik
zroubalik reviewed Jan 28, 2022
View reviewed changes
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.

Looking good, just a minor nits

pkg/scalers/azure/azure_app_insights.go Outdated Show resolved Hide resolved
pkg/scalers/azure/azure_app_insights.go Show resolved Hide resolved
pkg/scalers/azure_app_insights_scaler.go Outdated Show resolved Hide resolved
@zroubalik zroubalik added this to the v2.6.0 milestone Jan 28, 2022
@zroubalik
Copy link
Member

zroubalik commented Jan 28, 2022 •
edited by github-actions bot
Loading

/run-e2e azure-app-insights.*
Update: You can check the progres here

@markrzasa
Copy link
Contributor Author

here

Looks good. Thanks for re-running that.

@markrzasa
group keda deps together
1693315 
Signed-off-by: Mark Rzasa <[email protected]>
zroubalik
zroubalik approved these changes Jan 28, 2022
View reviewed changes
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

@tomkerkhove tomkerkhove merged commit f3b97b2 into kedacore:main Jan 30, 2022
@tomkerkhove
Copy link
Member

Thanks a ton @markrzasa, I love this one!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JorTurFer JorTurFer JorTurFer approved these changes

@zroubalik zroubalik zroubalik approved these changes

@tomkerkhove tomkerkhove Awaiting requested review from tomkerkhove

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.6.0
Development

Successfully merging this pull request may close these issues.

Problem with fetching Azure custom metric through Azure Monitor scaler
4 participants
@markrzasa @tomkerkhove @zroubalik @JorTurFer