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

Enable serving /_status and /_metrics via HTTP #295

Merged
merged 2 commits into from
Mar 6, 2020

Conversation

Wuvist
Copy link
Contributor

@Wuvist Wuvist commented Mar 4, 2020

The pull request is to address the issue #294

By making parameter --status handle value like http://127.0.0.1:8888, /_status and /_metrics could serve via HTTP.

@CLAassistant
Copy link

CLAassistant commented Mar 4, 2020

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 89.395% when pulling c00c9c2 on Wuvist:serve_http into 96a8839 on square:master.

main.go Outdated
@@ -615,6 +615,12 @@ func (context *Context) serveStatus() error {
mux.Handle("/debug/pprof/trace", http.HandlerFunc(pprof.Trace))
}

serveHTTPS := true
if strings.HasPrefix(*statusAddress, "http://") {
*statusAddress = (*statusAddress)[7:]
Copy link
Member

@csstaub csstaub Mar 4, 2020

Choose a reason for hiding this comment

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

(1) Would prefer that we avoid changing the global flag value here, as it may lead to odd side-effects if the value is ever used somewhere else. Let's introduce a local var for the status address.
(2) This code should also handle the case where the prefix might be https:// instead of http://, and behave accordingly. Ideally create a function that parses the status address and returns (https bool, network, address string, err error) based on parsing the flag value with net/url and socket.ParseAddress.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@csstaub Thanks for the comment.

Pushed a new commit, but instead of a function return (https bool, network, address string, err error), I created socket.ParseHTTPAddress function that return (https bool, address string)

the return address could be further parsed by existing socket.ParseAddress.

IMHO, this would avoid more code changes.

@csstaub
Copy link
Member

csstaub commented Mar 4, 2020

Thanks for the contribution @Wuvist! This seems reasonable as a feature, but I left some comments to improve the implementation.

socket/net.go Outdated
@@ -47,6 +47,12 @@ func ParseAddress(input string) (network, address, host string, err error) {
return
}

if strings.HasPrefix(input, "http:") {
network = "unix"
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why this block is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the typo, accidentally click ctrl +v and miss when committing.

amended commit.

@Wuvist
Copy link
Contributor Author

Wuvist commented Mar 6, 2020

rebased with newest master

@csstaub csstaub merged commit a7a9446 into ghostunnel:master Mar 6, 2020
@csstaub
Copy link
Member

csstaub commented Mar 6, 2020

Thanks @Wuvist!

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.

None yet

4 participants