Add disable_http_keep_alive option #1035

appare45 · 2024-11-29T06:06:12Z

Added disable_http_keep_alive option
The default value for disable_http_keep_alive is false
When disable_http_keep_alive is true, mkr requests mackerel without KeepAlive

appare45 · 2024-12-11T05:52:14Z

config/config.go

+	Verbose          bool
+	Silent           bool
+	Diagnostic       bool          `toml:"diagnostic"`
+	DisableKeepAlive bool          `toml:"disable_keep_alive"`


Added DisableKeepAlive here

Changed it to DisableHttpKeepAlive

mackerel-agent/config/config.go

Line 105 in 12e1b6e

DisableHttpKeepAlive bool `toml:"disable_http_keep_alive"`

appare45 · 2024-12-11T05:54:04Z

mackerel/api.go

+	t := http.DefaultTransport.(*http.Transport).Clone()
+	print(disableKeepAlive)
+	t.DisableKeepAlives = disableKeepAlive
+	c.HTTPClient.Transport = t


Since http.DefaultTransport is made from http.Transport, we can cast it here.
https://pkg.go.dev/net/http#:~:text=DefaultTransport%20is%20the,lowercase%20versions%20thereof

rmatsuoka · 2024-12-11T08:44:54Z

mackerel/api.go

 	c, err := mkr.NewClientWithOptions(apiKey, rawurl, verbose)
 	if err != nil {
 		return nil, err
 	}
 	c.PrioritizedLogger = logger
+	t := http.DefaultTransport.(*http.Transport).Clone()
+	print(disableKeepAlive)


You forgot to delete your print debugging.

rmatsuoka · 2024-12-11T09:02:03Z

Could you marge (or rebase) master branch onto your appare45:feat-disable-keep-alive? CI failure may be fixed.

appare45 · 2024-12-13T02:24:12Z

Could you marge (or rebase) master branch onto your appare45:feat-disable-keep-alive? CI failure may be fixed.

@rmatsuoka Thank you, I've rebased it and passed the CI

rmatsuoka

good! I added a note, but you can make changes or not.

rmatsuoka · 2024-12-18T02:18:36Z

command/command_api_test.go

[may]
In all the tests that call NewMackerelClient, you set the disableKeepAlive parameter to true. However, in the previous version, this parameter was implicitly set to false.
While I believe this change does not introduce any issues, it is preferable not to introduce such changes unnecessarily.

I changed it 7022f7c

rmatsuoka · 2024-12-18T02:44:50Z

Have you run the modified mackerel-agent and confirmed whether it sends metrics to Mackerel when disableKeepAlive is set to both true and false? If not, please test it.

appare45 · 2024-12-20T02:42:15Z

Have you run the modified mackerel-agent and confirmed whether it sends metrics to Mackerel when disableKeepAlive is set to both true and false? If not, please test it.

I've captured the packets between mackerel api and mackerel agent with mitmproxy.

Without `disable_http_keep_alive` (Default behaviour)

Since there is no connection header, HTTP/1.1 (mackerel api) reuses the connections ¹.

With `disable_http_keep_alive`

Since Connection header is set to close, the connection won't be reused.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Connection_management_in_HTTP_1.x#short-lived_connections:~:text=This%20model%20is%20the%20default%20model%20used%20in%20HTTP/1.0%20(if%20there%20is%20no%20Connection%20header%2C%20or%20if%20its%20value%20is%20set%20to%20close).%20In%20HTTP/1.1%2C%20this%20model%20is%20only%20used%20when%20the%20Connection%20header%20is%20sent%20with%20a%20value%20of%20close. ↩

rmatsuoka · 2024-12-23T02:45:06Z

command/command.go

@@ -693,8 +693,8 @@ func buildUA(ver, rev string) string {
 }

 // NewMackerelClient returns Mackerel API client for mackerel-agent
-func NewMackerelClient(apibase, apikey, ver, rev string, verbose bool) (*mackerel.API, error) {
-	api, err := mackerel.NewAPI(apibase, apikey, verbose)
+func NewMackerelClient(apibase, apikey, ver, rev string, verbose bool, disableHttpKeepAlive bool) (*mackerel.API, error) {


According to Go conventions, all letters in acronyms should be uppercase. Therefore, it is preferable to use disableHTTPKeepAlive across all your changes.

(ref. https://go.dev/wiki/CodeReviewComments#initialisms)

I changed it 4474546

rmatsuoka · 2024-12-23T02:47:26Z

config/config.go

+	if !config.DisableHttpKeepAlive {
+		config.DisableHttpKeepAlive = DefaultConfig.DisableHttpKeepAlive
+	}


The line is redundant: if config.DisableHTTPKeepAlive is already false, there is no need to set it to false (from DefaultConfig.DisableHTTPKeepAlive) again.

I changed it 78589bd

mackerelio#1035 (comment)

rmatsuoka

👍

appare45 force-pushed the feat-disable-keep-alive branch from 495b925 to 5c8c0d9 Compare December 11, 2024 05:49

appare45 commented Dec 11, 2024

View reviewed changes

appare45 marked this pull request as ready for review December 11, 2024 05:57

appare45 changed the title ~~Add keep_alive option~~ Add disable_keep_alive option Dec 11, 2024

rmatsuoka reviewed Dec 11, 2024

View reviewed changes

appare45 added 3 commits December 13, 2024 11:12

add keep_alive option for mackerel api client

935ed7a

change keep_alive option to disable_keep_alive

11d9b02

Changed disable_keep_alive options for the tests

c127806

appare45 force-pushed the feat-disable-keep-alive branch from d0b2804 to c127806 Compare December 13, 2024 02:12

Remove pring debugging

fb780e6

rmatsuoka approved these changes Dec 18, 2024

View reviewed changes

Changed Mackerel api client tests' disable_keep_alive

7022f7c

appare45 changed the title ~~Add disable_keep_alive option~~ Add disable_http_keep_alive option Dec 20, 2024

Changed disable_keep_alive to disable_http_keep_alive

05dcf6b

appare45 force-pushed the feat-disable-keep-alive branch from 12e1b6e to 05dcf6b Compare December 20, 2024 02:44

rmatsuoka reviewed Dec 23, 2024

View reviewed changes

appare45 added 2 commits December 26, 2024 10:52

Renamed Http to HTTP

4474546

mackerelio#1035 (comment)

Removed unused code

78589bd

rmatsuoka approved these changes Dec 26, 2024

View reviewed changes

rmatsuoka merged commit 6e910c9 into mackerelio:master Dec 26, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add disable_http_keep_alive option #1035

Add disable_http_keep_alive option #1035

appare45 commented Nov 29, 2024 •

edited

Loading

appare45 Dec 11, 2024

appare45 Dec 20, 2024

appare45 Dec 11, 2024

rmatsuoka Dec 11, 2024 •

edited

Loading

rmatsuoka commented Dec 11, 2024 •

edited

Loading

appare45 commented Dec 13, 2024

rmatsuoka left a comment

rmatsuoka Dec 18, 2024 •

edited

Loading

appare45 Dec 20, 2024

rmatsuoka commented Dec 18, 2024 •

edited

Loading

appare45 commented Dec 20, 2024

rmatsuoka Dec 23, 2024

appare45 Dec 26, 2024

rmatsuoka Dec 23, 2024 •

edited

Loading

appare45 Dec 26, 2024

rmatsuoka left a comment

Add disable_http_keep_alive option #1035

Add disable_http_keep_alive option #1035

Conversation

appare45 commented Nov 29, 2024 • edited Loading

appare45 Dec 11, 2024

Choose a reason for hiding this comment

appare45 Dec 20, 2024

Choose a reason for hiding this comment

appare45 Dec 11, 2024

Choose a reason for hiding this comment

rmatsuoka Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

rmatsuoka commented Dec 11, 2024 • edited Loading

appare45 commented Dec 13, 2024

rmatsuoka left a comment

Choose a reason for hiding this comment

rmatsuoka Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

appare45 Dec 20, 2024

Choose a reason for hiding this comment

rmatsuoka commented Dec 18, 2024 • edited Loading

appare45 commented Dec 20, 2024

Without disable_http_keep_alive (Default behaviour)

With disable_http_keep_alive

Footnotes

rmatsuoka Dec 23, 2024

Choose a reason for hiding this comment

appare45 Dec 26, 2024

Choose a reason for hiding this comment

rmatsuoka Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

appare45 Dec 26, 2024

Choose a reason for hiding this comment

rmatsuoka left a comment

Choose a reason for hiding this comment

appare45 commented Nov 29, 2024 •

edited

Loading

rmatsuoka Dec 11, 2024 •

edited

Loading

rmatsuoka commented Dec 11, 2024 •

edited

Loading

rmatsuoka Dec 18, 2024 •

edited

Loading

rmatsuoka commented Dec 18, 2024 •

edited

Loading

Without `disable_http_keep_alive` (Default behaviour)

With `disable_http_keep_alive`

rmatsuoka Dec 23, 2024 •

edited

Loading