Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

feat(outputs.nats): Use Jetstream publisher when using Jetstream #16570

Merged
merged 1 commit into from
Mar 5, 2025
Merged

feat(outputs.nats): Use Jetstream publisher when using Jetstream #16570

merged 1 commit into from
Mar 5, 2025
+5 −2

Conversation

kfollesdal
Copy link
Contributor

@kfollesdal kfollesdal commented Mar 3, 2025 •
edited
Loading

Summary

The NATS output plugin use the NATS Core publisher both for Core and Jetstream. This can be problematic when publishing many messages to a Jetsteam, you do not have pack and can overload the server. This PR change publisher to use Jetstream publisher insted of Core when publishing to Jetstream.

Checklist

  • No AI generated code was used in this PR

Related issues

resolves #16571

@telegraf-tiger 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 Mar 3, 2025
@kfollesdal kfollesdal force-pushed the nats-publish-jetstream branch 3 times, most recently from 73b824c to eb28751 Compare March 3, 2025 14:58
srebhan
srebhan reviewed Mar 3, 2025
View reviewed changes
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kfollesdal for your contribution! How about keeping the publish path synchronous? Do you plan to move this out of draft soon? If so we might be able to get this into the next feature release on Monday...

@srebhan srebhan self-assigned this Mar 3, 2025
@kfollesdal
Copy link
Contributor Author

kfollesdal commented Mar 3, 2025 •
edited
Loading

Thanks @srebhan, will try to finish up the next two days. You prefer synchronous over asynchronous? Think you will get some performance loss vs core implementation by using synchronous JetStream publisher since you then have to wait on each publish ack instead of check all at once at the end.

@kfollesdal
Copy link
Contributor Author

But of order is important synchronous is probably best. And then we do not get any breaking change. But maybe an option to choose between asynchronous and synchronous to keep high throughput as with core publisher?

@srebhan
Copy link
Member

srebhan commented Mar 4, 2025

Let's first fix this issue and discuss synchronous vs. asynchronous in a separate PR! What do you think?

@kfollesdal
feat(outputs.nats): Use Jetstream publisher when using Jetstream
977bea8 
The NATS output plugin use the NATS Core publisher both for
Core and Jetstream. This can be problematic when publishing many
messages to a Jetsteam, e.w you can overload the server. This
commit change publisher to use Jetstream publisher insted of
Core when publishing to Jetstream.

_sync
@kfollesdal kfollesdal force-pushed the nats-publish-jetstream branch from eb28751 to 977bea8 Compare March 4, 2025 13:41
@kfollesdal
Copy link
Contributor Author

Let's first fix this issue and discuss synchronous vs. asynchronous in a separate PR! What do you think?

Good plan. Have updated PR to use synchronous jetstream publisher insted of proposed asynchronous. We get lower throughput but no braking change with ordering. Asynchronous publish should probably be optional with an option. But that is for another PR.

@kfollesdal kfollesdal marked this pull request as ready for review March 4, 2025 13:52
@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Mar 4, 2025

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 change the Telegraf binary size

📦 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

@kfollesdal kfollesdal mentioned this pull request Mar 4, 2025
Open
@kfollesdal
Copy link
Contributor Author

@srebhan I don't see any indication that the failing integration test is related to NATS.

srebhan
srebhan approved these changes Mar 4, 2025
View reviewed changes
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kfollesdal!

@srebhan 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 Mar 4, 2025
@srebhan srebhan assigned DStrand1 and unassigned srebhan Mar 4, 2025
@srebhan srebhan mentioned this pull request Mar 4, 2025
Open
1 task
@DStrand1 DStrand1 merged commit 26a692a into influxdata:master Mar 5, 2025
26 of 27 checks passed
@github-actions github-actions bot added this to the v1.34.0 milestone Mar 5, 2025
@kfollesdal kfollesdal deleted the nats-publish-jetstream branch March 5, 2025 15:43
asaharn pushed a commit to asaharn/telegraf that referenced this pull request Mar 10, 2025
@kfollesdal @asaharn
feat(outputs.nats): Use Jetstream publisher when using Jetstream (inf…
dfad97e 
…luxdata#16570)
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@srebhan srebhan srebhan approved these changes

@DStrand1 DStrand1 DStrand1 approved these changes

Assignees

@DStrand1 DStrand1

Labels
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 ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Milestone
v1.34.0
Development

Successfully merging this pull request may close these issues.

[Output NATS]: Use Jetstream publisher insted of Core when publish to Jetstream
3 participants
@kfollesdal @srebhan @DStrand1