You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "kayx23 (via GitHub)" <gi...@apache.org> on 2023/11/11 02:35:27 UTC
[PR] docs: remove name attribute from kafka-logger plugin [apisix]
kayx23 opened a new pull request, #10476:
URL: https://github.com/apache/apisix/pull/10476
### Description
Reason for this update is the unique ID for batch processor of this plugin is actually hardcoded and not a configurable option. See https://github.com/apache/apisix/blob/master/apisix/plugins/kafka-logger.lua#L26
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [ ] I have added tests corresponding to this change
- [x] I have updated the documentation to reflect this change
- [ ] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: remove redundant name attribute from logging plugins [apisix]
Posted by "kayx23 (via GitHub)" <gi...@apache.org>.
kayx23 commented on PR #10476:
URL: https://github.com/apache/apisix/pull/10476#issuecomment-1809719588
Ok as it turns out (thanks to the failed CI tests) that this `name` attribute is actually configurable and its value gets exported to prometheus to identify batch processors.
The reason why the `name` attribute was documented to each of these logging plugin, despite being a batch processor attribute, was likely because these loggers initiate batch processor instances with specific names (the hard coded values), so they are the default values specific to these plugins.
In short, these docs should not be removed and the documented default values are consistent with what goes in the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] docs: remove name attribute from kafka-logger plugin [apisix]
Posted by "kayx23 (via GitHub)" <gi...@apache.org>.
kayx23 commented on PR #10476:
URL: https://github.com/apache/apisix/pull/10476#issuecomment-1807351771
Lmk if I should apply this change for other plugins in the same PR. Quite a few logging plugins have the same issues if I'm not mistaken.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] docs: remove name attribute from logging plugins [apisix]
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on PR #10476:
URL: https://github.com/apache/apisix/pull/10476#issuecomment-1809469188
> Just saw in `clickhouse-logger` and `sys-logger` the namea are indeed configurable:
>
> https://github.com/apache/apisix/blob/d8bd34414239ed36942c19f435ac10ad0d5e57c0/apisix/plugins/clickhouse-logger.lua#L28-L41
>
> 
>
> Are we going to:
>
> 1. make the other logger plugin names configurable as well;
> 2. update `clickhouse logger` and `sys logger` to be consistent with others, which hard code the name; or
> 3. Just update the docs for those that are not configurable (done)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] docs: remove name attribute from logging plugins [apisix]
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming closed pull request #10476: docs: remove name attribute from logging plugins
URL: https://github.com/apache/apisix/pull/10476
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] docs: remove name attribute from kafka-logger plugin [apisix]
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on PR #10476:
URL: https://github.com/apache/apisix/pull/10476#issuecomment-1807379538
> Lmk if I should apply this change for other plugins in the same PR. Quite a few logging plugins have the same issues if I'm not mistaken.
yes, please do it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] docs: remove name attribute from kafka-logger plugin [apisix]
Posted by "kayx23 (via GitHub)" <gi...@apache.org>.
kayx23 commented on PR #10476:
URL: https://github.com/apache/apisix/pull/10476#issuecomment-1808721169
Just saw in `clickhouse-logger` the name is indeed configurable:
https://github.com/apache/apisix/blob/d8bd34414239ed36942c19f435ac10ad0d5e57c0/apisix/plugins/clickhouse-logger.lua#L28-L41
Are we going to:
1. make the other logger plugin names configurable as well; or
2. Just update the docs for those that are not configurable?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: remove redundant name attribute from logging plugins [apisix]
Posted by "kayx23 (via GitHub)" <gi...@apache.org>.
kayx23 closed pull request #10476: fix: remove redundant name attribute from logging plugins
URL: https://github.com/apache/apisix/pull/10476
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org