Skip to content

Use async-http in monitor plugin #2447

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
Merged
repeatedly merged 12 commits into from
Jun 17, 2019
Merged

Use async-http in monitor plugin #2447

repeatedly 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

repeatedly commented Jun 10, 2019

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

repeatedly commented Jun 10, 2019

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

cosmo0920 commented Jun 11, 2019

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 added 6 commits June 11, 2019 14:05
@ganmacs
Experimental: Add http server helper which is async gem base
b7affc6 
Signed-off-by: Yuta Iwama <[email protected]>
@ganmacs
Replace webrick base http server in monitor_agent
2331dda 
with http server helper which is async base

Signed-off-by: Yuta Iwama <[email protected]>
@ganmacs
Fix typo
953d725 
Signed-off-by: Yuta Iwama <[email protected]>
@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 <[email protected]>
@ganmacs
change helper name http to http_server
f458fae 
http is a bit ambiguous

Signed-off-by: Yuta Iwama <[email protected]>
@ganmacs
Use stop method to make http server stop automatically
3e19005 
Signed-off-by: Yuta Iwama <[email protected]>
@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 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

@repeatedly repeatedly Jun 17, 2019

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

@repeatedly repeatedly Jun 17, 2019

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

@ganmacs ganmacs Jun 17, 2019

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

@repeatedly repeatedly Jun 17, 2019

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

@ganmacs ganmacs Jun 17, 2019

Choose a reason for hiding this comment

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

right 42fd38f

@repeatedly repeatedly merged commit 8e0d69a into fluent:master Jun 17, 2019
@repeatedly
Copy link
Member

repeatedly commented Jun 17, 2019

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

repeatedly commented Jul 2, 2019

@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
@daipom daipom mentioned this pull request Apr 10, 2025
Open
daipom added a commit to daipom/fluentd that referenced this pull request Apr 10, 2025
@daipom
http_server: stop fallback to WEBrick
ed56dfe 
WEBrick is no longer recommended for production use.

We don't need this feature because it was for supporting Ruby <
2.3. (fluent#2447)

Signed-off-by: Daijiro Fukuda <[email protected]>
@daipom daipom mentioned this pull request Apr 10, 2025
Merged
daipom added a commit that referenced this pull request Apr 10, 2025
@daipom
http_server: stop fallback to WEBrick (#4899)
83c19d1 
**Which issue(s) this PR fixes**: 
* Partially Fixes #4648

**What this PR does / why we need it**: 
Make `http_server` plugin helper stop the fallback to WEBrick.

WEBrick is no longer recommended for production use.

We don't need this feature because it was for supporting Ruby < 2.3.
(#2447)

**Docs Changes**:

https://docs.fluentd.org/plugin-helper-overview/api-plugin-helper-http_server
=> fluent/fluentd-docs-gitbook#575

**Release Note**: 
The same as the title.

Signed-off-by: Daijiro Fukuda <[email protected]>
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