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

Add new HTTPServer to serve /metrics on port 8082 #1085

Merged
merged 5 commits into from Mar 9, 2019

Conversation

Projects
None yet
4 participants
@viveksyngh
Copy link
Member

viveksyngh commented Feb 13, 2019

This is WIP PR to a new HTTPServer on port 8082 in a goroutine to serve
/metrics endpoint on a different port.

This also update the configurations and compose files.

Fixes: #1081

Signed-off-by: Vivek Singh vivekkmr45@yahoo.in

Description

Motivation and Context

How Has This Been Tested?

Tested on local swarm cluster:
screen shot 2019-02-15 at 7 37 13 pm

screen shot 2019-02-15 at 7 37 30 pm

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.
Add new HTTPServer to serve /metrics on port 8082
This commit runs a new HTTPServer on port 8082 in a goroutine to serve
/metrics endpoint on a different port.

This also update the configurations and compose files.

Fixes: #1081

Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>

@viveksyngh viveksyngh changed the title Add new HTTPServer to serve /metrics on port 8082 [WIP] Add new HTTPServer to serve /metrics on port 8082 Feb 13, 2019

WriteTimeout: config.WriteTimeout,
MaxHeaderBytes: http.DefaultMaxHeaderBytes,
Handler: routes,
}

This comment has been minimized.

@LucasRoesler

LucasRoesler Feb 14, 2019

Member

How do you plan on stopping this goroutine when the main thread stops? The server should be defined outside of the goroutine, there should be some kind of signal catch, and then sending s.Shutdown() to stop this gracefully.

This comment has been minimized.

@viveksyngh

viveksyngh Feb 14, 2019

Author Member

I think when main thread stops it will also stop the go routines launched from main thread, it may not do the cleanup on the resources used. I will implement the server to shutdown gracefully.

This comment has been minimized.

@alexellis

alexellis Feb 15, 2019

Member

I think we could set a shorter, static timeout value for the HTTP server given that this is not serving function requests.

This comment has been minimized.

@viveksyngh

viveksyngh Feb 15, 2019

Author Member

I have updated the server to use 30s for readTimeout and writeTimeout

This comment has been minimized.

@alexellis

alexellis Feb 20, 2019

Member

30s seems too high for this purpose, let's reduce it to 5s?

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Feb 15, 2019

Graceful shutdown should first be implemented separately for the main server - raise an issue if you want? I'm OK for this metrics endpoint to be shutdown from SIGTERM in the initial version.

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Feb 15, 2019

Do we need a /healthz on the new port, too?

@@ -207,7 +207,22 @@ func main() {
}

metricsHandler := metrics.PrometheusHandler()
r.Handle("/metrics", metricsHandler)

//Create a new server to serve /metrics endpoint

This comment has been minimized.

@alexellis

alexellis Feb 15, 2019

Member
	// Listen on a separate HTTP port for Prometheus metrics to keep this accessible from 
        // the internal network only. 

This comment has been minimized.

@viveksyngh

viveksyngh Feb 15, 2019

Author Member

Done.

r.Handle("/metrics", metricsHandler)

//Create a new server to serve /metrics endpoint
go func() {

This comment has been minimized.

@alexellis

alexellis Feb 15, 2019

Member

Let's do this in a separate method at least?

This comment has been minimized.

@viveksyngh

viveksyngh Feb 15, 2019

Author Member

I have put this in a separate method and added /healthz endpoint.

@@ -3,6 +3,7 @@ services:
gateway:
ports:
- 8080:8080
- 8082:8082

This comment has been minimized.

@alexellis

alexellis Feb 15, 2019

Member

Does this work if you remove the port? It should still be accessible from within the cluster. We don't need to expose it generally.

This comment has been minimized.

@viveksyngh

viveksyngh Feb 15, 2019

Author Member

Yes, It does and I have removed 8082 from exposed port.

@@ -32,7 +32,7 @@ scrape_configs:
scrape_interval: 5s
dns_sd_configs:
- names: ['tasks.gateway']
port: 8080
port: 8082

This comment has been minimized.

@alexellis

alexellis Feb 15, 2019

Member

From an operational standpoint I can't merge this PR until we do a new release of the gateway, this needs to be two separate PRs.

This comment has been minimized.

@viveksyngh

viveksyngh Feb 15, 2019

Author Member

Okay .. so these prometheus configs should be put in a different PR and one PR to add the gateway changes ?

This comment has been minimized.

@alexellis
Run metrics server in a separate method
This commit adds changes to run metrics server in a separate method and
also removed port 8082 from exposed port a/c to review comments.

It also uses a smaller static timeout value for new server.

Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>

@viveksyngh viveksyngh force-pushed the viveksyngh:issue-1081 branch from 4219ced to 25be25f Feb 15, 2019

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Feb 15, 2019

Do we need 30s for the metrics endpoint? It seems excessive

@viveksyngh

This comment has been minimized.

Copy link
Member Author

viveksyngh commented Feb 15, 2019

how about 5s ?

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Feb 15, 2019

That sounds more like what I had in mind

Reduce timeout value metrics server to 5 second
Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>

@viveksyngh viveksyngh changed the title [WIP] Add new HTTPServer to serve /metrics on port 8082 Add new HTTPServer to serve /metrics on port 8082 Feb 17, 2019

log.Fatal(s.ListenAndServe())
}

func healthzHandler(w http.ResponseWriter, r *http.Request) {

This comment has been minimized.

@alexellis

alexellis Feb 20, 2019

Member

Do we already have a healthz handler for the gateway itself? Perhaps this handler should be moved to the handlers package or shared?

This comment has been minimized.

@viveksyngh

viveksyngh Feb 21, 2019

Author Member

gateway using reverse proxy handler for healthzHandler as well. Shouldn't this have different healthz handler?

router.HandleFunc("/healthz", healthzHandler)

port := 8082
readTimeout := time.Duration(5) * time.Second

This comment has been minimized.

@alexellis

alexellis Feb 20, 2019

Member

Does 5 * time.Second not work?

This comment has been minimized.

@viveksyngh

viveksyngh Feb 21, 2019

Author Member

That will also work.

Move /healthz handler to handlers package
Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>
@burtonr

This comment has been minimized.

Copy link
Member

burtonr commented Mar 9, 2019

@viveksyngh This PR looks to be very nearly complete. The only thing preventing it from merging (from what I can tell) is removing the change to the prometheus/*.yml files.

Remove prometheus changes
Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>
@viveksyngh

This comment has been minimized.

Copy link
Member Author

viveksyngh commented Mar 9, 2019

@burtonr I have removed prometheus changes. Will raise a different PR for that.

@alexellis
Copy link
Member

alexellis left a comment

LGTM

@alexellis alexellis merged commit ebc4193 into openfaas:master Mar 9, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.