feat: Add datadog metric provider. Fixes #702 #705

stephen-harris · 2020-09-07T08:28:43Z

Checklist:

Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
I've signed the CLA.
I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
My builds are green. Try syncing with master if they are not.
My organization is added to USERS.md.

CLAassistant · 2020-09-07T08:28:49Z

All committers have signed the CLA.

codecov-commenter · 2020-09-07T09:25:24Z

Codecov Report

Merging #705 into master will decrease coverage by 1.62%.
The diff coverage is 68.18%.

@@            Coverage Diff             @@
##           master     #705      +/-   ##
==========================================
- Coverage   84.51%   82.88%   -1.63%     
==========================================
  Files          95       96       +1     
  Lines        8988     8034     -954     
==========================================
- Hits         7596     6659     -937     
+ Misses        983      964      -19     
- Partials      409      411       +2

Impacted Files	Coverage Δ
metricproviders/datadog/datadog.go	`67.44% <67.44%> (ø)`
utils/analysis/factory.go	`100.00% <100.00%> (ø)`
utils/unstructured/unstructured.go	`57.77% <0.00%> (-26.23%)`	⬇️
rollout/context.go	`90.00% <0.00%> (-8.57%)`	⬇️
utils/log/redactor_formatter.go	`66.66% <0.00%> (-8.34%)`	⬇️
rollout/replicaset.go	`57.81% <0.00%> (-8.02%)`	⬇️
utils/tolerantinformer/rollout.go	`67.85% <0.00%> (-5.12%)`	⬇️
utils/tolerantinformer/experiment.go	`67.85% <0.00%> (-5.12%)`	⬇️
utils/tolerantinformer/analysisrun.go	`67.85% <0.00%> (-5.12%)`	⬇️
utils/tolerantinformer/analysistemplate.go	`67.85% <0.00%> (-5.12%)`	⬇️
... and 88 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1fd695...e7b5bae. Read the comment docs.

jessesuen · 2020-09-09T08:10:03Z

I think we need to treat DataDog API Keys and App Keys more first class in the metric provider spec. For reference, this is how we do something similar wavefront:
https://argoproj.github.io/argo-rollouts/features/analysis/#wavefront-metrics

I've read through https://docs.datadoghq.com/account_management/api-app-keys/, but I still have some questions on where would be an appropriate place to configure API keys and app keys.

With Wavefront, a single API token can be specified for an entire domain, e.g. example1.wavefront.com. For this reason, API tokens are configured at the rollout controller level, in a secret in the same namespace of argo-rollouts controller. The rollout end users do not need to specify API keys when using the wavefront metric provider, since this is configured at the controller.

Since I don't understand DataDog, can you answer some questions about it:

Does it make more sense to configure DataDog API keys and/or App keys at a controller level? With wavefront, it made sense to configure it at a controller level, since the same token could be used for the entire wavefront address. By having centrally configured keys, it relieved the burden from developers having to configure and duplicate keys in every one of their application's namespace. This would be a pain to manage, especially with a lot of namespaces.
What is the scope of API keys and App keys? In a large organization, who (account administrators, developers, operators) typically generates API keys vs. App keys? How many of either of these keys typically exist? I see from their documentation, "Your org must have at least one API key and at most five API keys." I'm not sure how large an "org" is in this context, but the statement leads me to believe that API keys are quite limited, and would lend itself to be centrally configured vs. configured at a namespace secret.

pkg/apis/rollouts/v1alpha1/analysis_types.go

metricproviders/datadog/datadog.go

metricproviders/datadog/datadog_test.go

stephen-harris · 2020-09-11T10:17:26Z

Hi @jessesuen I'll make those changes over the weekend, but to answer your questions around DataDog:

I wasn't aware of the 5-API key limit. Our organisation is a heavy datadog user and we appear to have had that limit waivered as we have over 200 API keys (each team has a key per environment, and some are using different keys for different servcies). In our org each team is encouraged to create their own API & App keys, but not through DataDog directly (presumably because only admins can create API keys)

Application keys are tied to a user, so I imagine even in other organisations they are creating app keys per service.

Just checking the issues, I think @johnkost & @ozooxo might be using DataDog - I don't know if they want to weigh in here.

johnkost · 2020-09-11T12:36:59Z

We use DataDog as well and haven't had any issues with the 5-API key limit (roughly same setup as @stephen-harris).

I think it makes sense to have this setup similarly at the controller level as Argo-Rollouts is an application, just like any of the other apps we have.

stephen-harris · 2020-09-14T16:09:15Z

If anyone wants to try this out, I've published an image which forks off of 0.9.0: https://hub.docker.com/layers/stephenharris13/argo-rollouts/0.9.0-datadog-bbf9c9b/images/sha256-8151ef6208e792eacd19d476f238c0ed605a8c250f75faea1e4d766c993d91bc?context=explore

metricproviders/datadog/datadog_test.go

jessesuen · 2020-09-22T05:48:12Z

docs/features/analysis.md

+  name: wavefront-api-tokens
+type: Opaque
+data:
+  api-key: <datadog-api-key>
+  app-key: <datadog-app-key>


docs still say "wavefront-api-tokens". I think this should be datadog-auth-keys

don't we need to allow multiple API/APP keys to be configured?

(1) - oops! It should be datadog. Fixed now

(2) - I don't thinks so. All key pairs are assigned to the organisation, and using them you can pull any metrics from your organisation. Our organisation encourages teams to own their own keys, but team a can use its keys to access metrics reported by team b. We could make the secret name configurable, and you specify it as an AnalysisTemplate parameter, but I don't see the value in doing that.

OK I'll take your word for it, but I'll also pose the question in #argo-rollouts slack in case there's users who may need to configure multiple, in case there's something we're missing.

Just to be clear:

With the proposed spec, we allow the DataDog address to be specified in the metric (so it can vary between AnalysisTemplates), but API/APP keys are configured centrally in the controller. This presumes that the same API/APP key can be used for different DataDog URLs. This may very well be possible with DataDog (I don't know the answer to this), but I wanted to call that out as a potential flaw.

That's a good point. No API/APP keys are not cross-site. In fact I believe an organisation is in either the EU or US site. Although not a secret, for ease of configuration we could move these to the datadog secret (alongside app_key and api_key). What do you think?

Yes. I agree we should make the address and api/app keys centrally configured, and remove the address field from the metric. I'm thinking the Secret should look something like:

data: default: | address: https://api.datadoghq.com api-key: igq7imbxlm97vnlw61bo app-key: 35bu8ofvc0osyjmqxtgi

This also leaves the door open so if in the future, multiple addresses and/or app keys could to be specified in the same cluster. e.g.:

data: us-app1: | address: https://api.datadoghq.com api-key: igq7imbxlm97vnlw61bo app-key: 35bu8ofvc0osyjmqxtgi us-app2: | address: https://api.datadoghq.com api-key: igq7imbxlm97vnlw61bo app-key: cb7ehxo1jwagyceth8v8 eu: | address: https://api.datadoghq.eu api-key: cb7ehxo1jwagyceth8v8 app-key: timk0do15pafuvmfwimg

And the names us-app1 and us-app2 could be referenced in the metric.

But for MVP, for simplicity, we can go out with a single configured DataDog region and single api/app key combination.

I've implemented this, though I'm starting to think perhaps we should have just one level (e.g. a secret per app/region), and then reference the name of the secret in the metric.

apiVersion: v1 kind: Secret metadata: name: datadog data: address: https://api.datadoghq.com api-key: igq7imbxlm97vnlw61bo app-key: 35bu8ofvc0osyjmqxtgi --- apiVersion: v1 kind: Secret metadata: name: datadog-app2 data: address: https://api.datadoghq.eu api-key: cb7ehxo1jwagyceth8v8 app-key: timk0do15pafuvmfwimg

Not too fussed though; I think in most use cases organisations will only have one region, and will only have key pair used by rollouts.

though I'm starting to think perhaps we should have just one level (e.g. a secret per app/region), and then reference the name of the secret in the metric.

Actually I agree with you. A secret per app/region sounds better as it doesn't put a special datastructure in the secret payload. Then things like kustomize patching the secret will just work. Let's go with your suggestion, but we don't have to do anything yet with regards to referencing until someone has a need to reference multiple regions.

metricproviders/datadog/datadog.go

jessesuen · 2020-10-06T08:21:43Z

metricproviders/datadog/datadog.go

+
+func NewDatadogProvider(logCtx log.Entry, kubeclientset kubernetes.Interface) (*Provider, error) {
+	ns := Namespace()
+	secret, err := kubeclientset.CoreV1().Secrets(ns).Get(DatadogTokensSecretName, metav1.GetOptions{})


FYI I updated the k8s libraries from underneath you, which now require context objects passed into every call. You can just use context.TODO() for now.

…f multiple key pairs

codecov-io · 2020-10-09T20:23:42Z

Codecov Report

Merging #705 into master will decrease coverage by 0.10%.
The diff coverage is 73.62%.

@@            Coverage Diff             @@
##           master     #705      +/-   ##
==========================================
- Coverage   82.66%   82.56%   -0.11%     
==========================================
  Files          95       96       +1     
  Lines        8071     8162      +91     
==========================================
+ Hits         6672     6739      +67     
- Misses        997     1013      +16     
- Partials      402      410       +8

Impacted Files	Coverage Δ
metricproviders/datadog/datadog.go	`73.03% <73.03%> (ø)`
utils/analysis/factory.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be3495d...f42d41e. Read the comment docs.

jessesuen

Awesome work!

stephen-harris force-pushed the datadog branch 2 times, most recently from c9f5710 to 815d4c2 Compare September 7, 2020 09:20

jessesuen self-assigned this Sep 8, 2020

jessesuen requested changes Sep 9, 2020

View reviewed changes

pkg/apis/rollouts/v1alpha1/analysis_types.go Outdated Show resolved Hide resolved

metricproviders/datadog/datadog.go Outdated Show resolved Hide resolved

metricproviders/datadog/datadog_test.go Outdated Show resolved Hide resolved

jessesuen mentioned this pull request Sep 22, 2020

web metric jsonPath always returns values as strings #728

Closed

jessesuen reviewed Sep 22, 2020

View reviewed changes

stephen-harris force-pushed the datadog branch from 3ed3178 to e7b5bae Compare September 24, 2020 20:09

jessesuen reviewed Sep 24, 2020

View reviewed changes

metricproviders/datadog/datadog.go Outdated Show resolved Hide resolved

jessesuen reviewed Oct 6, 2020

View reviewed changes

stephen-harris force-pushed the datadog branch from 3d77904 to 6f2c557 Compare October 9, 2020 20:18

stephen-harris added 13 commits October 9, 2020 21:22

feat: Add datadog metric provider

4356d25

Rename URL as Address and mark it as optional

f6d2628

Fix documentation

7459890

Add interval parameter

bf3e4e5

Expect datadog credentials to be stored in a controller-level secret

24df735

Remove apikey/appkey attributes from manifests

1ae8309

Fix documentation

3ef60e9

Expcility cast metric value to float64

00d5fb4

use http.Status for readability. Expect a 200 for success

bef2cb7

Refactor implementation without using jsonpath

fa55498

move address configuration to api/app secrets. Allow future support o…

905cecb

…f multiple key pairs

Add context to kubeclient call

d56e187

Simplify handling of datadog secrets

f42d41e

stephen-harris force-pushed the datadog branch from 6f2c557 to f42d41e Compare October 9, 2020 20:24

jessesuen approved these changes Oct 9, 2020

View reviewed changes

jessesuen merged commit 6c3fcfe into argoproj:master Oct 9, 2020

jessesuen mentioned this pull request Oct 9, 2020

feat: Add New Relic metricprovider #772

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add datadog metric provider. Fixes #702 #705

feat: Add datadog metric provider. Fixes #702 #705

stephen-harris commented Sep 7, 2020 •

edited

CLAassistant commented Sep 7, 2020 •

edited

codecov-commenter commented Sep 7, 2020 •

edited

jessesuen commented Sep 9, 2020

stephen-harris commented Sep 11, 2020

johnkost commented Sep 11, 2020

stephen-harris commented Sep 14, 2020

jessesuen Sep 22, 2020

stephen-harris Sep 24, 2020

jessesuen Sep 24, 2020

jessesuen Sep 24, 2020

stephen-harris Sep 24, 2020

jessesuen Sep 28, 2020

stephenharris Oct 3, 2020

jessesuen Oct 6, 2020

jessesuen Oct 6, 2020

codecov-io commented Oct 9, 2020 •

edited

jessesuen left a comment

feat: Add datadog metric provider. Fixes #702 #705

feat: Add datadog metric provider. Fixes #702 #705

Conversation

stephen-harris commented Sep 7, 2020 • edited

CLAassistant commented Sep 7, 2020 • edited

codecov-commenter commented Sep 7, 2020 • edited

Codecov Report

jessesuen commented Sep 9, 2020

stephen-harris commented Sep 11, 2020

johnkost commented Sep 11, 2020

stephen-harris commented Sep 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 9, 2020 • edited

Codecov Report

jessesuen left a comment

Choose a reason for hiding this comment

stephen-harris commented Sep 7, 2020 •

edited

CLAassistant commented Sep 7, 2020 •

edited

codecov-commenter commented Sep 7, 2020 •

edited

codecov-io commented Oct 9, 2020 •

edited