New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Add Clarify output plugin #13220

Merged

powersj merged 9 commits into influxdata:master from clarify:clarify-output-plugin

May 22, 2023

Contributor

bbergshaven commented May 2, 2023

Add plugin to support writing metrics to Clarify.

Required for all PRs

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

Added output plugin to write metric to Clarify.
www.clarify.io

telegraf-tiger bot added the feat label

bbergshaven force-pushed the clarify-output-plugin branch from 2e7a342 to b5c9866 Compare

May 2, 2023 10:40

srebhan added new plugin plugin/output labels

srebhan self-assigned this

srebhan requested changes

View reviewed changes

Member

srebhan left a comment

Thanks for the cool plugin @bbergshaven! I do have some minor comments in the code. One bigger issue is the missing test-cases. Please provide unit-tests especially for the key-and metric-generation! In the best case, you provide a full integration test, but you should at least be able to provide a mock backend to check the validity of generated metrics...

plugins/outputs/clarify/README.md Outdated Show resolved Hide resolved

plugins/outputs/clarify/clarify.go Show resolved Hide resolved

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

plugins/outputs/clarify/clarify.go Show resolved Hide resolved

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

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

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

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

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

plugins/outputs/clarify/sample.conf Outdated Show resolved Hide resolved

bbergshaven force-pushed the clarify-output-plugin branch 2 times, most recently from 7a6c7de to fce840e Compare

May 3, 2023 13:31

Contributor Author

bbergshaven commented May 3, 2023 •

edited

Loading

Thank you @srebhan for a great and swift review.
I have taken most of your feedback into consideration and pushed fixes.
Left some comments on some of the specific parts that I would like to keep.

Looking forward to hear back from you!

bbergshaven force-pushed the clarify-output-plugin branch from fce840e to 4e6dc0d Compare

May 3, 2023 13:51

bbergshaven requested a review from srebhan

May 3, 2023 14:56

bbergshaven force-pushed the clarify-output-plugin branch from f5fecef to 1e15027 Compare

May 8, 2023 21:30

Hipska suggested changes

View reviewed changes

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

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

plugins/outputs/clarify/sample.conf Outdated Show resolved Hide resolved

plugins/outputs/clarify/sample.conf Show resolved Hide resolved

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

bbergshaven force-pushed the clarify-output-plugin branch 2 times, most recently from 5238d76 to fc61ce9 Compare

May 9, 2023 20:57

srebhan reviewed

View reviewed changes

Member

srebhan left a comment

@bbergshaven thanks for the update! I have some more smaller comments, but we are quite close...

docs/LICENSE_OF_DEPENDENCIES.md Outdated Show resolved Hide resolved

plugins/outputs/clarify/sample.conf Outdated Show resolved Hide resolved

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

plugins/outputs/clarify/clarify.go Show resolved Hide resolved

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

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

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

plugins/outputs/clarify/clarify_test.go Outdated Show resolved Hide resolved

plugins/outputs/clarify/clarify_test.go Outdated Show resolved Hide resolved

plugins/outputs/clarify/clarify_test.go Outdated Show resolved Hide resolved

Hipska suggested changes

View reviewed changes

Contributor

Hipska left a comment

Whoops, forgot to submit my review..

plugins/outputs/clarify/README.md Outdated Show resolved Hide resolved

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

plugins/outputs/clarify/sample.conf Outdated Show resolved Hide resolved

bbergshaven force-pushed the clarify-output-plugin branch 2 times, most recently from 0883521 to 9629b6e Compare

May 15, 2023 22:19

srebhan reviewed

View reviewed changes

Member

srebhan left a comment

Still some formatting comments and simplifications...

internal/type_conversions.go Outdated Show resolved Hide resolved

internal/type_conversions.go Outdated Show resolved Hide resolved

internal/type_conversions.go Outdated Show resolved Hide resolved

plugins/outputs/clarify/sample.conf Outdated Show resolved Hide resolved

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

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

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

Hipska suggested changes

View reviewed changes

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

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

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

bbergshaven added 8 commits

May 22, 2023 15:08


          feat: Add Clarify output plugin

b55f708

Add plugin to support writing metrics to Clarify.


          fixups

f1fe72a

- fix change requests
- add tests


          fix tests

18b1000

Fix failing tests due to the fact that FieldList returns the fields as a slice in an undefined order.


          fix gosum


          Fixups

080cf4b

- Add missing header to sample.conf and README
- Add user configurable tag to specify tag to use for inputID
- Remove ambiguous default value for optional config option


          use Init method

c0fcf2a

- Move client setup from Connect to Init method.


          Add bool to internal conversion methods

8eeb9f1


          fixups

60cfb79

bbergshaven force-pushed the clarify-output-plugin branch from 9629b6e to f446694 Compare

May 22, 2023 13:11

bbergshaven force-pushed the clarify-output-plugin branch from f446694 to 6282cc5 Compare

May 22, 2023 13:23

Hipska suggested changes

View reviewed changes

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

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

plugins/outputs/clarify/README.md Outdated Show resolved Hide resolved

bbergshaven force-pushed the clarify-output-plugin branch from 6282cc5 to 34ad1b4 Compare

May 22, 2023 13:35


          fixups

5d4a1c5

- simplify bool type conversion
- test credentials to ensure only one type is used
- fix error messages
- correctly indent sample.conf
- Update README

bbergshaven force-pushed the clarify-output-plugin branch from 34ad1b4 to 5d4a1c5 Compare

May 22, 2023 13:40

Hipska approved these changes

View reviewed changes

Hipska added the ready for final review label

Contributor

telegraf-tiger bot commented May 22, 2023

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

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

📦 Click here to get additional PR build artifacts

Artifact URLs

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

srebhan approved these changes

View reviewed changes

Member

srebhan left a comment

Looks good to me. Thanks you very much for your effort and contribution @bbergshaven!

srebhan assigned powersj and unassigned srebhan

powersj approved these changes

View reviewed changes

Contributor

powersj left a comment

Thanks for this - Will the Tools or Integration page of clarify.io get updated to reflect this?

The ID and series name creation does concern me. While I understand the need to differentiate data, it seems like if you have multiple tags and the order switched around, this could cause confusion?

powersj merged commit df166cf into influxdata:master

chenrui333 mentioned this pull request

telegraf 1.27.0 Homebrew/homebrew-core#133473

Merged

srebhan added this to the v1.27.0 milestone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat new plugin plugin/output ready for final review