feat: Add New Relic metricprovider #772

jwelch92 · 2020-10-08T22:56:43Z

Closes #736

Due to the different result format for NRQL queries I had to deviate a bit from some of the common result representations in the other providers. I hope the documentation makes it clear how to use the results correctly.

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.

jwelch92 · 2020-10-08T23:04:02Z

cc/ @ctrombley @sanderblue @zlesnr

jessesuen · 2020-10-09T23:08:32Z

pkg/apis/rollouts/v1alpha1/analysis_types.go

@@ -130,6 +130,8 @@ type MetricProvider struct {
 	Web *WebMetric `json:"web,omitempty"`
 	// Wavefront specifies the wavefront metric to query
 	Wavefront *WavefrontMetric `json:"wavefront,omitempty"`
+	// NewRelic specifies the newrelic metric to query
+	NewRelic *NewRelicMetric `json:"newrelic,omitempty"`


The API violation in violation_exceptions.list is complaining that NewRelic has the R in the field name uppercased, but the JSON tag is all lowercase. The json tag should be newRelic to avoid the violation.

Thanks I'll get that fixed up!

jessesuen · 2020-10-09T23:40:31Z

docs/features/analysis.md

+        region: "us"
+        accountID: 1234


I have some questions about the appropriate level of where region and accountID should be configured. i.e. could/should this be configuration at the controller level vs. AnalysisTemplate level? Maybe these clarifying questions would help me better understand:

can the same account ID be used in different regions? If not, then I think we should eliminate region from the analysis spec, and configure the region centrally (same place where the accountID api-key is specified).

is it common to have multiple accountIDs in the same organization? If most organizations use a single accountID, I would propose to make their experience better / less configuration by not require accountID to be specified in the AnalysisTemplate, and provide a way where a single accountID could be configured centrally. Then AnalysisTemplates would only need to specify query and nothing else.

If there is a need / use case to support multiple accountIDs within the same cluster/organization, I think accountID could still be made an optional field in the analysis spec, but when omitted, the controller could either use a designated "default" accountID / region, or infer the accountID/region/key if there is only a single accountID configured.

Note that a very similar discussion just happened with the Datadog provider:
#705 (comment)

With DataDog, it was decided that it would be uncommon for a single organization to configure multiple DataDog API/APP keys, so currently argo-rollouts will look to a single Datadog secret for its configuration:

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

NOTE that this doesn't preclude the possibility in the future to support multiple Datadog secrets/api-keys in the same cluster, but it makes the default end-user/developer experience better since end-users do not need to concern themselves with specifying addresses/API/APP keys in the AnalysisTemplate.

Another example is with wavefront. With wavefront, it is common to use different wavefront hosts even within the same cluster. So, it's configuration looks like this:

Centrally configured mapping from Host-to-API tokens:

apiVersion: v1 kind: Secret metadata: name: wavefront-api-tokens type: Opaque data: example1.wavefront.com: <token1> example2.wavefront.com: <token2>

In the AnalysisTemplate, a reference to which host to use:

apiVersion: argoproj.io/v1alpha1 kind: AnalysisTemplate metadata: name: success-rate spec: args: - name: service-name metrics: - name: success-rate interval: 5m successCondition: result >= 0.95 provider: wavefront: address: example.wavefront.com

Given the DataDog and Wavefront examples, which approach would NewRelic more closely follow?

Thanks for the detailed feedback @jessesuen!

Based on your feedback I am working on updating this PR with changes that will more closely follow the pattern used by DataDog. New Relic API keys are specific to an account ID and accounts are specific to a region of New Relic. Given that, the model of putting all of the configuration into the secret to be a global config makes a lot more sense.

I am adding support for multiple secrets out of the gate because we have the need to support multiple accounts within a given cluster. It's fairly common for New Relic users to have primary accounts with sub-accounts that each have their own monitored applications and separate API keys.

jwelch92 · 2020-10-14T23:40:00Z

Updated the New Relic configuration to more closely match other consumers of the newrelic-client-go library like the newrelic-cli which uses profiles for credentials.

newrelic profile add --help
Add a new profile

The add command creates a new profile for use with the New Relic CLI.
API key and region are required. An Insights insert key is optional, but required
for posting custom events with the `newrelic events`command.

Usage:
  newrelic profile add [flags]

Examples:
newrelic profile add --name <profileName> --region <region> --apiKey <apiKey> --insightsInsertKey <insightsInsertKey>

Flags:
      --apiKey string              your personal API key
  -h, --help                       help for add
      --insightsInsertKey string   your Insights insert key
  -n, --name string                unique profile name to add
  -r, --region string              the US or EU region

Global Flags:
      --format string   output text format [Text, YAML, JSON] (default "JSON")
      --plain           output compact text

jwelch92 · 2020-10-27T15:28:42Z

Hey @jessesuen can you please give this another look when you get a chance? I'll try to clear the new merge conflicts as soon as possible. Thanks!!

jessesuen · 2020-10-29T20:13:19Z

docs/features/analysis.md

+    successCondition: result.successRate >= 0.95
+    provider:
+      newRelic:
+        profileSecretName: my-newrelic-secret  # optional, defaults to 'newrelic'


I like the idea of profiles, which match the new relic CLI. However, I'd prefer to keep the fact that profiles are stored as secrets, an implementation detail from the spec. So can we rename this to just profile ?

jessesuen · 2020-10-29T20:16:59Z

@jwelch92 this looks just about ready to go. The only change I think needs to be done is renaming profileSecretName to profile.

In the future, this would free us to change the implementation of how profiles are stored, and the spec would still make sense. For example, newrelic profiles could be changed to be configured in a central configmap, and reference arbitrarily named secrets.

- Removed accountID and region fields from the NewRelic metric spec - Modified expected secret format to include account ID, region, and api key - added new field to metric spec to support multiple New Relic config secrets

jwelch92 · 2020-10-29T23:27:00Z

manifests/install.yaml

@@ -5647,8 +5661,6 @@ spec:
                    properties:
                      datadog:
                        properties:
-                          address:


Hmm not sure why this diff is showing for datadog. I rebased upstream master and ran codegen and rebuilt manifests.

jwelch92 · 2020-10-29T23:30:10Z

Thanks again @jessesuen! Updated with your requested changes.

jwelch92 · 2020-10-30T18:08:29Z

Build failed on tests unrelated to my changes..is master broken right now? I rebased yesterday when I was fixing up my changes.

jessesuen · 2020-10-30T18:11:22Z

Let me check. Heads up, I might commit to your fork/branch if it's something I can easily fix.

codecov-io · 2020-10-31T01:52:31Z

Codecov Report

Merging #772 into master will increase coverage by 0.03%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master     #772      +/-   ##
==========================================
+ Coverage   82.34%   82.37%   +0.03%     
==========================================
  Files          96       97       +1     
  Lines        8202     8279      +77     
==========================================
+ Hits         6754     6820      +66     
- Misses       1037     1045       +8     
- Partials      411      414       +3

Impacted Files	Coverage Δ
metricproviders/newrelic/newrelic.go	`85.33% <85.33%> (ø)`
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 330add6...444f2dc. Read the comment docs.

jessesuen · 2020-10-31T01:53:31Z

Seems it just needed to resubmit and it passed.

Great work!

jwelch92 marked this pull request as ready for review October 9, 2020 15:03

jessesuen self-assigned this Oct 9, 2020

jessesuen reviewed Oct 9, 2020

View reviewed changes

jwelch92 force-pushed the add-new-relic branch from 8d1e1f9 to 2980062 Compare October 14, 2020 23:35

jessesuen reviewed Oct 29, 2020

View reviewed changes

Jared Welch added 4 commits October 29, 2020 13:32

feat: Add New Relic metricprovider

b110ccd

chore: add New Relic to users list in readme

2873622

Addressing PR feedback

7407602

- Removed accountID and region fields from the NewRelic metric spec - Modified expected secret format to include account ID, region, and api key - added new field to metric spec to support multiple New Relic config secrets

chore: rename New Relic profileSecretName to just profile

444f2dc

jwelch92 force-pushed the add-new-relic branch from 2980062 to 444f2dc Compare October 29, 2020 23:25

jwelch92 commented Oct 29, 2020

View reviewed changes

jessesuen approved these changes Oct 31, 2020

View reviewed changes

jessesuen merged commit c16119e into argoproj:master Oct 31, 2020

jwelch92 deleted the add-new-relic branch October 31, 2020 04:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add New Relic metricprovider #772

feat: Add New Relic metricprovider #772

jwelch92 commented Oct 8, 2020 •

edited

Loading

jwelch92 commented Oct 8, 2020

jessesuen Oct 9, 2020

jwelch92 Oct 11, 2020

jessesuen Oct 9, 2020 •

edited

Loading

jwelch92 Oct 11, 2020

jwelch92 commented Oct 14, 2020

jwelch92 commented Oct 27, 2020

jessesuen Oct 29, 2020

jessesuen commented Oct 29, 2020

jwelch92 Oct 29, 2020

jwelch92 commented Oct 29, 2020

jwelch92 commented Oct 30, 2020

jessesuen commented Oct 30, 2020

codecov-io commented Oct 31, 2020

jessesuen commented Oct 31, 2020

feat: Add New Relic metricprovider #772

feat: Add New Relic metricprovider #772

Conversation

jwelch92 commented Oct 8, 2020 • edited Loading

jwelch92 commented Oct 8, 2020

jessesuen Oct 9, 2020

Choose a reason for hiding this comment

jwelch92 Oct 11, 2020

Choose a reason for hiding this comment

jessesuen Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

jwelch92 Oct 11, 2020

Choose a reason for hiding this comment

jwelch92 commented Oct 14, 2020

jwelch92 commented Oct 27, 2020

jessesuen Oct 29, 2020

Choose a reason for hiding this comment

jessesuen commented Oct 29, 2020

jwelch92 Oct 29, 2020

Choose a reason for hiding this comment

jwelch92 commented Oct 29, 2020

jwelch92 commented Oct 30, 2020

jessesuen commented Oct 30, 2020

codecov-io commented Oct 31, 2020

Codecov Report

jessesuen commented Oct 31, 2020

jwelch92 commented Oct 8, 2020 •

edited

Loading

jessesuen Oct 9, 2020 •

edited

Loading