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

Splunk Observability Scaler - Initial PR #5910

Closed

sschimper-splunk wants to merge 7 commits into kedacore:main from sschimper-splunk:dev

sschimper-splunk commented Jun 26, 2024

Provide a description of what has been changed

Checklist

When introducing a new scaler, I agree with the scaling governance policy
I have verified that my change is according to the deprecations & breaking changes policy
Tests have been added
Changelog has been updated and is aligned with our changelog requirements
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)
Commits are signed with Developer Certificate of Origin (DCO - learn more)

Fixes #

Relates to #

sschimper-splunk added 7 commits

February 15, 2024 11:47


          feat: added Splunk O11y scaler

49a2fdf


          edited o11y scaler

e731167


          corrected vendoring

90cad3e


          fixed bug with access token not being found

5be4d82


          removed log statement showing access token

2ad598a


          added enhanced logging

8e528d2


          removed debug statements

cbfc4af

sschimper-splunk requested a review from a team as a code owner

June 26, 2024 13:46

iwankgb reviewed

View reviewed changes

Makefile

               .PHONY: e2e-test-clean
               e2e-test-clean: get-cluster-context ## Delete all namespaces labeled with type=e2e
-              	kubectl delete ns -l type=e2e
+              	microk8s kubectl delete ns -l type=e2e

iwankgb Jun 26, 2024

I guess that you need to remove all the changes from this file.

Author

sschimper-splunk Jul 15, 2024

Hi, thanks for pointing that out. I manually added "microk8s" to be able to build the project with my MicroK8s cluster that I have set up.

config/manager/kustomization.yaml

    
            @@ -6,5 +6,5 @@ apiVersion: kustomize.config.k8s.io/v1beta1
          
              kind: Kustomization

              images:

              - name: ghcr.io/kedacore/keda

                newName: ghcr.io/kedacore/keda

                newName: docker.io/sschimpersplunk/keda

iwankgb Jun 26, 2024

It should point to official container image.

config/metrics-server/kustomization.yaml

@@ @@ -10,5 +10,5 @@ apiVersion: kustomize.config.k8s.io/v1beta1 @@
               kind: Kustomization
               images:
               - name: ghcr.io/kedacore/keda-metrics-apiserver
-                newName: ghcr.io/kedacore/keda-metrics-apiserver

iwankgb Jun 26, 2024

It should point to official container image.

config/webhooks/kustomization.yaml

@@ @@ -7,5 +7,5 @@ apiVersion: kustomize.config.k8s.io/v1beta1 @@
               kind: Kustomization
               images:
               - name: ghcr.io/kedacore/keda-admission-webhooks
-                newName: ghcr.io/kedacore/keda-admission-webhooks

iwankgb Jun 26, 2024

It should point to official container image.

go.mod

@@ @@ -77,7 +77,7 @@ require ( @@
               	github.com/segmentio/kafka-go/sasl/aws_msk_iam_v2 v0.1.0
               	github.com/spf13/cast v1.6.0
               	github.com/spf13/pflag v1.0.5
-              	github.com/stretchr/testify v1.8.4
+              	github.com/stretchr/testify v1.9.0

iwankgb Jun 26, 2024

Avoid changes unrelated to your PR, please. Leave testify in original version :)

Author

sschimper-splunk Jul 15, 2024

Understood, will do.

pkg/scalers/splunk_o11y_scaler.go

+              		Program: s.metadata.query,
+              	})
+              	if err != nil {
+              		return -1, fmt.Errorf("error: could not execute signalflow query: %w", err)

iwankgb Jun 26, 2024

You should return zero value (0.0 in this case) when returning an error.

pkg/scalers/splunk_o11y_scaler.go

Comment on lines +167 to +181

+              	var duration time.Duration = 10000000000 // ten seconds in nano seconds
+              	comp, err := s.apiClient.Execute(context.Background(), &signalflow.ExecuteRequest{
+              		Program: s.metadata.query,
+              	})
+              	if err != nil {
+              		return -1, fmt.Errorf("error: could not execute signalflow query: %w", err)
+              	}
+              	go func() {
+              		time.Sleep(duration)
+              		if err := comp.Stop(context.Background()); err != nil {
+              			s.logger.Info("Failed to stop computation")
+              		}
+              	}()

iwankgb Jun 26, 2024

You should rather use WithTimeout():

Suggested change

      
            	var duration time.Duration = 10000000000 // ten seconds in nano seconds
          
            	comp, err := s.apiClient.Execute(context.Background(), &signalflow.ExecuteRequest{
          
            		Program: s.metadata.query,
          
            	})
          
            	if err != nil {
          
            		return -1, fmt.Errorf("error: could not execute signalflow query: %w", err)
          
            	}
          
            	go func() {
          
            		time.Sleep(duration)
          
            		if err := comp.Stop(context.Background()); err != nil {
          
            			s.logger.Info("Failed to stop computation")
          
            		}
          
            	}()
          
            	var duration time.Duration = 10000000000 // ten seconds in nano seconds
          
                    xCtx, cancel := context.WithTimeout(ctx, duration)
          
                    defer cancel()
          
            	comp, err := s.apiClient.Execute(xCtx, &signalflow.ExecuteRequest{
          
            		Program: s.metadata.query,
          
            	})
          
            	if err != nil {
          
            		return 0.0, fmt.Errorf("error: could not execute signalflow query: %w", err)
          
            	}

Author

sschimper-splunk Jul 15, 2024

Yes, that makes sense. I will update the function accordingly.

pkg/scalers/splunk_o11y_scaler.go

Comment on lines +217 to +229

+              	switch s.metadata.queryAggegrator {
+              	case "max":
+              		logMessage(s.logger, "Returning max value ", max)
+              		return max, nil
+              	case "min":
+              		logMessage(s.logger, "Returning min value ", min)
+              		return min, nil
+              	case "avg":
+              		avg := valueSum / float64(valueCount)
+              		logMessage(s.logger, "Returning avg value ", avg)
+              		return avg, nil
+              	default:
+              		return max, nil

iwankgb Jun 26, 2024

Can these aggregations be performed as part of the Splunk query? I think it would be more reliable to use server side logic.

Author

sschimper-splunk Jul 15, 2024

I will look into that.

pkg/scalers/splunk_o11y_scaler.go

Comment on lines +190 to +211

+              	for msg := range comp.Data() {
+              		s.logger.Info("getQueryResult -> msg: %+v\n", msg)
+              		if len(msg.Payloads) == 0 {
+              			s.logger.Info("getQueryResult -> No data retreived. Continuing")
+              			continue
+              		}
+              		for _, pl := range msg.Payloads {
+              			value, ok := pl.Value().(float64)
+              			if !ok {
+              				return -1, fmt.Errorf("error: could not convert Splunk Observability metric value to float64")
+              			}
+              			logMessage(s.logger, "Encountering value ", value)
+              			if value > max {
+              				max = value
+              			}
+              			if value < min {
+              				min = value
+              			}
+              			valueSum += value
+              			valueCount++
+              		}
+              	}

iwankgb Jun 26, 2024

If you decide to use Splunk-side aggregation, then this loop would be significantly simpler and you should expect one and only one value.

pkg/scalers/splunk_o11y_scaler.go

+              	}
+              	metric := GenerateMetricInMili(metricName, num)
+              	logMessage(s.logger, "num", num)

iwankgb Jun 26, 2024

num will never equal -1 here (-1 is returned from s.getQueryResult() only when error is returned and such a scenario id handled in line 237).

Member

JorTurFer commented Jul 22, 2024

Is this still active?

Author

sschimper-splunk commented Jul 23, 2024

Hi @JorTurFer,
I will clarify this and get back to you.

iwankgb commented Jul 23, 2024

Is this still active?

I hope so :) Sebastian was off for a while as far as I understand.

sschimper-splunk changed the title ~~Test PR~~ Splunk Observability Scaler - Initial PR

Member

JorTurFer commented Jul 24, 2024

There is other PR merged supporting Splunk -> #5905
Is this a different feature or so?

Author

sschimper-splunk commented Jul 24, 2024

Hi @JorTurFer, this seems to be independent of our work on a Scaler for Splunk Observability.

Member

JorTurFer commented Jul 24, 2024

Could we extend the current Splunk scaler instead of creating a totally new one? (IDK if it's possible or not tbh)

Author

sschimper-splunk commented Jul 24, 2024

Yes, I'd even think its for the better.

Member

JorTurFer commented Jul 25, 2024

Yes, I'd even think its for the better.

Thanks ❤️ by chance, 2 different contributions related with spluk have been done at the same time (this and the other already merged) xD.

We want to cut a release during the next week, if the PR is merged before Thursday, it'll be released next week (no rush at all, if it's not during this release, it'll be in the next one)

zroubalik reviewed

View reviewed changes

Member

zroubalik left a comment

Any updates on this please Shall we extend the current Splunk scaler?

Author

sschimper-splunk commented Aug 8, 2024

Hi @zroubalik,
the work on a Splunk Keda scaler on the mentioned PR was regarding our Splunk Enterprise/Cloud solutions. I would like to work towards a Keda scaler for Splunk Observability Cloud.
If that is alright with you, I'd like to submit another PR in a "proper fashion", and therefore we can delete this PR.
Thank you, and apologies for all the confusion.

sschimper-splunk closed this by deleting the head repository

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

Labels

None yet