Expose prometheus metrics at /metrics endpoint

[lebaptiste]

The chi.Router is not used and I believe it is not intentional.
I have added tests to cover the expected behaviour. go fmt has added a small diff in pubsub.go.

Also out of curiosity, is there any benefit to do

	wait := make(chan struct{})
	go func() {
		<-wait
		server.Close()
	}()

	return func() { close(wait) }

instead of directly?

	return func() { server.Close() }

Thanks

[maclav3]

Hi @lebaptiste, thanks a lot for contributing!

First of all, yes, the omission of router in http.Server was a mistake.
However, when not using a router, http://localhost:8081/whatever actually serves the metrics, so that's why we didn't notice it earlier.
I think nothing else is exposed at this port, so there was no occasion for conflicts.

Also, thanks to your PR, I discovered some invalid paths in the metrics example that we overlooked #169

Regarding the use of the gochannel, you're right, probably return func() { server.Close() } would be sufficient here.
The gochannel solution is most probably a remnant of some larger prototype solution that was simplified since :D

So, regarding this PR:
By merging this, we restrict the current functionality, by causing :8081/all-routes-apart-from-metrics to cease to return metrics.
While this may cause trouble for someone, it was never an intended functionality, so I think we may merge this and from now on, only serve the metrics at :8081/metrics?
@roblaszczak

[lebaptiste]

Also the test_race and test_short are not passing, but I ran them locally and did not have any issue. I suppose the port 8080 is already used in you docker compose. Picking an other port is simple enough but happy to discuss if there is a preferable way for the tests. Thanks.

[sagikazarmark]

This will panic for a nil err too.

[lebaptiste]

server.ListenAndServe is guaranteeing to return non-nil error. Checking for nil might be more explicit though.

[sagikazarmark]

Ah, indeed, I only checked the error handling, but not what it actually handles.

[maclav3]

@lebaptiste probably return func() { server.Close() } could be simplified even to return server.Close, since they share the same signature.

About the test still failing, even with port 8090: maybe the two tests you wrote are competing for the same port? You could try using 8091 for example in the second one, the logic should not be dependent on the port number.

This could happen if the tests are being run in parallel, or the port is not released properly after server.Close() is called maybe?

[bkielbasa]

I'd suggest not using the default client because he has no timeouts set https://golang.org/src/net/http/client.go line 110

[lebaptiste]

Thanks for the feedback, it was a while and I forgot a bit about this PR.
For the default http client, I agree with you and I would not use it for production code. However since we are in tests, I was considering leaving the default test timeout handling this instead, I found it simpler. What do you think?

[bkielbasa]

ok, tests have their timeouts so it can stay as it is

[lebaptiste]

Thanks for the feedbacks, I got some issues with the port setup and using different ports as suggested has fixed it 👍
It seems I am out of luck with GolangCI step though. Any idea?

[bkielbasa]

I think that this message is the most important

allowed: module contains a go.mod file, so semantic import versioning is required

[bkielbasa]

+1 for checking the resp before the error :)

[lebaptiste]

In fact I did this by habit, however if both err and response are returned, you don't have to close the body.
https://stackoverflow.com/questions/32818472/do-we-need-to-close-the-response-object-if-an-error-occurs-while-calling-http-ge

[roblaszczak]

I have no idea why this PR was stuck for such a long time 🤔 Thanks for the contribution!