feat: add new groundwork output plugin #9891

VladislavSenkevich · 2021-10-08T12:11:50Z

Required for all PRs:

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

telegraf-tiger · 2021-10-08T12:11:56Z

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

Hipska

Great to see a Groundwork output integrated in telegraf! I do have some comments:

plugins/outputs/gw8/README.md

plugins/outputs/gw8/gw8.go

plugins/outputs/gw8/addons/types.go

telegraf-tiger · 2021-10-11T14:54:39Z

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

…or fixes

telegraf-tiger · 2021-10-11T15:06:24Z

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

VladislavSenkevich · 2021-10-11T15:06:54Z

Great to see a Groundwork output integrated in telegraf! I do have some comments:

Thank you for your feedback! I prepared another commit to support all your change requests

Hipska

Don't forget to also update the README file with new config options, and maybe some examples of which tags should or could be present and how they will used by GW?

Note that there are also a lot of linter warnings, and you need to sign the CLA.

plugins/outputs/gw8/gw8.go

plugins/outputs/gw8/gw8_test.go

plugins/outputs/gw8/gw8.go

plugins/outputs/gw8/addons/milliseconds.go

plugins/outputs/gw8/gw8.go

VladislavSenkevich · 2021-10-12T11:52:51Z

!signed-cla

srebhan

Hey @VladislavSenkevich, I think this is a very nice and valuable PR. Thanks for the contribution!

The code currently is IMO more complex than necessary. Please get rid of most of the addon stuff. Implement Timestamp in a way that only the required MarshalJSON is implemented and base it on time.Time (as @Hipska suggested). Furthermore, fold the sendRequest() function into the main package. The same holds for your time definitions.

plugins/outputs/gw8/README.md

plugins/outputs/gw8/addons/milliseconds.go

plugins/outputs/gw8/gw8.go

plugins/outputs/gw8/gw8_test.go

plugins/outputs/gw8/gw8.go

testutil/testutil.go

plugins/outputs/gw8/gw8.go

srebhan · 2021-10-28T14:06:10Z

@VladislavSenkevich any update here?

VladislavSenkevich · 2021-10-28T14:14:58Z

@VladislavSenkevich any update here?

We are working on a library from our Go side to avoid code duplication and will be back soon to make the plugin lighter

srebhan · 2021-10-28T14:17:46Z

This sounds amazing! I keep my fingers crossed and looking forward to the new plugin layout.

Hipska · 2021-10-28T14:20:43Z

Great! Could you maybe mark this PR as draft in the meantime?

plugins/outputs/groundwork/groundwork.go

Hipska · 2021-11-29T14:30:26Z

Hey @VladislavSenkevich I think you will need to recheck your last push, it seems like some find and replace messed up some other unrelated stuff..

VladislavSenkevich · 2021-11-29T14:35:32Z

Hey @VladislavSenkevich I think you will need to recheck your last push, it seems like some find and replace messed up some other unrelated stuff..

Yes, sorry! Jusr renamed using GoLand and didn't notice changes to other files
P.S Fixed

Hipska · 2021-11-29T16:37:13Z

The tests also fail. Apart from that and the thresholds problem, all seems fine for me.

VladislavSenkevich · 2021-11-30T09:25:28Z

The tests also fail. Apart from that and the thresholds problem, all seems fine for me.

Thresholds problem has been fixed. The same for tests.
And Thomas Stocking still working on the license issue.

Hipska

There is still a linter check that fails.

Note that there will be a RC release tomorrow, would be nice if the license problem could be fixed.

plugins/outputs/groundwork/groundwork.go

srebhan

Nice update @VladislavSenkevich! There are a few comments from my side. Please also care about the markdown linter-issues...

plugins/outputs/groundwork/groundwork.go

srebhan · 2021-11-30T10:34:29Z

plugins/outputs/groundwork/groundwork.go

+	if g.DefaultHost == "" {
+		return errors.New("no 'default_host' provided")
+	}
+	if g.ResourceTag == "" {
+		return errors.New("no 'resource_tag' provided")
+	}
+	if !validStatus(g.DefaultServiceState) {
+		return errors.New("invalid 'default_service_state' provided")
+	}


Right. Other plugins handle empty strings set by the user just as if they have set the default. In this case, you can move the default handling in here and just use the default if the string is empty, no matter if not set or explicitly set empty by the user.
This makes the code a bit cleaner, but if you insist on the present behavior, you should at least add a comment on why you do so.

plugins/outputs/groundwork/groundwork.go

plugins/outputs/groundwork/groundwork_test.go

srebhan

Looks like you want to keep the default handling as-is!?

Looks good from my side. Thanks @VladislavSenkevich for your effort!

Please be aware that the current license of the dependency CANNOT BE USED!

VladislavSenkevich · 2021-11-30T14:53:41Z

Looks like you want to keep the default handling as-is!?

Looks good from my side. Thanks @VladislavSenkevich for your effort!

Please be aware that the current license of the dependency CANNOT BE USED!

Thanks! I will conract Thomas in an hour about the license. I hope we will resolve this issue today

VladislavSenkevich · 2021-11-30T16:44:45Z

Please be aware that the current license of the dependency CANNOT BE USED!

@srebhan
@Hipska

The license has been updated. Please review.
Thank you for your patience guys!

srebhan · 2021-11-30T17:36:52Z

The license has been updated. Please review.

@VladislavSenkevich please update the information in the LICENSE_OF_DEPENDENCIES.md file.

VladislavSenkevich · 2021-11-30T17:57:50Z

The license has been updated. Please review.

@VladislavSenkevich please update the information in the LICENSE_OF_DEPENDENCIES.md file.

Looks like I forgot to push... Done now

telegraf-tiger · 2021-11-30T18:17:00Z

☺️ 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

feat: add new gw8 output plugin

a79c6fe

telegraf-tiger bot added fix pr to fix corresponding bug new plugin plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins labels Oct 8, 2021

Hipska self-assigned this Oct 11, 2021

Hipska suggested changes Oct 11, 2021

View reviewed changes

Hipska removed the fix pr to fix corresponding bug label Oct 11, 2021

Hipska requested a review from srebhan October 11, 2021 09:16

VladislavSenkevich requested a review from Hipska October 11, 2021 14:58

feat: add re-login mechanism, logic to build unique trace token + min…

ce057f6

…or fixes

VladislavSenkevich force-pushed the feature/gw8-output branch from 5f8cdf9 to ce057f6 Compare October 11, 2021 15:06

Hipska suggested changes Oct 11, 2021

View reviewed changes

srebhan requested changes Oct 12, 2021

View reviewed changes

srebhan self-assigned this Oct 12, 2021

VladislavSenkevich force-pushed the feature/gw8-output branch from cc0ca3d to e8f854a Compare October 12, 2021 13:58

Hipska suggested changes Oct 12, 2021

View reviewed changes

plugins/outputs/gw8/gw8.go Outdated Show resolved Hide resolved

testutil/testutil.go Outdated Show resolved Hide resolved

plugins/outputs/gw8/gw8.go Outdated Show resolved Hide resolved

plugins/outputs/gw8/gw8.go Outdated Show resolved Hide resolved

feat: fix timestamp, sampleConfig; add string value for metric parser

3089604

VladislavSenkevich force-pushed the feature/gw8-output branch from e8f854a to 3089604 Compare October 12, 2021 15:15

Hipska reviewed Oct 13, 2021

View reviewed changes

plugins/outputs/gw8/gw8.go Outdated Show resolved Hide resolved

Hipska reviewed Oct 13, 2021

View reviewed changes

plugins/outputs/gw8/gw8.go Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/master' into feature/gw8-output

a374b44

VladislavSenkevich requested a review from srebhan November 29, 2021 07:12

Hipska reviewed Nov 29, 2021

View reviewed changes

plugins/outputs/groundwork/groundwork.go Outdated Show resolved Hide resolved

VladislavSenkevich and others added 5 commits November 29, 2021 15:20

feat: provide minor fixes

76933d1

feat: update README.md

431862e

feat: update go mod

8b1afb8

feat: improve logging and add license

e9050e3

feat: provide minor fixes

793a849

VladislavSenkevich force-pushed the feature/gw8-output branch from e30def7 to 1fb85d5 Compare November 29, 2021 12:20

VladislavSenkevich force-pushed the feature/gw8-output branch from 1fb85d5 to 57c225f Compare November 29, 2021 14:34

VladislavSenkevich force-pushed the feature/gw8-output branch from 57c225f to 05eddb7 Compare November 30, 2021 09:23

VladislavSenkevich changed the title ~~feat: add new gw8 output plugin~~ feat: add new groundwork output plugin Nov 30, 2021

Hipska approved these changes Nov 30, 2021

View reviewed changes

plugins/outputs/groundwork/groundwork.go Outdated Show resolved Hide resolved

Hipska requested a review from zak-pawel November 30, 2021 10:31

srebhan reviewed Nov 30, 2021

View reviewed changes

feat: add GWClient

fae55d2

VladislavSenkevich force-pushed the feature/gw8-output branch from 05eddb7 to fae55d2 Compare November 30, 2021 11:28

VladislavSenkevich requested a review from srebhan November 30, 2021 13:44

srebhan approved these changes Nov 30, 2021

View reviewed changes

feat: update LICENSE

8c94403

reimda approved these changes Nov 30, 2021

View reviewed changes

reimda merged commit 27dea9b into influxdata:master Nov 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add new groundwork output plugin #9891

feat: add new groundwork output plugin #9891

VladislavSenkevich commented Oct 8, 2021 •

edited by Hipska

Loading

telegraf-tiger bot commented Oct 8, 2021

Hipska left a comment

telegraf-tiger bot commented Oct 11, 2021

telegraf-tiger bot commented Oct 11, 2021

VladislavSenkevich commented Oct 11, 2021

Hipska left a comment

VladislavSenkevich commented Oct 12, 2021

srebhan left a comment

srebhan commented Oct 28, 2021

VladislavSenkevich commented Oct 28, 2021

srebhan commented Oct 28, 2021

Hipska commented Oct 28, 2021

Hipska commented Nov 29, 2021

VladislavSenkevich commented Nov 29, 2021 •

edited

Loading

Hipska commented Nov 29, 2021

VladislavSenkevich commented Nov 30, 2021 •

edited

Loading

Hipska left a comment

srebhan left a comment

srebhan Nov 30, 2021 •

edited

Loading

srebhan left a comment •

edited

Loading

VladislavSenkevich commented Nov 30, 2021

VladislavSenkevich commented Nov 30, 2021

srebhan commented Nov 30, 2021

VladislavSenkevich commented Nov 30, 2021

telegraf-tiger bot commented Nov 30, 2021

Artifact URLs

feat: add new groundwork output plugin #9891

feat: add new groundwork output plugin #9891

Conversation

VladislavSenkevich commented Oct 8, 2021 • edited by Hipska Loading

Required for all PRs:

telegraf-tiger bot commented Oct 8, 2021

Hipska left a comment

Choose a reason for hiding this comment

telegraf-tiger bot commented Oct 11, 2021

telegraf-tiger bot commented Oct 11, 2021

VladislavSenkevich commented Oct 11, 2021

Hipska left a comment

Choose a reason for hiding this comment

VladislavSenkevich commented Oct 12, 2021

srebhan left a comment

Choose a reason for hiding this comment

srebhan commented Oct 28, 2021

VladislavSenkevich commented Oct 28, 2021

srebhan commented Oct 28, 2021

Hipska commented Oct 28, 2021

Hipska commented Nov 29, 2021

VladislavSenkevich commented Nov 29, 2021 • edited Loading

Hipska commented Nov 29, 2021

VladislavSenkevich commented Nov 30, 2021 • edited Loading

Hipska left a comment

Choose a reason for hiding this comment

srebhan left a comment

Choose a reason for hiding this comment

srebhan Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

srebhan left a comment • edited Loading

Choose a reason for hiding this comment

VladislavSenkevich commented Nov 30, 2021

VladislavSenkevich commented Nov 30, 2021

srebhan commented Nov 30, 2021

VladislavSenkevich commented Nov 30, 2021

telegraf-tiger bot commented Nov 30, 2021

Artifact URLs

VladislavSenkevich commented Oct 8, 2021 •

edited by Hipska

Loading

VladislavSenkevich commented Nov 29, 2021 •

edited

Loading

VladislavSenkevich commented Nov 30, 2021 •

edited

Loading

srebhan Nov 30, 2021 •

edited

Loading

srebhan left a comment •

edited

Loading