Skip to content
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

fix: remove redundant name attribute from logging plugins #10476

Closed
wants to merge 4 commits into from

Conversation

kayx23
Copy link
Member

@kayx23 kayx23 commented Nov 11, 2023

Description

Unique ID for batch processor (attribute name) are hardcoded in for quite many logging plugins. The docs, schema of certain plugins, and tests, previously treat name as a configurable attribute, which could create confusions, such as #10474.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • 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 first)

@kayx23
Copy link
Member Author

kayx23 commented Nov 13, 2023

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.

@moonming
Copy link
Member

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

pottekkat
pottekkat previously approved these changes Nov 13, 2023
Copy link
Member

@pottekkat pottekkat left a comment

Choose a reason for hiding this comment

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

LGTM and +1 to go ahead and fix this in other plugins as well.

@kayx23
Copy link
Member Author

kayx23 commented Nov 13, 2023

Just saw in clickhouse-logger and sys-logger the namea are indeed configurable:

local batch_processor_manager = bp_manager_mod.new(plugin_name)
local schema = {
type = "object",
properties = {
-- deprecated, use "endpoint_addrs" instead
endpoint_addr = core.schema.uri_def,
endpoint_addrs = {items = core.schema.uri_def, type = "array", minItems = 1},
user = {type = "string", default = ""},
password = {type = "string", default = ""},
database = {type = "string", default = ""},
logtable = {type = "string", default = ""},
timeout = {type = "integer", minimum = 1, default = 3},
name = {type = "string", default = "clickhouse logger"},

image

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)?

@kayx23 kayx23 dismissed stale reviews from pottekkat and shreemaan-abhishek via 4e94ddd November 13, 2023 18:10
@kayx23 kayx23 changed the title docs: remove name attribute from kafka-logger plugin docs: remove name attribute from logging plugins Nov 13, 2023
@moonming
Copy link
Member

moonming commented Nov 14, 2023

Just saw in clickhouse-logger and sys-logger the namea are indeed configurable:

local batch_processor_manager = bp_manager_mod.new(plugin_name)
local schema = {
type = "object",
properties = {
-- deprecated, use "endpoint_addrs" instead
endpoint_addr = core.schema.uri_def,
endpoint_addrs = {items = core.schema.uri_def, type = "array", minItems = 1},
user = {type = "string", default = ""},
password = {type = "string", default = ""},
database = {type = "string", default = ""},
logtable = {type = "string", default = ""},
timeout = {type = "integer", minimum = 1, default = 3},
name = {type = "string", default = "clickhouse logger"},

image

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)?

Option 2 is the clear choice for me

@moonming moonming closed this Nov 14, 2023
@moonming moonming reopened this Nov 14, 2023
@kayx23 kayx23 changed the title docs: remove name attribute from logging plugins fix: remove redundant name attribute from logging plugins Nov 14, 2023
@kayx23 kayx23 removed the approved label Nov 14, 2023
@kayx23
Copy link
Member Author

kayx23 commented Nov 14, 2023

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.

@kayx23 kayx23 closed this Nov 14, 2023
@kayx23 kayx23 deleted the kafka-batch-processor-name branch November 16, 2023 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants