-
Notifications
You must be signed in to change notification settings - Fork 5.7k
feat(outputs.nats): Allow providing a subject layout #17213
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.
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution @mgale! Can we please split this PR into two, one for the subject templating and one for the externally managed streams!? Feel free to create a new one and modify this for the other subject...
I have moved #17072 into: I will complete the cleanup of this PR shortly. |
b17b3ac
to
0a06dd2
Compare
0a06dd2
to
e90ffbc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @mgale! My question is, why not just treat the subject
setting as template instead of introducing another (redundant) setting? We've done the same in e.g. the remote file output...
Would you consider changing the subject from a string to a list with this change? If not, the primary concern with that approach arises when If the subject template is embedded as a single string (e.g., subject = "telegraf.{{ .GetTag "region" }}.{{ .GetTag "host" }}"), we run into a few challenges: I think this would make it difficult to correctly derive the subject wildcard needed for stream creation. |
@mgale I'm a bit lost here. Why does the first element has to be static if Telegraf is responsible for generating the stream? If we go for subject templates, you have to |
cca97ad
to
4757861
Compare
You’re right, the first element doesn’t need to be static. I had initially leaned that way for simplicity, but I’ve since refactored the code to better align with dynamic subject handling. Specifically: This avoids relying on the computed subject during Write to update the stream, which I think is important. I’d prefer to avoid performing stream registration or modification at write time if we can. Let me know if you’re okay with this approach and constraint. I’m open to alternatives, but I believe this keeps stream creation/update logic clean and predictable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update @mgale! Much better but too complex. ;-) Please find my suggestions and comments in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mgale! This looks much better. Some more comments from my side...
@srebhan I believe all feedback has now been addressed. Given the extensive back-and-forth on this PR, I’ve resolved all comments from previous reviews and believe all outstanding items have been covered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mgale for your patience! The code looks much cleaner now. Some more comments, especially regarding looping over the metrics only once...
… externally managed streams
…out handling into its own file
Improved error handling. Co-authored-by: Sven Rebhan <[email protected]>
Doc update Co-authored-by: Sven Rebhan <[email protected]>
Co-authored-by: Sven Rebhan <[email protected]>
b99f25d
to
fbe1668
Compare
… container is now reused and only defined once
ec6aa39
to
cb9030f
Compare
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
@srebhan I believe all comments have been addressed, let me know if there is further work required on this PR. |
Summary
This PR provides the following feature:
Checklist
Related issues
resolves #16558