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

feat: datadog metric provider for KLT #948

Merged
merged 17 commits into from
Mar 20, 2023

Conversation

sudiptob2
Copy link
Member

@sudiptob2 sudiptob2 commented Mar 1, 2023

Fixes #554

Due to a breaking change I have closed my previous draft and opened this updated one.

I am hoping to get your opinion on DataDog APIs. I'm still getting the hang of things and would appreciate your input on what I've learned so far and where to go next. As a newcomer to DataDog, I'm eager to soak up as much knowledge as possible, so any assistance you can provide would be awesome 👍

Progress

  • I have looked into the dyantrace.go and prometheus.go files and tried to understand EvaluateQuery function.
  • Similar to the above implementation the goal datadog.go is to fetch the SLIs from the DataDog API.
  • Also using this API we can query time series data from data dog.
  • I looked into the @vadasambar's code (here) and found some logic for fetching the metric data and preparing Sliresult ✔️
  • I have tried Go Client for DD which was suggested in the issue description page. Unfortunately, it has some limitations.
    • I could not mock the target-server
    • According to the docs you can change the target-server like this. However it does not work as you can only choose target-server among "datadoghq.com", "us3.datadoghq.com", "us5.datadoghq.com", "datadoghq.eu", "ddog-gov.com"
    • It limits the possibility to test the provider using mock server (similar to dynatrace provider)
  • I proceed with httpclient

Suggestion required

  • ✔️ How do I get 2 keys (APP_KEY and API_KEY) from Kubernetes secret? KeptnMetricsProviderSpec can handle only one SecretKeyRef, I am not sure about the syntax on how to handle 2 keys here. Update: marked SecretKeySelector as optional. Now e can expect as many keys as we need in the Kubernetes secret.
  • To get a single value from the metric API I used this logic found in keptn-sandbox/datadog-service
  • I saw in dynatrace implementation, a getSingleValue is implemented. It basically, returns the average of the metric value. Therefore, I think the current logic might need some amendments. Please suggest.

I need suggestions and mentorship to successfully implement this. I would appreciate any reference materials or comment.

@sudiptob2
Copy link
Member Author

sudiptob2 commented Mar 1, 2023

Update: marked SecretKeySelector as optional. Now e can expect as many keys as we need in the Kubernetes secret.


I have made some improvements and can fetch the metric data from the API. If you look at the code here, I have to provide the apiKeyAuth and appKeyAuth

I am planning to get these keys from Kubernetes secret. I found some reference in dynatrace implementation here.
In the dynatrace_test.go we have some examples of adding secrets. Ref: here

p := metricsapi.KeptnMetricsProvider{
		Spec: metricsapi.KeptnMetricsProviderSpec{
			SecretKeyRef: v1.SecretKeySelector{
				LocalObjectReference: v1.LocalObjectReference{
					Name: "myapitoken",
				},
				Key: "mykey",
			},
			TargetServer: svr.URL,
		},
	}

Basically I am trying to write datadog_test.go and do something like the above code for datadog (see here). The problem is for datadog, I need to pass 2 keys. I need help on how I can pass 2 secrets (DD_API_KEY and DD_APP_KEY) using the KeptnMetricsProviderSpec

I would appreciate it If anyone can give me some hint.

@sudiptob2 sudiptob2 force-pushed the feat/554/datadog-metric-provider branch from 7e2bd93 to 4d16296 Compare March 4, 2023 07:40
@netlify
Copy link

netlify bot commented Mar 4, 2023

Deploy Preview for keptn-lifecycle-toolkit ready!

Name Link
🔨 Latest commit 7be4ac9
🔍 Latest deploy log https://app.netlify.com/sites/keptn-lifecycle-toolkit/deploys/6405d1aabac83b0008b29797
😎 Deploy Preview https://deploy-preview-948--keptn-lifecycle-toolkit.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@sudiptob2 sudiptob2 force-pushed the feat/554/datadog-metric-provider branch from 4d16296 to 2e6ed2b Compare March 4, 2023 08:15
@sudiptob2 sudiptob2 marked this pull request as ready for review March 4, 2023 08:22
…lient library due to its limitation

Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>
Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>
Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>
…alidation

Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>
Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>
@sudiptob2 sudiptob2 force-pushed the feat/554/datadog-metric-provider branch from 2e6ed2b to 7be4ac9 Compare March 6, 2023 11:42
@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Merging #948 (a30c132) into main (d56bfa4) will increase coverage by 0.51%.
The diff coverage is 80.64%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #948      +/-   ##
==========================================
+ Coverage   57.50%   58.02%   +0.51%     
==========================================
  Files         119      128       +9     
  Lines        9914    10210     +296     
==========================================
+ Hits         5701     5924     +223     
- Misses       4015     4072      +57     
- Partials      198      214      +16     
Impacted Files Coverage Δ
...or/controllers/common/providers/datadog/datadog.go 76.92% <76.92%> (ø)
...tor/controllers/common/providers/datadog/common.go 86.36% <86.36%> (ø)
...-operator/controllers/common/providers/provider.go 80.76% <100.00%> (+5.76%) ⬆️

... and 15 files with indirect coverage changes

Flag Coverage Δ
certificate-operator 64.15% <ø> (ø)
component-tests 53.25% <ø> (+2.32%) ⬆️
lifecycle-operator 53.63% <ø> (ø)
metrics-operator 63.96% <80.64%> (+0.97%) ⬆️
scheduler 21.17% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@RealAnna
Copy link
Contributor

RealAnna commented Mar 6, 2023

@sudiptob2 as you can see we have a picky linter complaining about how your import are sorted https://github.com/keptn/lifecycle-toolkit/actions/runs/4343018946

usually, this can be automagically fixed by running
run --fix --disable=typecheck --config ../.golangci.yml -v

we have a brief explanation of this in our CONTRIBUTING.md

Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>
Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>
Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>
@RealAnna
Copy link
Contributor

RealAnna commented Mar 7, 2023

codecoverage pipeline fails, this is because the provider is not in this test here https://github.com/sudiptob2/lifecycle-toolkit/blob/feat/554/datadog-metric-provider/metrics-operator/controllers/common/providers/provider_test.go

@sudiptob2
Copy link
Member Author

codecoverage pipeline fails, this is because the provider is not in this test here https://github.com/sudiptob2/lifecycle-toolkit/blob/feat/554/datadog-metric-provider/metrics-operator/controllers/common/providers/provider_test.go

thanks, a great catch, I missed that. I will update it. Also, I am addressing the other issues you suggested. 🙏

Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>
Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>
Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>
Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>
Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>
Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>
Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>
@sudiptob2 sudiptob2 requested a review from RealAnna March 7, 2023 19:59
@thisthat thisthat self-assigned this Mar 8, 2023
Copy link
Member

@thisthat thisthat left a comment

Choose a reason for hiding this comment

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

Great job @sudiptob2 🙌
I have only a minor remark/question

Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>
Copy link
Contributor

@RealAnna RealAnna left a comment

Choose a reason for hiding this comment

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

as per https://github.com/keptn/lifecycle-toolkit/pull/948/files#r1129308074
it would be good to implement an avg

@sudiptob2
Copy link
Member Author

as per https://github.com/keptn/lifecycle-toolkit/pull/948/files#r1129308074 it would be good to implement an avg

hmm, thanks for the confirmation, working on it.

Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>
@sonarcloud
Copy link

sonarcloud bot commented Mar 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sudiptob2 sudiptob2 requested review from thisthat and RealAnna and removed request for thisthat March 9, 2023 11:12
Copy link
Member

@thisthat thisthat left a comment

Choose a reason for hiding this comment

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

LGTM. Great work @sudiptob2 🙌

@thisthat thisthat dismissed RealAnna’s stale review March 14, 2023 20:27

changes addressed

Copy link
Contributor

@thschue thschue left a comment

Choose a reason for hiding this comment

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

lgtm

@thisthat thisthat merged commit 597a23f into keptn:main Mar 20, 2023
@keptn-bot keptn-bot mentioned this pull request Mar 22, 2023
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.

Add support for DataDog as Evaluation Provider
4 participants