Revert "feat(http-logger): support for specified the log formats via …

[moonming]

…admin API (#2294)"

This reverts commit 89f89f3.

The branch was merged without replying to the PMC member’s question #2294 (comment) #2294 (comment) , so I ask to revert this PR.

What this PR does / why we need it:

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?

[juzhiyuan]

Not sure if that PR is needed, maybe there should ping @membphis.

[ShiningRush]

We can continue to discuss its necessity in the original PR
If it is a non-essential function point, we could revert it

[moonming]

@juzhiyuan @ShiningRush I think we should revert this PR first, then revert this revert if needed.
This has nothing to do with the feature, but about how to merge PR. I think it is impolite to ignore the comments and merge directly.

[membphis]

#2294 (comment)

sorry for missing them. I reply to you here.

why not add log_format in http-logger plugin directly?

the plugin_metadata can be the default value of plugin, and it supports a way we can control it by admin api.

Different routes may have different log formats

That is not a good way. Like Nginx, only one access.log, all of the requests will generate access log to access.log.
I do not think our user need this feature.

[membphis]

#2294 (comment)
why we need this PR?

#2294 (comment)

I have wrote the description for this PR. please take a look first.

And I posted the use case.

[ShiningRush]

I got it, this is a feature for sharing data in the same type of plugin. I think it is useful for users. For example, a plugin needs to use the redis connection string, but I don’t want to configure it in each plugin.this function is very helpful at this time.
However, the name may cause some misunderstandings. I think it may be easier to understand if it is called ShareData or other meaningful name.

[membphis]

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

Here is the answer: #2307 (comment)

BTW, here is the old way to support the default value for log-rotate plugin:
https://github.com/apache/apisix/blob/master/conf/config-default.yaml#L199-L202

this way is not firendly:

1. local file, if we want to update it, we need to update the file and reload APISIX instance.
2. no schema, the user may set a wong value.
3. each plugin needs to check the default value if it is right by itself.

but the /apisix/plugin_metadata fixed all of them, it is a standard way to set a default value for each plugin.

[membphis]

The branch was merged without replying to the PMC member’s question #2294 (comment) #2294 (comment) , so I ask to revert this PR.

have to say sorry for missing the comments. I submitted lots of commits when I was developing. As a result, I did not see your message on the PR :( .

[juzhiyuan]

Got it, we could revert it first, once most of the members agree to accept it, we could do a re-revert.

[membphis]

I think it may be easier to understand if it is called ShareData or other meaningful name.

that is the another PR which implemented the plugin_metadata: #2268

/apisix/plugin_metadata/http-logger -> {log_format: {....}}

this value only can be used in plugin http-logger, it can not be used for other plugin.
so I think ShareData is not a good name for this case.

[membphis]

Got it, we could revert it first, once most of the members agree to accept it, we could do a re-revert.

I do not agree with it now. if we want to revert, we should to confirm the old PR is wong.

[ShiningRush]

We can concern a meaningful name, plugin_metadata is just a global configuration cross all plugins, not real meta data.

[membphis]

@moonming Different routes may have different log formats

that is not a good idea, another reason:
we also need to collect the access log of request when not matched any route.

[moonming]

Got it, we could revert it first, once most of the members agree to accept it, we could do a re-revert.

I do not agree with it now. if we want to revert, we should to confirm the old PR is wong.

Revert this pr is not because of this feature, but because this operation will affect review.

[juzhiyuan]

Got it, we could revert it first, once most of the members agree to accept it, we could do a re-revert.

I do not agree with it now. if we want to revert, we should to confirm the old PR is wong.

Revert that PR is not because THAT feature has something wrong, but just because there has some arguments about merging that PR, once most of the members accpet it, we could re-revert it.

[membphis]

revert it first. then I'll create a new PR soon