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

feat: file-logger add vars #9712

Merged
merged 19 commits into from
Jul 13, 2023
Merged

feat: file-logger add vars #9712

merged 19 commits into from
Jul 13, 2023

Conversation

kamly
Copy link
Contributor

@kamly kamly commented Jun 22, 2023

Description

file-loggerr plugins add vars

fixes #9596

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)

@kamly kamly changed the title feat: file-loggerr add vars feat: file-logger add vars Jun 22, 2023
@monkeyDluffy6017
Copy link
Contributor

@kamly please fix the code lint error

@monkeyDluffy6017
Copy link
Contributor

test cases and docs are needed

@kamly
Copy link
Contributor Author

kamly commented Jun 28, 2023

hi, @monkeyDluffy6017 all done

  • add test
  • add doc
  • fix code lint

@kamly
Copy link
Contributor Author

kamly commented Jun 30, 2023

hi, @monkeyDluffy6017 fix~

@kamly
Copy link
Contributor Author

kamly commented Jul 6, 2023

@monkeyDluffy6017 , fix again ~

@monkeyDluffy6017 monkeyDluffy6017 added wait for update wait for the author's response in this issue/PR and removed need test cases labels Jul 6, 2023
@Gallardot
Copy link
Member

Refer to include_req_body and include_req_body_expr to see if it is better to implement an include_req_header and include_req_header_expr feature so that other logging plugins can support this feature without each plugin implementing it separately.

@kamly @monkeyDluffy6017 WDYT?

@kamly
Copy link
Contributor Author

kamly commented Jul 8, 2023

Refer to include_req_body and include_req_body_expr to see if it is better to implement an include_req_header and include_req_header_expr feature so that other logging plugins can support this feature without each plugin implementing it separately.

@kamly @monkeyDluffy6017 WDYT?

Good idea, but I read in the docs that file-logger plugin doesn't support include_req_body_expr.

In the http-logger.lua

function _M.check_schema(conf, schema_type)
    if schema_type == core.schema.TYPE_METADATA then
        return core.schema.check(metadata_schema, conf)
    end

    local ok, err = core.schema.check(schema, conf)
    if not ok then
        return nil, err
    end
    return log_util.check_log_schema(conf)
end

Compared in the file-logger.lua

function _M.check_schema(conf, schema_type)
    if schema_type == core.schema.TYPE_METADATA then
        return core.schema.check(metadata_schema, conf)
    end
    return core.schema.check(schema, conf)
end

I think of these few steps:

  1. The file-logger plugin can support this feature first.
  2. This feature add to other loggers plugin.
  3. Another pr refactors file-logger plugin to support log_util.check_log_schema.

The main reason is that I need this feature to be used in a production environment, and it is a big challenge for me to refactor file-logger plugin. I want features to be implemented in small steps.

@Gallardot @monkeyDluffy6017 WDYT ?

@Gallardot
Copy link
Member

Good idea, but I read in the docs that file-logger plugin doesn't support include_req_body_expr.

In fact, file-logger plugin support include_req_body and include_req_body_expr. You can refer to the kafka plugin for configuration and have a try.

function _M.log(conf, ctx)
local entry = log_util.get_log_entry(plugin_name, conf, ctx)
write_file_data(conf, entry)

entry = get_full_log(ngx, conf)

if conf.include_req_body then

If it doesn't meet your scenario, you can implement the include_req_header and include_req_header_expr feature in get_full_log.

local function get_full_log(ngx, conf)

@kamly
Copy link
Contributor Author

kamly commented Jul 9, 2023

In fact, file-logger plugin support include_req_body and include_req_body_expr. You can refer to the kafka plugin for configuration and have a try.

I got it ~

But file-logger plugin does not support include_req_body_expr because the schema does not define include_req_body_expr .

local schema = {
type = "object",
properties = {
path = {
type = "string"
},
log_format = {type = "object"},
include_resp_body = {type = "boolean", default = false},
include_resp_body_expr = {
type = "array",
minItems = 1,
items = {
type = "array"
}
}
},
required = {"path"}
}

Maybe the author who wrote it before missed it, I can try to align kafka-logger plugin

@monkeyDluffy6017 WDYT ?

@kamly
Copy link
Contributor Author

kamly commented Jul 9, 2023

@Gallardot Please take a look ?

@Gallardot
Copy link
Member

Gallardot commented Jul 9, 2023

But file-logger plugin does not support include_req_body_expr because the schema does not define include_req_body_expr .

@kamly
Although it is not defined in the schema, but it can be used directly in file-logger plugin. Therefore, I think it is only necessary to clearly define this feature in file-logger so that users can know more clearly that this feature is supported.

@kamly
Copy link
Contributor Author

kamly commented Jul 9, 2023

@kamly Although it is not defined in the schema, but it can be used directly in file-logger plugin. Therefore, I think it is only necessary to clearly define this feature in file-logger so that users can know more clearly that this feature is supported.

@Gallardot got it, I have resubmitted following your solution. Please take a look ?

in 900e438

@kamly
Copy link
Contributor Author

kamly commented Jul 10, 2023

@monkeyDluffy6017 @Gallardot , please take a look

@monkeyDluffy6017
Copy link
Contributor

monkeyDluffy6017 commented Jul 11, 2023

so that other logging plugins can support this feature without each plugin implementing it separately.

Hi @Gallardot, I don't get your point, how can we do this? each plugin is independent

@Gallardot
Copy link
Member

Gallardot commented Jul 11, 2023

I don't get your point, how can we do this? each plugin is independent

@monkeyDluffy6017
Yes, each plugin is independent on runtime. However, in terms of implementation, some logging plug-ins use the same method log_util.get_log_entry.

@kamly
Copy link
Contributor Author

kamly commented Jul 12, 2023

@monkeyDluffy6017 What else do I need to update?

kamly and others added 4 commits July 13, 2023 09:23
Co-authored-by: Liu Wei <375636559@qq.com>
Co-authored-by: Liu Wei <375636559@qq.com>
@kamly
Copy link
Contributor Author

kamly commented Jul 13, 2023

@monkeyDluffy6017 all fix again ~

@monkeyDluffy6017 monkeyDluffy6017 added approved and removed wait for update wait for the author's response in this issue/PR labels Jul 13, 2023
@monkeyDluffy6017 monkeyDluffy6017 merged commit 94fc894 into apache:master Jul 13, 2023
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

help request: How to filter HEAD requests when using file-logger?
3 participants