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.

Sign up for GitHub

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

Jump to bottom

Use async-http in monitor plugin #2447

Merged
merged 12 commits into from
Jun 17, 2019
Merged

Use async-http in monitor plugin #2447

merged 12 commits into from
Jun 17, 2019

Conversation

ganmacs
Copy link
Member

@ganmacs ganmacs commented Jun 6, 2019 •
edited
Loading

depends on #2424, last 2 commits are for this patch.

Which issue(s) this PR fixes:
no

What this PR does / why we need it:

in_monitor_agent: Replace webrick base HTTP server with async-http base HTTP server.

Simple benchmark: https://gist.github.com/ganmacs/3f7984930a3cd9109bfbb8a2ca5d6991

Docs Changes:

no
Add http_server helper document

Release Note:

monitor_agent uses async-http base HTTP server instead of webrick base HTTP server

@ganmacs ganmacs requested a review from repeatedly June 6, 2019 07:21
@repeatedly repeatedly added the enhancement Feature request or improve operations label Jun 7, 2019
@repeatedly
Copy link
Member

Hmm... async doesn't support 2.2 or earlier but fluentd still supports these version in this year.
http helper looks good. How about using webrick when async-http is not available?

@repeatedly
Copy link
Member

The smart way is http helper implements compatibility layer.
The simple way is check require and choose in_monitr_agent_async/in_monitor_agent_webrick.

@ganmacs ganmacs force-pushed the use-sync-in-monitor-plugin branch 3 times, most recently from 72084c2 to c8f751b Compare June 10, 2019 05:29
@ganmacs
Copy link
Member Author

ganmacs commented Jun 10, 2019

I did the former way! c8f751b

@cosmo0920
Copy link
Contributor

Hi, these patches seems to be reasonable for me. 👍
Thank you for your hard work. 😄

I have a nitpick question.
Should we add test cases for http plugin helper?

@ganmacs
Experimental: Add http server helper which is async gem base
b7affc6 
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
@ganmacs
Replace webrick base http server in monitor_agent
2331dda 
with http server helper which is async base

Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
@ganmacs
Fix typo
953d725 
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
@ganmacs
Add compat layer which use webrick for async gem
9388e78 
becuase it does not support ruby version < 2.3

Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
@ganmacs
change helper name http to http_server
f458fae 
http is a bit ambiguous

Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
@ganmacs
Use stop method to make http server stop automatically
3e19005 
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
@ganmacs ganmacs force-pushed the use-sync-in-monitor-plugin branch from c8f751b to 569257e Compare June 11, 2019 08:45
@ganmacs ganmacs changed the title Use async-http in monitor plugin [WIP] Use async-http in monitor plugin Jun 11, 2019
@ganmacs
Copy link
Member Author

ganmacs commented Jun 11, 2019

I'm going to add more tests later :) (So I changed this PR title into the one which starts with [WIP] )

@ganmacs ganmacs force-pushed the use-sync-in-monitor-plugin branch from 569257e to 8611f67 Compare June 12, 2019 09:42
@ganmacs
Add test and fix some bugs
66def29 
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
@ganmacs
Remove unnecessary method
e02735d 
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
@ganmacs
http server should stop before thread stop
264cf48 
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
@ganmacs
test plain is correct
7947e54 
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
@ganmacs ganmacs force-pushed the use-sync-in-monitor-plugin branch from 8611f67 to 7947e54 Compare June 13, 2019 04:35
@ganmacs ganmacs changed the title [WIP] Use async-http in monitor plugin se async-http in monitor plugin Jun 13, 2019
@ganmacs ganmacs changed the title se async-http in monitor plugin Use async-http in monitor plugin Jun 13, 2019
@ganmacs
Copy link
Member Author

ganmacs commented Jun 13, 2019

Ready to be reviewed! @cosmo0920 @repeatedly could you review this?

cosmo0920
cosmo0920 approved these changes Jun 15, 2019
View reviewed changes
Copy link
Contributor

@cosmo0920 cosmo0920 left a comment

Choose a reason for hiding this comment

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

Looks good to me. 😄

These http_server plugin helper implementation is greater than I had thought. 😁

repeatedly
repeatedly reviewed Jun 17, 2019
View reviewed changes
lib/fluent/plugin_helper/http_server/server.rb Outdated
end

def start(notify = nil)
@logger.debug("Start HTTP server listening #{@uri}")
Copy link
Member

Choose a reason for hiding this comment

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

For debug, I want async-http in the log like below

Start async HTTP server listening #{@uri}"

lib/fluent/plugin_helper/http_server/compat/server.rb
end

def start(notify = nil)
build_handler
Copy link
Member

Choose a reason for hiding this comment

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

logger.debug with Start webrick HTTP server listening

Copy link
Member Author

Choose a reason for hiding this comment

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

4b48997 fixed

repeatedly
repeatedly reviewed Jun 17, 2019
View reviewed changes
lib/fluent/plugin/in_monitor_agent.rb Outdated
render_json(resp, code: code, pretty_json: pretty_json)
end

def render_json(obj, code: 200, **opts)
Copy link
Member

Choose a reason for hiding this comment

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

render_json accepts only pretty_json so how about replace **opts with pretty_json: false?

Copy link
Member Author

Choose a reason for hiding this comment

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

right 42fd38f

@ganmacs
Fix debug message to tell that running server is webrick or aysnc
4b48997 
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
@ganmacs
Specify argument explicitly
42fd38f 
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
@repeatedly repeatedly merged commit 8e0d69a into fluent:master Jun 17, 2019
@repeatedly
Copy link
Member

Thanks!

@ganmacs ganmacs deleted the use-sync-in-monitor-plugin branch June 18, 2019 03:05
This was referenced Jun 19, 2019
Merged
Merged
Closed
@repeatedly
Copy link
Member

@ganmacs Could you send the patch to add http_server article to gitbook?

@ganmacs
Copy link
Member Author

ganmacs commented Jul 2, 2019

will do!

@ganmacs ganmacs mentioned this pull request Jul 2, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@repeatedly repeatedly repeatedly left review comments

@cosmo0920 cosmo0920 cosmo0920 approved these changes

Assignees
No one assigned
Labels
enhancement Feature request or improve operations
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ganmacs @repeatedly @cosmo0920