feat: Add support of aggregator as Starlark script #9419

essobedo · 2021-06-23T10:40:48Z

Required for all PRs:

Updated associated README.md.
Wrote appropriate unit tests.

resolves #8869

essobedo · 2021-06-23T10:53:20Z

@ssoroka It is not yet ready since the doc is missing but I would like to be sure that it is the right direction so could you please tell me WDYT of this PR?

plugins/aggregators/starlark/README.md

plugins/aggregators/starlark/starlark.go

ssoroka

looking pretty good. left a few questions

plugins/aggregators/starlark/starlark.go

plugins/aggregators/starlark/testdata/merge.star

plugins/aggregators/starlark/testdata/min_max.star

plugins/processors/starlark/starlark.go

sjwang90 · 2021-09-15T23:18:27Z

@essobedo - would you like someone to continue reviewing this PR? If so, feel free to take it out of its draft state.

essobedo · 2021-09-18T09:23:20Z

@essobedo - would you like someone to continue reviewing this PR? If so, feel free to take it out of its draft state.

@sjwang90 yes please 🙏

telegraf-tiger · 2021-09-18T09:30:25Z

Looks like new artifacts were built from this PR. Get them here!

Artifact URLs

plugins/aggregators/starlark/README.md

plugins/aggregators/starlark/testdata/min_max.star

essobedo · 2021-10-08T08:34:15Z

@reimda thank you for your review, I addressed your remarks so please check again

essobedo · 2021-10-15T15:09:00Z

@reimda any other remarks about this PR?

essobedo · 2021-11-04T14:02:30Z

@srebhan Hi, could you please tell me what you think of this PR?

sjwang90 · 2021-11-04T15:56:12Z

@essobedo If all the comments above are addressed can you make sure to mark them as resolved. We can then get someone to do another review on the PR.

essobedo · 2021-11-04T16:02:35Z

@sjwang90 Yes it is what I did except for remarks for which I gave an answer, as I did not get any approval, I considered them as still open

srebhan

Thank you @essobedo for the cool PR. I think it makes sense to provide a scriptable aggregator. We need to work a bit on the structure though. I second the view of @reimda to use the state as a shared thing to persist values and I think there is nothing speaking against such an approach. Furthermore, we should follow the Aggregator interface also in starlark, i.e. having a add, push and reset function as people are used to those and there is documentation on what they are intended to do.

What do you think?

plugins/aggregators/starlark/README.md

plugins/aggregators/starlark/starlark.go

plugins/common/starlark/starlark.go

plugins/processors/starlark/starlark_test.go

essobedo · 2021-11-08T10:53:09Z

@srebhan thank you for your feedbacks, I addressed your remarks, please check again

plugins/aggregators/starlark/starlark.go

plugins/common/starlark/starlark.go

plugins/aggregators/starlark/starlark.go

srebhan

Very nice update @essobedo! Some more ideas to move more code into common... :-)

essobedo · 2021-11-08T14:58:22Z

@srebhan I improved the function InitFunction based on your remarks and used the same code to manage the resulting value of the Call functions.

essobedo · 2021-11-09T11:06:29Z

@srebhan function AddResultingValue and related changes have been reverted. A map has been used as requested to store the functions and the arguments, please check again

telegraf-tiger · 2021-11-09T11:21:03Z

🥳 This pull request decreases the Telegraf binary size by -0.05 % for linux amd64 (new size: 130.7 MB, nightly size 130.8 MB)

📦 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

Almost there. I still want to avoid the //nolint "magic" when calling Call and have a suggestion. Please let me know what you think.

plugins/common/starlark/starlark.go

essobedo · 2021-11-10T08:48:58Z

Almost there. I still want to avoid the //nolint "magic" when calling Call and have a suggestion. Please let me know what you think.

fixed, please check again

telegraf-tiger · 2021-11-10T09:05:53Z

🥳 This pull request decreases the Telegraf binary size by -0.04 % for linux amd64 (new size: 130.7 MB, nightly size 130.8 MB)

📦 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

plugins/common/starlark/starlark.go

essobedo · 2021-11-15T10:45:31Z

Please note that the build failure is not related to this change

srebhan

This is beautiful @essobedo! Some more minor comments... :-/

plugins/aggregators/starlark/starlark.go

plugins/common/starlark/starlark.go

srebhan

Perfect @essobedo. Thank you very much for your work!

reimda

Thanks @essobedo! This PR is getting very close. Could you take a look at how metrics are hashed? If we can resolve that (and a typo I found), I think it's ready to merge.

plugins/aggregators/starlark/README.md

plugins/aggregators/starlark/starlark.go

plugins/processors/starlark/starlark.go

plugins/processors/starlark/starlark_test.go

plugins/common/starlark/metric.go

essobedo · 2021-11-17T08:42:33Z

@reimda Fixed, please check again

telegraf-tiger · 2021-11-17T09:00:15Z

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

reimda · 2021-11-18T23:11:33Z

Thanks @essobedo for all your work on this!

MyaLongmire · 2021-11-19T19:09:24Z

@essobedo Thank you so much for this pr! There is a leftover //nolint comment that is keeping the file from being linted. I removed it in this pr but didn't touch anything else, just wanted to let you know.

telegraf-tiger bot added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin new plugin plugin/aggregator 1. Request for new aggregator plugins 2. Issues/PRs that are related to aggregator plugins labels Jun 23, 2021

essobedo commented Jun 23, 2021

View reviewed changes

plugins/aggregators/starlark/README.md Outdated Show resolved Hide resolved

essobedo commented Jun 23, 2021

View reviewed changes

plugins/aggregators/starlark/starlark.go Outdated Show resolved Hide resolved

ssoroka reviewed Jun 29, 2021

View reviewed changes

essobedo requested a review from ssoroka July 3, 2021 14:29

essobedo marked this pull request as ready for review September 16, 2021 06:57

essobedo changed the title ~~Add support of aggregator as Starlark script~~ feat: Add support of aggregator as Starlark script Sep 18, 2021

reimda removed the request for review from ssoroka September 24, 2021 20:29

reimda suggested changes Sep 28, 2021

View reviewed changes

essobedo requested a review from reimda October 8, 2021 08:33

srebhan requested changes Nov 5, 2021

View reviewed changes

srebhan self-assigned this Nov 5, 2021

essobedo requested a review from srebhan November 8, 2021 10:52

srebhan reviewed Nov 8, 2021

View reviewed changes

plugins/aggregators/starlark/starlark.go Outdated Show resolved Hide resolved

plugins/aggregators/starlark/starlark.go Show resolved Hide resolved

srebhan reviewed Nov 8, 2021

View reviewed changes

plugins/common/starlark/starlark.go Outdated Show resolved Hide resolved

plugins/common/starlark/starlark.go Outdated Show resolved Hide resolved

plugins/aggregators/starlark/starlark.go Outdated Show resolved Hide resolved

srebhan reviewed Nov 8, 2021

View reviewed changes

essobedo requested a review from srebhan November 8, 2021 14:56

essobedo added 2 commits November 9, 2021 11:22

feat: Improve Starlark function initialization

97d1a7f

feat: Register functions and arguments in a map

4786cf2

essobedo requested a review from srebhan November 9, 2021 11:04

srebhan reviewed Nov 10, 2021

View reviewed changes

plugins/common/starlark/starlark.go Outdated Show resolved Hide resolved

essobedo requested a review from srebhan November 10, 2021 08:48

srebhan reviewed Nov 11, 2021

View reviewed changes

plugins/common/starlark/starlark.go Outdated Show resolved Hide resolved

feat: Add a function to log errors

88a7e2b

essobedo requested a review from srebhan November 15, 2021 09:01

srebhan reviewed Nov 16, 2021

View reviewed changes

feat: Cleanup the code

09f8abb

essobedo requested a review from srebhan November 16, 2021 10:32

srebhan approved these changes Nov 16, 2021

View reviewed changes

srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Nov 16, 2021

reimda suggested changes Nov 16, 2021

View reviewed changes

feat: Define specific functions to group metrics

5839cba

essobedo requested a review from reimda November 17, 2021 08:41

srebhan approved these changes Nov 17, 2021

View reviewed changes

reimda approved these changes Nov 18, 2021

View reviewed changes

reimda merged commit 4f2ade5 into influxdata:master Nov 18, 2021

essobedo deleted the 8869/starlark-as-aggregator branch November 19, 2021 08:00

MyaLongmire mentioned this pull request Nov 19, 2021

chore: remove leftover nolint comment #10138

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support of aggregator as Starlark script #9419

feat: Add support of aggregator as Starlark script #9419

essobedo commented Jun 23, 2021 •

edited

Loading

essobedo commented Jun 23, 2021

ssoroka left a comment

sjwang90 commented Sep 15, 2021

essobedo commented Sep 18, 2021

telegraf-tiger bot commented Sep 18, 2021

Artifact URLs

essobedo commented Oct 8, 2021

essobedo commented Oct 15, 2021

essobedo commented Nov 4, 2021

sjwang90 commented Nov 4, 2021

essobedo commented Nov 4, 2021

srebhan left a comment •

edited

Loading

essobedo commented Nov 8, 2021

srebhan left a comment

essobedo commented Nov 8, 2021 •

edited

Loading

essobedo commented Nov 9, 2021

telegraf-tiger bot commented Nov 9, 2021

Artifact URLs

srebhan left a comment

essobedo commented Nov 10, 2021

telegraf-tiger bot commented Nov 10, 2021

Artifact URLs

essobedo commented Nov 15, 2021

srebhan left a comment

srebhan left a comment

reimda left a comment

essobedo commented Nov 17, 2021 •

edited

Loading

telegraf-tiger bot commented Nov 17, 2021

Artifact URLs

reimda commented Nov 18, 2021 •

edited

Loading

MyaLongmire commented Nov 19, 2021

feat: Add support of aggregator as Starlark script #9419

feat: Add support of aggregator as Starlark script #9419

Conversation

essobedo commented Jun 23, 2021 • edited Loading

Required for all PRs:

essobedo commented Jun 23, 2021

ssoroka left a comment

Choose a reason for hiding this comment

sjwang90 commented Sep 15, 2021

essobedo commented Sep 18, 2021

telegraf-tiger bot commented Sep 18, 2021

Artifact URLs

essobedo commented Oct 8, 2021

essobedo commented Oct 15, 2021

essobedo commented Nov 4, 2021

sjwang90 commented Nov 4, 2021

essobedo commented Nov 4, 2021

srebhan left a comment • edited Loading

Choose a reason for hiding this comment

essobedo commented Nov 8, 2021

srebhan left a comment

Choose a reason for hiding this comment

essobedo commented Nov 8, 2021 • edited Loading

essobedo commented Nov 9, 2021

telegraf-tiger bot commented Nov 9, 2021

Artifact URLs

srebhan left a comment

Choose a reason for hiding this comment

essobedo commented Nov 10, 2021

telegraf-tiger bot commented Nov 10, 2021

Artifact URLs

essobedo commented Nov 15, 2021

srebhan left a comment

Choose a reason for hiding this comment

srebhan left a comment

Choose a reason for hiding this comment

reimda left a comment

Choose a reason for hiding this comment

essobedo commented Nov 17, 2021 • edited Loading

telegraf-tiger bot commented Nov 17, 2021

Artifact URLs

reimda commented Nov 18, 2021 • edited Loading

MyaLongmire commented Nov 19, 2021

essobedo commented Jun 23, 2021 •

edited

Loading

srebhan left a comment •

edited

Loading

essobedo commented Nov 8, 2021 •

edited

Loading

essobedo commented Nov 17, 2021 •

edited

Loading

reimda commented Nov 18, 2021 •

edited

Loading