feat(outputs.nats): Introduce NATS Jetstream option #14236

neelayu · 2023-11-01T17:23:49Z

Required for all PRs

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

resolves #14235

powersj

Thanks for the PR! I have added some initial high-level questions.

Additionally, please resolve the test issues:

plugins/outputs/nats/nats.go:124:29  revive   unused-parameter: parameter 'stream' seems to be unused, consider removing or renaming it as _
plugins/outputs/nats/nats.go:124:29  unparam  `(*NATS).createStream` - `stream` is unused

plugins/outputs/nats/nats_test.go

plugins/outputs/nats/nats.go

plugins/outputs/nats/README.md

plugins/outputs/nats/nats.go

plugins/outputs/nats/sample.conf

plugins/outputs/nats/nats.go

neelayu · 2023-11-17T12:18:30Z

@powersj and @Hipska Sorry I was occupied with work, so did not get a chance to review the comments. I'll address them in the coming week. Thank you for your patience

powersj

Thank you for the updates! I think if we can get the config cleaned up, we are nearly there.

Thanks again!

plugins/outputs/nats/nats.go

plugins/outputs/nats/README.md

plugins/outputs/nats/nats.go

Hipska · 2023-12-14T10:30:22Z

plugins/outputs/nats/sample.conf

+  ## Jetstream specific configuration. If specified, telegraf will use Jetstream to publish messages
+  # [outputs.nats.jetstream]
+    ## Specifies whether telegraf should create the stream at the startup or not. It will only create if it doesn't exist.
+    # auto_create_stream = true


This default value is missing from the init or Init methods, so default currently is false instead of true.

reworked the logic. can you go through it again?

plugins/outputs/nats/nats_test.go

powersj

Thanks for the large update! I have a couple suggestions to clean up the config and I want to chat with the other maintainer about the use of the TOML parser in the plugin. Otherwise, this looks great.

plugins/outputs/nats/nats.go

plugins/outputs/nats/README.md

plugins/outputs/nats/sample.conf

plugins/outputs/nats/nats.go

plugins/outputs/nats/nats_test.go

Hipska

looks good, still have a look at the linters as they still fail.

powersj

Thanks for driving this!

I rebased on master and cleaned up the one lint issue.

srebhan

@neelayu thanks for the nice contribution. I do have some comments...

plugins/outputs/nats/sample.conf

plugins/outputs/nats/nats.go

srebhan · 2024-01-18T11:47:49Z

plugins/outputs/nats/nats.go

+		if err != nil {
+			return fmt.Errorf("failed to create or update stream: %w", err)
+		}
+		n.Log.Infof("stream (%s) successfully created or updated", n.Jetstream.Name)


Shouldn't this be a debug message? In any case it should start with an uppercase letter...

srebhan · 2024-01-18T11:52:51Z

plugins/outputs/nats/nats.go

+func (n *NATS) convertToJetstreamConfig(streamConfig *jetstream.StreamConfig) {
+	telegrafStreamConfig := reflect.ValueOf(n.Jetstream).Elem()
+	natsStreamConfig := reflect.ValueOf(streamConfig).Elem()
+	for i := 0; i < telegrafStreamConfig.NumField(); i++ {
+		destField := natsStreamConfig.FieldByName(telegrafStreamConfig.Type().Field(i).Name)
+		if destField.IsValid() && destField.CanSet() {
+			destField.Set(telegrafStreamConfig.Field(i))
+		}
+	}
+}


This has footgun potential as you circumvent Golangs type-checking. If for example one field is removed upstream (or the type is changed) you silently will ignore the Telegraf setting and nobody will notice until some unexpected behavior happens. I really would feel better if we explicitly set the config-fields here (even though this bloats the code) or if we use the jetstream.StreamConfig directly in the Telegraf configuration.

Using jetstream.StreamConfig directly was done before, but rejected since it didn't have toml conversion, but only json.

Well it is not absolutely necessary to have toml tags, but I agree and would prefer the explicit setting of options. The question is, do we really need to expose all those options or can we live with the defaults?

Yes it is not necessary to have toml tags. but there are some fields which did not support toml unmarshalling such as retention, limits. This is the reason why in the configuration, users are expected to pass integer values for such fields because it we can resuse the Stringer interface defined on those types.

There are use cases when we definitely need full set of options.

@neelayu ok, so we can go with the full set of options, but I really think we should not hamper the user experience with the integers just because it's more convenient to use the String() interface. So please use speaking-strings where possible and do the explicit transformation. @powersj what do you think?

Sounds good to me

Will make the changes.

…tput plugin

Co-authored-by: Joshua Powers <powersj@fastmail.com> Co-authored-by: Thomas Casteleyn <thomas.casteleyn@me.com>

srebhan

@neelayu thanks for the update, just a few more minor comments. I added suggestions so it is quick for you to change those. I do have one more question about subject appending (in the code) and would love to hear your opinion there.

plugins/outputs/nats/nats.go

plugins/outputs/nats/nats_test.go

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

neelayu · 2024-01-22T10:36:26Z

@srebhan Thanks for your code suggestions. One important thing I learnt in this PR is, even if the code becomes verbose, its okay to do it. Its nice to see that telegraf doesn't compromise on code quality 😄

plugins/outputs/nats/nats.go

telegraf-tiger · 2024-01-23T06:00:18Z

Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.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

@neelayu thank you for all the effort you put into this PR!

Co-authored-by: Joshua Powers <powersj@fastmail.com> Co-authored-by: Thomas Casteleyn <thomas.casteleyn@me.com>

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

powersj reviewed Nov 1, 2023

View reviewed changes

srebhan assigned powersj Nov 2, 2023

powersj added the waiting for response waiting for response from contributor label Nov 7, 2023

Hipska suggested changes Nov 14, 2023

View reviewed changes

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

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

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

telegraf-tiger bot removed the waiting for response waiting for response from contributor label Nov 17, 2023

neelayu force-pushed the nats-js branch from 9dbe42a to 2c7436a Compare November 30, 2023 10:24

neelayu requested review from Hipska and powersj November 30, 2023 10:26

powersj reviewed Nov 30, 2023

View reviewed changes

Hipska suggested changes Dec 14, 2023

View reviewed changes

neelayu force-pushed the nats-js branch 2 times, most recently from 631cedb to 7b876c6 Compare January 5, 2024 13:02

powersj reviewed Jan 5, 2024

View reviewed changes

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

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

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

powersj reviewed Jan 5, 2024

View reviewed changes

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

Hipska suggested changes Jan 8, 2024

View reviewed changes

plugins/outputs/nats/nats_test.go Outdated Show resolved Hide resolved

plugins/outputs/nats/nats_test.go Show resolved Hide resolved

Hipska reviewed Jan 15, 2024

View reviewed changes

plugins/outputs/nats/nats_test.go Outdated Show resolved Hide resolved

Hipska approved these changes Jan 15, 2024

View reviewed changes

powersj force-pushed the nats-js branch from 2a6a552 to 40a0966 Compare January 17, 2024 16:36

powersj approved these changes Jan 17, 2024

View reviewed changes

powersj 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 Jan 17, 2024

powersj assigned srebhan and unassigned powersj Jan 17, 2024

Hipska approved these changes Jan 18, 2024

View reviewed changes

srebhan reviewed Jan 18, 2024

View reviewed changes

srebhan changed the title ~~feat(outputs.nats): Introduce NATS Jetstream as an option for NATS output plugin~~ feat(outputs.nats): Introduce NATS Jetstream option Jan 18, 2024

neelayu added 2 commits January 21, 2024 16:48

feat(outputs.nats): Introduce NATS Jetstream as an option for NATS ou…

b4cd550

…tput plugin

fix configuration parameter names

2a28cec

neelayu and others added 12 commits January 21, 2024 16:48

fix test

809a483

introduce json based stream create config

32e5c97

auto generate readme

a530d78

use require in tests

f361dbc

remove unused line

be84ca4

rework the configuration logic

ad82f7c

simplify connect process

0a42d35

Apply suggestions from code review

dba791f

Co-authored-by: Joshua Powers <powersj@fastmail.com> Co-authored-by: Thomas Casteleyn <thomas.casteleyn@me.com>

go fmt

1ee03a2

define streamconfig struct in telegraf

869c639

fix linter

2f18c9e

force string data types for config and changes in unit tests

2ecf9f3

neelayu force-pushed the nats-js branch from f4e7251 to bd4ec9e Compare January 21, 2024 11:19

reverse expected and actual in tests

204d355

neelayu force-pushed the nats-js branch from bd4ec9e to 204d355 Compare January 21, 2024 12:02

srebhan reviewed Jan 22, 2024

View reviewed changes

neelayu and others added 2 commits January 22, 2024 15:59

Apply suggestions from code review

9606d08

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

address review comments

e3adc7d

Hipska reviewed Jan 22, 2024

View reviewed changes

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

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

move to init

fb528eb

Hipska approved these changes Jan 23, 2024

View reviewed changes

srebhan approved these changes Jan 23, 2024

View reviewed changes

srebhan merged commit 06c1366 into influxdata:master Jan 23, 2024
26 checks passed

github-actions bot added this to the v1.30.0 milestone Jan 23, 2024

hhiroshell pushed a commit to hhiroshell/telegraf that referenced this pull request Feb 1, 2024

feat(outputs.nats): Introduce NATS Jetstream option (influxdata#14236)

0d20cd0

Co-authored-by: Joshua Powers <powersj@fastmail.com> Co-authored-by: Thomas Casteleyn <thomas.casteleyn@me.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(outputs.nats): Introduce NATS Jetstream option #14236

feat(outputs.nats): Introduce NATS Jetstream option #14236

neelayu commented Nov 1, 2023

powersj left a comment

neelayu commented Nov 17, 2023

powersj left a comment

Hipska Dec 14, 2023

neelayu Jan 5, 2024

powersj left a comment

Hipska left a comment

powersj left a comment

srebhan left a comment

srebhan Jan 18, 2024

srebhan Jan 18, 2024

Hipska Jan 18, 2024

srebhan Jan 18, 2024

neelayu Jan 19, 2024

srebhan Jan 19, 2024

powersj Jan 19, 2024

neelayu Jan 19, 2024

srebhan left a comment

neelayu commented Jan 22, 2024

telegraf-tiger bot commented Jan 23, 2024

Artifact URLs

srebhan left a comment •

edited

Loading

feat(outputs.nats): Introduce NATS Jetstream option #14236

feat(outputs.nats): Introduce NATS Jetstream option #14236

Conversation

neelayu commented Nov 1, 2023

Required for all PRs

powersj left a comment

Choose a reason for hiding this comment

neelayu commented Nov 17, 2023

powersj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

powersj left a comment

Choose a reason for hiding this comment

Hipska left a comment

Choose a reason for hiding this comment

powersj left a comment

Choose a reason for hiding this comment

srebhan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srebhan left a comment

Choose a reason for hiding this comment

neelayu commented Jan 22, 2024

telegraf-tiger bot commented Jan 23, 2024

Artifact URLs

srebhan left a comment • edited Loading

Choose a reason for hiding this comment

srebhan left a comment •

edited

Loading