Instrument RoundTripper via middleware #295

stuartnelson3 · 2017-04-25T20:24:13Z

Closing #288 as it was easier to open a new pull request than deal with the rebase/merge conflicts.

grobie

No real code review, I assume you discussed the implementation with @beorn7.

grobie · 2017-04-25T20:45:26Z

.travis.yml

@@ -2,8 +2,8 @@ sudo: false
 language: go

 go:
- - 1.6.3


We just had a discussion about supported versions in #291 and the decision was to test against 1.6 but to just build tags for 1.7+ features.

grobie · 2017-04-25T20:46:38Z

prometheus/promhttp/client.go

+// may choose to use separately buckets Histograms, or implement custom
+// instance labels on a per function basis.
+type InstrumentTrace struct {
+	GotConn, PutIdleConn, GotFirstResponseByte, Got100Continue, DNSStart, DNSDone, ConnectStart, ConnectDone, TLSHandshakeStart, TLSHandshakeDone, WroteHeaders, Wait100Continue, WroteRequest func(float64)


I'd prefer to list one struct member per line, even if that means repeating the value, than having such a long line.

Totally cool with me

This reverts commit 2c1b043.

grobie · 2017-04-26T13:42:39Z

I guess the whole rountripper becomes a go1.7+ feature, e.g. adding the build tag to both files you added here.

…

On Wed, Apr 26, 2017, 15:39 stuart nelson ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In .travis.yml <#295 (comment)> : > @@ -2,8 +2,8 @@ sudo: false language: go go: - - 1.6.3 How do we want to handle testing features that are only available in 1.7, like httptrace? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#295 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAANaO_066-XhJM7vKkWxWxbU1Fx9G1Tks5rz0kPgaJpZM4NIAka> .

httptrace is only available in go-1.7+

httptrace hook functions I'm referencing were only added in 1.8 it seems.

beorn7 · 2017-04-27T15:03:56Z

On my review queue. Will get to it once the conference permits.

beorn7

The filename client.go is inconsistent with the existing instrument_server.go. Please change either of it to create consistency (with the respective test files).

The package level doc comment needs to be updated as suggested earlier.

beorn7 · 2017-04-27T17:40:14Z

prometheus/promhttp/client.go

+	"time"
+
+	"github.com/prometheus/client_golang/prometheus"
+	dto "github.com/prometheus/client_model/go"


This line should be before the previous one and separated by a blank line (coming from a different repo).

beorn7 · 2017-04-27T17:45:44Z

prometheus/promhttp/client.go

+)
+
+// RoundTripperFunc is an adapter to allow wrapping an interface implementing
+// http.RoundTripper, allowing the user to construct layers of middleware.


The doc comment is the wrong way around. I'd just mirror the doc comment of the standard HandlerFunc, i.e. "The RoundTripperFunc type is an adapter to allow the use of ordinary functions as RoundTrippers. If f is a function with the appropriate signature, RountTriiperFunc(f) is a RoundTripper that calls f."

beorn7 · 2017-04-27T17:49:34Z

prometheus/promhttp/client.go

+		if err != nil {
+			return nil, err
+		}
+		gauge.Dec()


This needs to be defer'd. See InstrucmentHandlerInFlight for comparison.

beorn7 · 2017-04-27T17:54:24Z

prometheus/promhttp/client.go

+// in the CounterVec. For unpartitioned observations, use a CounterVec with
+// zero labels.
+//
+// If the wrapped RoundTripper panics, the Counter is not incremented.


...panics or returns a non-nil error, ...

beorn7 · 2017-04-27T17:56:36Z

prometheus/promhttp/client.go

+// names are "code" and "method". The function panics if any other instance
+// labels are provided. Partitioning of the CounterVec happens by HTTP status
+// code and/or HTTP method if the respective instance label names are present
+// in the CounterVec. For unpartitioned observations, use a CounterVec with


observations → counting

This actually applies to InstrumentHandlerCounter, which I missed in the previous review.

beorn7 · 2017-04-27T18:06:24Z

prometheus/promhttp/client.go

+// ObserverVec with zero labels. Note that partitioning of Histograms is
+// expensive and should be used judiciously.
+//
+// If the wrapped RoundTripper panics, no values are reported.


... or returns a non-nil error...

beorn7 · 2017-04-27T18:08:44Z

prometheus/promhttp/client.go

+	})
+}
+
+func checkEventLabel(c prometheus.Collector) {


This isn't used anywhere.

beorn7 · 2017-04-27T18:11:15Z

prometheus/promhttp/client_1_8.go

+// See the example for ExampleInstrumentRoundTripperDuration for example usage.
+func InstrumentRoundTripperTrace(it *InstrumentTrace, next http.RoundTripper) RoundTripperFunc {
+	return RoundTripperFunc(func(r *http.Request) (*http.Response, error) {
+		var (


I really wouldn't use a var block for a single initialization.

beorn7 · 2017-04-27T18:20:29Z

prometheus/promhttp/client_1_8_test.go

+
+	trace := &InstrumentTrace{
+		DNSStart: func(t float64) {
+			dnsLatencyVec.WithLabelValues("DNSStart")


In general, we prefer snake_case for names in Prometheus.

Applicable to label names below, too.

beorn7 · 2017-04-27T18:38:34Z

prometheus/promhttp/client_1_8.go

+// httptrace.ClientTrace hook functions. Each function is passed a float64
+// representing the time in seconds since the start of the http request. A user
+// may choose to use separately buckets Histograms, or implement custom
+// instance labels on a per function basis.


Perhaps document somehow that the various trace functions that get an error passed (PutIdleConn and others) don't trigger a call of the function given here if that error is non-nil.

stuartnelson3 · 2017-05-02T15:06:19Z

Hello from the other side ... let me know how it looks :)

beorn7 · 2017-05-02T22:26:33Z

I'm also on the other side. :-)

Will have a look ASAP.

beorn7

Just a minor concern about return values in case of errors.

beorn7 · 2017-05-03T15:55:17Z

prometheus/promhttp/instrument_client.go

+	return RoundTripperFunc(func(r *http.Request) (*http.Response, error) {
+		gauge.Inc()
+		defer gauge.Dec()
+		resp, err := next.RoundTrip(r)


I think it's better to just directly return here, i.e. return next.RoundTrip(r). Should RountTrip return a non-nil error together with a non-nil Response, we probably want to keep that behavior.

beorn7 · 2017-05-03T15:58:22Z

prometheus/promhttp/instrument_client.go

+	return RoundTripperFunc(func(r *http.Request) (*http.Response, error) {
+		resp, err := next.RoundTrip(r)
+		if err != nil {
+			return nil, err


Again, I think we want to return whatever RoundTrip returned in any case, i.e. return resp, err.

Which means you can just increment the counter in a if err == nil clause and have only one return line.

beorn7 · 2017-05-03T15:58:36Z

prometheus/promhttp/instrument_client.go

+		start := time.Now()
+		resp, err := next.RoundTrip(r)
+		if err != nil {
+			return nil, err


Same concern as above.

beorn7 · 2017-05-08T15:47:06Z

@stuartnelson3 Have you seen my review? It's only minor changes that are still needed. Should be easy to update.

(In different news, I'll have a look at improbable-eng/go-httpwares#12 and see if we can align with each other somehow. But that will be a separate thing from this PR.)

stuartnelson3 · 2017-05-08T16:57:15Z

Sorry for the delay, I missed your comments. Pushed!

beorn7 · 2017-05-09T16:47:48Z

Thanks. I'll close the related issues, but will also check out improbable-eng/go-httpwares#12 to see where we can align. And of course, I have to think about #299 .

stuartnelson3 added 4 commits April 24, 2017 21:34

Add client

78c294c

Fix client_test.go

29ba5d1

Update comments and examples

ec79472

Build for go 1.8.1, drop 1.6

2c1b043

stuartnelson3 requested a review from beorn7 April 25, 2017 20:24

stuartnelson3 mentioned this pull request Apr 25, 2017

Http client #288

Closed

grobie reviewed Apr 25, 2017

View reviewed changes

stuartnelson3 added 2 commits April 26, 2017 15:41

Re-format InstrumentTrace struct

0fba3c1

Revert "Build for go 1.8.1, drop 1.6"

890cefe

This reverts commit 2c1b043.

stuartnelson3 added 2 commits April 26, 2017 15:47

Separate code by build directive

e7cbef5

httptrace is only available in go-1.7+

Build against go-1.8

3b0e2d1

httptrace hook functions I'm referencing were only added in 1.8 it seems.

beorn7 requested changes Apr 27, 2017

View reviewed changes

stuartnelson3 added 3 commits May 2, 2017 10:30

Rename all client files to instrument_client

60ff76b

Update package doc comment

1544107

Address comments from beorn

51be061

beorn7 requested changes May 3, 2017

View reviewed changes

beorn7 mentioned this pull request May 4, 2017

Prometheus Monitoring Middleware/Tripperware improbable-eng/go-httpwares#12

Open

Always return response and err

63bb61a

beorn7 approved these changes May 9, 2017

View reviewed changes

beorn7 merged commit d300d5c into master May 9, 2017

beorn7 deleted the stn/http-client branch May 9, 2017 16:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instrument RoundTripper via middleware #295

Instrument RoundTripper via middleware #295

stuartnelson3 commented Apr 25, 2017

grobie left a comment

grobie Apr 25, 2017

grobie Apr 25, 2017

stuartnelson3 Apr 25, 2017

grobie commented Apr 26, 2017 via email

beorn7 commented Apr 27, 2017

beorn7 left a comment

beorn7 Apr 27, 2017

beorn7 Apr 27, 2017

beorn7 Apr 27, 2017

beorn7 Apr 27, 2017

beorn7 Apr 27, 2017

beorn7 Apr 27, 2017

beorn7 Apr 27, 2017

beorn7 Apr 27, 2017

beorn7 Apr 27, 2017

beorn7 Apr 27, 2017

stuartnelson3 commented May 2, 2017

beorn7 commented May 2, 2017

beorn7 left a comment

beorn7 May 3, 2017

beorn7 May 3, 2017

beorn7 May 3, 2017

beorn7 commented May 8, 2017

stuartnelson3 commented May 8, 2017

beorn7 commented May 9, 2017

Instrument RoundTripper via middleware #295

Instrument RoundTripper via middleware #295

Conversation

stuartnelson3 commented Apr 25, 2017

grobie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grobie commented Apr 26, 2017 via email

beorn7 commented Apr 27, 2017

beorn7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartnelson3 commented May 2, 2017

beorn7 commented May 2, 2017

beorn7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beorn7 commented May 8, 2017

stuartnelson3 commented May 8, 2017

beorn7 commented May 9, 2017