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

Ftr: AccessLogFilter support #214

Merged
merged 10 commits into from
Sep 26, 2019

Conversation

flycash
Copy link
Member

@flycash flycash commented Sep 22, 2019

What this PR does:
Add AccessLogFilter

Which issue(s) this PR fixes:
Ftr: AccessLogFilter support

Fixes #

Special notes for your reviewer:
Please review the code about channel and file operation carefully. I'm new to Golang.

Does this PR introduce a user-facing change?:


@flycash flycash force-pushed the feature/access_log_filter branch from 1a0e3ec to c5082fd Compare September 22, 2019 14:38
@codecov-io
Copy link

codecov-io commented Sep 22, 2019

Codecov Report

Merging #214 into develop will increase coverage by 0.81%.
The diff coverage is 72.8%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #214      +/-   ##
===========================================
+ Coverage    65.16%   65.98%   +0.81%     
===========================================
  Files           94       95       +1     
  Lines         6399     6511     +112     
===========================================
+ Hits          4170     4296     +126     
+ Misses        1792     1774      -18     
- Partials       437      441       +4
Impacted Files Coverage Δ
config/service_config.go 65.51% <100%> (+0.4%) ⬆️
protocol/dubbo/listener.go 64.88% <100%> (+14.88%) ⬆️
filter/impl/access_log_filter.go 71.02% <71.02%> (ø)
cluster/cluster_impl/base_cluster_invoker.go 54.71% <0%> (-16.99%) ⬇️
filter/impl/hystrix_filter.go 68.64% <0%> (-3.39%) ⬇️
protocol/dubbo/readwriter.go 70.37% <0%> (+2.46%) ⬆️
protocol/dubbo/codec.go 82.35% <0%> (+5.88%) ⬆️
protocol/dubbo/pool.go 73.6% <0%> (+15.73%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfbdd01...ec06f21. Read the comment docs.

filter/impl/access_log_filter.go Show resolved Hide resolved
filter/impl/access_log_filter.go Outdated Show resolved Hide resolved
filter/impl/access_log_filter.go Outdated Show resolved Hide resolved
filter/impl/access_log_filter.go Outdated Show resolved Hide resolved
filter/impl/access_log_filter.go Outdated Show resolved Hide resolved
filter/impl/access_log_filter_test.go Outdated Show resolved Hide resolved
@flycash flycash force-pushed the feature/access_log_filter branch 4 times, most recently from 6657456 to db80eae Compare September 23, 2019 02:38
filter/impl/access_log_filter.go Outdated Show resolved Hide resolved
filter/impl/access_log_filter_test.go Outdated Show resolved Hide resolved
@flycash flycash force-pushed the feature/access_log_filter branch from db80eae to 9c4ae4e Compare September 23, 2019 10:46
Copy link
Contributor

@hxmhlt hxmhlt left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@fangyincheng fangyincheng left a comment

Choose a reason for hiding this comment

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

Do you test in the examples for your feature?

filter/impl/access_log_filter.go Outdated Show resolved Hide resolved
@fangyincheng
Copy link
Contributor

How to start this filter?

@flycash
Copy link
Member Author

flycash commented Sep 24, 2019

Now I add the example in general/go-server.
It looks like that:
image

and clients need to do nothing

@AlexStocks
Copy link
Contributor

@fangyincheng should the examples be placed in dubbogo-examples?

@flycash
Copy link
Member Author

flycash commented Sep 24, 2019

I change the example to be comments:
image

It would be better.

@fangyincheng
Copy link
Contributor

@fangyincheng should the examples be placed in dubbogo-examples?

yes

@fangyincheng
Copy link
Contributor

I change the example to be comments:
image

It would be better.

Should it be default or user configuration? What about Java dubbo? And should you add some comments in access_log_filter.go instead of examples?

@flycash
Copy link
Member Author

flycash commented Sep 24, 2019

It's a default filter in Java. But if the user does not specify the value of "accesslog" to store the access log. It won't do anything.
So, maybe we could make it as default filter. But if the user doesn't configure the of "accesslog", we do nothing.

@fangyincheng
Copy link
Contributor

It's a default filter in Java. But if the user does not specify the value of "accesslog" to store the access log. It won't do anything.
So, maybe we could make it as default filter. But if the user doesn't configure the of "accesslog", we do nothing.

you are right. Pls do it.

@flycash
Copy link
Member Author

flycash commented Sep 25, 2019

done, pls review that

@flycash flycash force-pushed the feature/access_log_filter branch from 5ffe461 to 2fa5a79 Compare September 25, 2019 10:39
@flycash flycash force-pushed the feature/access_log_filter branch from 568e015 to 8600f70 Compare September 25, 2019 14:30
@AlexStocks
Copy link
Contributor

LGTM

@AlexStocks
Copy link
Contributor

@fangyincheng u recheck it and merge it, pls.

@flycash flycash force-pushed the feature/access_log_filter branch from 8600f70 to 25c3bf9 Compare September 25, 2019 15:09
@flycash flycash force-pushed the feature/access_log_filter branch from 7d01558 to ec06f21 Compare September 26, 2019 07:29
Copy link
Contributor

@fangyincheng fangyincheng left a comment

Choose a reason for hiding this comment

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

LGTM

@AlexStocks AlexStocks merged commit ab77c92 into apache:develop Sep 26, 2019
@flycash flycash deleted the feature/access_log_filter branch September 29, 2019 14:19
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