feat: Add Cloud Run Output Plugin & Update to Wavefront Serializer by crflanigan · Pull Request #10210 · influxdata/telegraf

crflanigan · 2021-12-02T21:14:29Z

Required for all PRs:

Updated associated README.md.
Wrote appropriate unit tests.
Pull request title or commits are in conventional commit format

resolves #7913

Added an output plugin for Google Cloud Run which implements the OAuth 2.0 protocol enabling the Telegraf agent to send encrypted metric payloads into a time series metrics proxy running in GCP.

Additionally, customers using the Wavefront serializer who don't wish to have their metric prefixes modified may configure their output plugin in the same way as the Wavefront Output Plugin.

This is accomplished by adding the convert_paths = false boolean option to the output section of their conf file.

Example prefix before getting processed by the Wavefront serializer:
prod.prefix_name.metric_name

Existing behavior:
prefix.name.metric.name

New behavior with convert_paths = false option:
prod.prefix_name.metric_name

Development

@zachmares

Auth.go Test Case Changes Great work @zachmares !!

- tweak function and argument names - cleaned up comments, new lines - adjust TestGetAccessToken test case

- added some TODOs for further strengthening of test cases, but established testing pattern.

- Close() and Connect() are called inside Write() and are required to have no error - Adjust TODO

telegraf-tiger · 2021-12-02T21:14:34Z

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

crflanigan · 2021-12-02T21:14:41Z

@zachmares

telegraf-tiger · 2021-12-02T21:16:30Z

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

crflanigan · 2021-12-02T21:27:09Z

!signed-cla

telegraf-tiger · 2021-12-02T22:57:46Z

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Looks like new artifacts were built from this PR.

Expand this list to get them here ! 🐯

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armel.deb	armv6hl.rpm	freebsd_armv7.tar.gz
armhf.deb	i386.rpm	freebsd_i386.tar.gz
i386.deb	ppc64le.rpm	linux_amd64.tar.gz
mips.deb	s390x.rpm	linux_arm64.tar.gz
mipsel.deb	x86_64.rpm	linux_armel.tar.gz
ppc64el.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_s390x.tar.gz
		static_linux_amd64.tar.gz

srebhan

@crflanigan thank you very much for this cool contribution! The linter found some issues and I also have some comments in the code. The most severe ones are

Please split this PR into one for the Wavefront part and one for the CloudRun part. You probably best move the Wavefront part to a new PR and remove it from this one. Please remember to adapt the PR title and description accordingly. Feel free to assign (or mention) me in the newly created PR so I can give it a review.
Almost half of the PR goes to common/gcp dealing with the authentication. I wonder why you can't use the existing oauth2 library. Is there a reason? If you really can't use any external lib, my impression is that common/gcp should be a library outside of Telegraf as it is in no way specific to this project.
Please check the linter warnings and errors.

Looking forward to the next round and your comments!

srebhan · 2021-12-03T08:35:43Z

config/config.go

-		// Skip inputs that are registered twice for backward compatibility
 		if pname == "cisco_telemetry_gnmi" {
 			continue
 		}
-		if pname == "KNXListener" {
-			continue
-		}


Why do you remove those?

srebhan · 2021-12-03T08:37:01Z

go.mod


-require github.com/libp2p/go-reuseport v0.1.0
+require (
+	github.com/golang-jwt/jwt v3.2.2+incompatible


Would it be possible to use github.com/golang-jwt/jwt/v4 which we already carry as a dependency instead of v3?

Let me check, thank you!

srebhan · 2021-12-03T08:38:59Z

plugins/common/gcp/auth.go

@@ -0,0 +1,118 @@
+package gcp


Why do you put this in common? The only user currently AFAICS is cloutrun, so it belongs there. If you plan to add more users later we can still move the files...

Sure, would you suggest (if we keep this package) we keep this code alongside the cloudrun package code?

Kind of. First of all, please check carefully if using golang.org/x/oauth2/google is an option instead of really having this code. Second, I tend to making the code in this package an external packages that does not live in telegraf. However, if you have good reasons why option one and two cannot be pursued we should move it to the cloudrun plugin directory.

srebhan · 2021-12-03T08:40:33Z

plugins/common/gcp/auth.go

+		return "", fmt.Errorf("could not parse service account JSON: %v", err)
+	}
+
+	signedJWT, err := generateSignedJWT(conf, audience, 120)


Is that expiry time the only valid value? Maybe you should allow to provide the expiry time as a parameter.

srebhan · 2021-12-03T08:42:05Z

plugins/common/gcp/auth.go

+func generateSignedJWT(conf *jwt.Config, audience string, expiryLength int64) (string, error) {
+	now := time.Now().Unix()


To be honest I would feel a bit better to get the expiry time as a time.Duration...

Suggested change

func generateSignedJWT(conf *jwt.Config, audience string, expiryLength int64) (string, error) {

now := time.Now().Unix()

func generateSignedJWT(conf *jwt.Config, audience string, expiryLength time.Duration) (string, error) {

now := time.Now().Unix()

expiry := now.Add(expiryLength)

srebhan · 2021-12-03T09:18:29Z

plugins/outputs/cloudrun/cloudrun.go

+	req.Header.Set("Content-Type", defaultContentType)
+	req.Header.Set("Accept", defaultAccept)


Please directly set the values here instead of using default... constants as this is the only user and you directly can see what is set instead of navigating the whole file.

srebhan · 2021-12-03T09:19:26Z

plugins/outputs/cloudrun/cloudrun.go

+		if strings.ToLower(k) == "host" {
+			req.Host = v
+		}


Do you mean to continue here as otherwise host is set twice once in req.Host and another time as req.Header...

srebhan · 2021-12-03T09:19:56Z

plugins/outputs/cloudrun/cloudrun.go

+	}
+
+	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
+		return fmt.Errorf("when writing to [%s] received status code: %d", cr.URL, resp.StatusCode)


Suggested change

return fmt.Errorf("when writing to [%s] received status code: %d", cr.URL, resp.StatusCode)

return fmt.Errorf("when writing to %q received status code: %d", cr.URL, resp.StatusCode)

srebhan · 2021-12-03T09:24:30Z

plugins/serializers/registry.go

 	PrometheusStringAsLabel bool `toml:"prometheus_string_as_label"`
+
+	// Convert "_" in paths to "." for Wavefront
+	WavefrontConvertPaths bool `toml:"convert_paths"`


With this change you will break all existing wavefront users as suddenly (when using their old config) their paths change without further notice on updateing telegraf. This cannot happen. Please invert that setting such that by default (i.e. not set by the user) the paths are converted as before.

A natural alternative would be wavefront_keep_path or wavefront_disable_path_conversion.

srebhan · 2021-12-03T09:26:25Z

plugins/serializers/wavefront/wavefront_test.go

 	}

-	s := WavefrontSerializer{}
+	s := WavefrontSerializer{ConvertPaths: true}


That's exactly the point mentioned above. All current users would need to do the same in their config. Please keep existing tests unchanged to ensure backward compatibility!

Good point!
Let me fix that.

crflanigan · 2021-12-09T20:06:39Z

Going to close this as suggested by @srebhan and open two pull requests, one for the update to the Wavefront serializer and one for the Cloud Run code.

Flanigan and others added 16 commits November 19, 2021 15:03

Adding cloudrun code to fork

49fed17

Merge branch 'master' into development

ec89857

Add convert_paths config option to Wavefront serializer

eb803e5

make fmt

16e34d1

Merge pull request #1 from homedepot/development

28952d0

Development

WIP: Generate JWT Token Test

80a1396

WIP: Continuing with generate JWT test

6299db0

WIP: Refactor while working on Auth tests

43c8372

Auth Test

b93430d

Merge pull request #2 from homedepot/testing

0484d76

Auth.go Test Case Changes Great work @zachmares !!

Update Cloud Run README

24217f7

Remove comment from CloudRun struct

64277c7

Minor changes to auth.go

7321332

- tweak function and argument names - cleaned up comments, new lines - adjust TestGetAccessToken test case

Cloud Run Output test code in place

733d1e4

- added some TODOs for further strengthening of test cases, but established testing pattern.

Minor touch-up to test_key_file.json content

641b3f1

Removed Cloud Run Close() and Connect() test funcs

0e50f32

- Close() and Connect() are called inside Write() and are required to have no error - Adjust TODO

telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Dec 2, 2021

zachmares and others added 4 commits December 2, 2021 16:14

Merge branch 'testing'

b2b71f1

Update go.mod

5edfe16

Update wavefront_test.go with new convert_paths options

e2e5f5c

Merge branch 'master' into master

373f709

srebhan requested changes Dec 3, 2021

View reviewed changes

srebhan self-assigned this Dec 6, 2021

crflanigan closed this Dec 9, 2021

crflanigan mentioned this pull request Dec 9, 2021

feat: Add option to disable Wavefront prefix conversion #10252

Merged

3 tasks

		func generateSignedJWT(conf *jwt.Config, audience string, expiryLength int64) (string, error) {
		now := time.Now().Unix()

		req.Header.Set("Content-Type", defaultContentType)
		req.Header.Set("Accept", defaultAccept)

	return fmt.Errorf("when writing to [%s] received status code: %d", cr.URL, resp.StatusCode)
	return fmt.Errorf("when writing to %q received status code: %d", cr.URL, resp.StatusCode)

Conversation

crflanigan commented Dec 2, 2021

Required for all PRs:

Uh oh!

telegraf-tiger bot commented Dec 2, 2021

Uh oh!

crflanigan commented Dec 2, 2021

Uh oh!

telegraf-tiger bot commented Dec 2, 2021

Uh oh!

crflanigan commented Dec 2, 2021

Uh oh!

telegraf-tiger bot commented Dec 2, 2021

Artifact URLs

Uh oh!

srebhan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crflanigan commented Dec 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

srebhan left a comment •

edited

Loading