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

Add Splunk as a metrics provider #1733

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

kane8n
Copy link
Contributor

@kane8n kane8n commented Nov 28, 2024

This PullRequest adds Splunk as a metrics provider.
It has been tested on my cluster.

Copy link
Member

@aryan9600 aryan9600 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 opening this PR! left a few comments

flaggerv1 "github.com/fluxcd/flagger/pkg/apis/flagger/v1beta1"
)

// https://docs.datadoghq.com/api/
Copy link
Member

Choose a reason for hiding this comment

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

please change this to splunk's api docs


sp := SplunkProvider{
timeout: 5 * time.Second,
metricsQueryEndpoint: strings.Replace(strings.Replace(address+signalFxSignalFlowApiPath, "http", "ws", 1), "api", "stream", 1),
Copy link
Member

Choose a reason for hiding this comment

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

could you add some context here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The library used to handle the signalflow api uses websockets to connect, so the address is converted.
I've added a COMMENT.


signalFxTokenHeaderKey = "X-SF-Token"

signalFxFromDeltaMultiplierOnMetricInterval = 10
Copy link
Member

Choose a reason for hiding this comment

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

could you specify the purpose of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I referred to the datadog implementation.
In some cases, the splunk api could only acquire empty data if the specified period was short, so to ensure that data is acquired, a period 10 times longer than the set interval is used.

pkg/metrics/providers/splunk.go Show resolved Hide resolved
@kane8n
Copy link
Contributor Author

kane8n commented Dec 1, 2024

@aryan9600
Thanks for the review!
I added some comments.

@kane8n kane8n requested a review from aryan9600 December 1, 2024 07:45
@katzchang
Copy link

Hi reviewer team @stefanprodan @aryan9600,
I'm also waiting for this in review, is there anything we need to do to move the situation forward?

@@ -0,0 +1,195 @@
/*
Copyright 2020 The Flux authors
Copy link
Member

Choose a reason for hiding this comment

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

This should be Copyright 2024 in all new files added in this PR

Comment on lines 764 to 767
query: |
total = data('traces.count', filter=filter('sf_service', 'my-service-primary', 'my-service')).sum().publish(enable=False)
success = data('traces.count', filter=filter('sf_service', 'my-service-primary', 'my-service') and filter('sf_error', 'false')).sum().publish(enable=False)
((success/total) * 100).publish()
Copy link
Member

@stefanprodan stefanprodan Dec 12, 2024

Choose a reason for hiding this comment

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

@kane8n did you tested this with Flagger? By the looks of it, this includes the primary service traces, but Flagger should look at the canary only. Also we should be using the template variables here.

@kane8n
Copy link
Contributor Author

kane8n commented Dec 13, 2024

@stefanprodan
Thanks for the review!
I followed your advice and modified the sample query.
It has been tested with flaager in my cluster.

@kane8n kane8n requested a review from stefanprodan December 13, 2024 07:00
@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 65.74074% with 37 lines in your changes missing coverage. Please review.

Project coverage is 39.24%. Comparing base (8f83838) to head (d4bd0f2).
Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
pkg/metrics/providers/splunk.go 66.98% 25 Missing and 10 partials ⚠️
pkg/metrics/providers/factory.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1733      +/-   ##
==========================================
+ Coverage   35.08%   39.24%   +4.15%     
==========================================
  Files         283      284       +1     
  Lines       24653    22354    -2299     
==========================================
+ Hits         8650     8773     +123     
+ Misses      15063    12634    -2429     
- Partials      940      947       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stefanprodan
Copy link
Member

@kane8n please squash all commits into one

Copy link
Member

@aryan9600 aryan9600 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 @kane8n 🎖️

Signed-off-by: kane8n <[email protected]>
@kane8n kane8n force-pushed the add-splunk-provider branch from d0a0852 to d4bd0f2 Compare December 13, 2024 13:23
@kane8n
Copy link
Contributor Author

kane8n commented Dec 13, 2024

@stefanprodan

please squash all commits into one

done!

@stefanprodan stefanprodan added the area/metrics Metric analysis related issues and PRs label Dec 13, 2024
Copy link
Member

@stefanprodan stefanprodan 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 @kane8n

@stefanprodan stefanprodan merged commit 0138e2e into fluxcd:main Dec 13, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/metrics Metric analysis related issues and PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants