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

Feature/splunk observability scaler #6192

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sschimper-splunk
Copy link

@sschimper-splunk sschimper-splunk commented Sep 26, 2024

With this pull request, I would like to add a new custom KEDA scaler that interacts with the Splunk Observability Cloud Platform. It is able to query metrics from Splunk Observability Cloud and scale a deployment according to a predefined target value.

As for now, I do not have the created a pull request to update the Helm chart, becasue I did not think it necessary. However, my knowledge about Helm charts is admittedly limited, and I am happy to fix this in hindsight if that is necessary.
Thank you.

Checklist

Relates to:

  • Initial proposal, Fixes #6190
  • Pull request containing the documentation on this scaler: #1477

@sschimper-splunk sschimper-splunk requested a review from a team as a code owner September 26, 2024 08:20
@circa10a
Copy link
Contributor

The only files we should be changing under pkg/ in this PR is scalers/ and scaling/. We should remove the other changes introduced.

name: splunk-secrets
namespace: {{.TestNamespace}}
data:
accessToken: YW1JeUpqVHRJd185cDhOWG01X21KQQ== # one time through-away access token used just for testing
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this test relies on an actual upstream being available to communicate with. Would it be possible to create a pod here that simply mocks responses? We could override the endpoint in the scaler config and point it to our mocked API. This would ensure the tests could still run in a more closed loop fashion without any upstream dependencies. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

agree

kedautil "github.com/kedacore/keda/v2/pkg/util"
)

type splunkObservabilityMetadata struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we support a parameter to override an endpoint in case that changes in the future?

Copy link
Author

Choose a reason for hiding this comment

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

Note to self: TODO base url overwriting

Copy link
Member

Choose a reason for hiding this comment

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

do we expect the endpoint to be changed? if so we can add the parameter.

pkg/scalers/splunk_observability_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/splunk_observability_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/splunk_observability_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/splunk_observability_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/splunk_observability_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/splunk_observability_scaler.go Show resolved Hide resolved
pkg/scalers/splunk_observability_scaler.go Show resolved Hide resolved
@circa10a
Copy link
Contributor

We should probably set this PR to WIP/Draft state

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 for the contribution! could of points

  • helm update is not needed
  • please fix DCO

go.mod Show resolved Hide resolved
@@ -16,7 +16,7 @@
// Code generated by protoc-gen-go. DO NOT EDIT.
// versions:
// protoc-gen-go v1.34.2
// protoc v5.27.3
// protoc v4.25.2
Copy link
Member

Choose a reason for hiding this comment

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

please don't include these changes in the pb files, not related

kedautil "github.com/kedacore/keda/v2/pkg/util"
)

type splunkObservabilityMetadata struct {
Copy link
Member

Choose a reason for hiding this comment

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

do we expect the endpoint to be changed? if so we can add the parameter.

pkg/scalers/splunk_observability_scaler.go Show resolved Hide resolved
pkg/scalers/splunk_observability_scaler.go Show resolved Hide resolved
name: splunk-secrets
namespace: {{.TestNamespace}}
data:
accessToken: YW1JeUpqVHRJd185cDhOWG01X21KQQ== # one time through-away access token used just for testing
Copy link
Member

Choose a reason for hiding this comment

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

agree

@sschimper-splunk
Copy link
Author

Thank you for reviewing, @zroubalik. I will work during the next days on fixing the things pointed out.

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.

Thank you for reviewing, @zroubalik. I will work during the next days on fixing the things pointed out.

@sschimper-splunk any updates here please? We will cut a new release Nov 7, so if you want this included in KEDA 2.16 we should resolve this asap. If you don't have we can release this in the following release, not a problem :)

@sschimper-splunk
Copy link
Author

Hi @zroubalik,
thanks for the heads-up. Unfortunately, I will not be ready for the new release on November 7th, and I ask you to go ahead without this Splunk Observability Scaler.
I work towards being ready for the next release after November 7th. Apologies if I slowed things down.

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.

Support Splunk Observability Cloud scaler
3 participants