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
Throttle filter add template support #3819
Conversation
Build SUCCESS |
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.
LGTM, I have tried it out, it works as intended.
I found a crash :/ If I didn't set in the filter the template option. Example config
|
77d2881
to
fc1d8c2
Compare
Nice catch, the last commit also removed the check if the template was provided (77d2881). |
Build SUCCESS |
LGTM, approve. |
Signed-off-by: Kokan <kokaipeter@gmail.com>
Example: ``` throttle( rate(1) template("${.journald._UID}") ); ``` Signed-off-by: Kokan <kokaipeter@gmail.com>
Signed-off-by: Kokan <kokaipeter@gmail.com>
The new template support were added to throttle filter, a modification only requires users to change value("A") --> template("${A}"). The template is a better option to have, the only downside could be performance, but it is possible to use a lightweight version of template called trivial template that is quick. As all template containing only one NV key is trivial, this should not cause a regression. As the value option was never published in a release, it is safe to remove and only provide template option. Signed-off-by: Kokan <kokaipeter@gmail.com>
fc1d8c2
to
0908985
Compare
Only rebased on current master to make CI happy. |
Build SUCCESS |
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.
LGTM, approved!
This PR adds template supports, and as a last step removes
value()
option.value("A")
can be replaced withtemplate("${A}")
.This PR somewhat conflicts with:
I'll provide patches in those, but as the result no separate news entry is needed here.
Example config: