Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(http-logger): support for specified the log formats via admin API #2294

Merged
merged 12 commits into from
Sep 24, 2020
Merged

feat(http-logger): support for specified the log formats via admin API #2294

merged 12 commits into from
Sep 24, 2020

Conversation

membphis
Copy link
Member

@membphis membphis commented Sep 23, 2020

What this PR does / why we need it:

as title.

For example:

curl http://****/apisix/admin/plugin_metadata/http-logger -d '
{
    "log_format": {
        "host": "$host",
        "@timestamp": "$time_iso8601",
        "client_ip": "$remote_addr"
    }
}'

when we enabled plugin http-logger, we will get the message body like:

{"host":"localhost","@timestamp":"2020-09-23T18:29:07-04:00","client_ip":"127.0.0.1","route_id":"1"}
{"host":"localhost","@timestamp":"2020-09-23T18:29:07-04:00","client_ip":"127.0.0.1","route_id":"1"}

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible?

@membphis membphis requested a review from nic-chen September 23, 2020 23:07
@membphis membphis marked this pull request as ready for review September 24, 2020 01:25
@moonming
Copy link
Member

why not add log_format in http-logger plugin directly?

@moonming
Copy link
Member

Different routes may have different log formats

@@ -1025,7 +1025,7 @@ local function init_etcd(show_output)
for _, dir_name in ipairs({"/routes", "/upstreams", "/services",
"/plugins", "/consumers", "/node_status",
"/ssl", "/global_rules", "/stream_routes",
"/proto"}) do
"/proto", "/plugin_metadata"}) do
Copy link
Member

Choose a reason for hiding this comment

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

What is the difference between metadata and the properties of plugin itself? I didn't see why we need it.

Copy link
Member Author

Choose a reason for hiding this comment

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

get plugin schema:
/apisix/admin/plugins/{plugin-name}, eg: /apisix/admin/plugins/limit-req

get plugin metadata:
/apisix/admin/plugin_metadata/{plugin-name}, eg: /apisix/admin/plugin_metadata/example-plugin

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

why we need this PR?

Copy link
Member

@juzhiyuan juzhiyuan Sep 24, 2020

Choose a reason for hiding this comment

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

I would recommend using /plugins/{name}/metadata in a RESTful way if this PR is needed..

Copy link
Contributor

Choose a reason for hiding this comment

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

I am also curious about what scenarios need metadata instead of plugin's option.

Copy link
Member Author

Choose a reason for hiding this comment

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

this PR has been merged. we can talk in here: #2307 (comment)

@membphis membphis merged commit 89f89f3 into apache:master Sep 24, 2020
moonming added a commit to moonming/apisix that referenced this pull request Sep 24, 2020
membphis pushed a commit that referenced this pull request Sep 24, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants