[ANN] httpsnoop, an easy way to capture http related metrics (response time, bytes written, and http status code)

[Felix Geisendoerfer]

Hi all,

while working on instrumenting my application, I ran into the fact that capturing metrics such as status codes from my own http.Handlers is surprisingly difficult to get right.

Therefor I created a package that hopefully avoids most of the common pitfalls. 

https://github.com/felixge/httpsnoop

I would love for net/http experts to take a look at the "Why this package exists" section of the README, as well as the horrible hack required to make things work:

https://github.com/felixge/httpsnoop/blob/master/wrap.go#L44-L163

Please let me know if you have suggestions for simpler approaches and/or spot any bugs in my implementation.

Thanks a lot!

Felix Geisendörfer

[Ian Davis]

On Thu, Nov 10, 2016, at 11:09 AM, Felix Geisendoerfer wrote:

I would love for net/http experts to take a look at the "Why this package exists" section of the README, as well as the horrible hack required to make things work:

https://github.com/felixge/httpsnoop/blob/master/wrap.go#L44-L163

Please let me know if you have suggestions for simpler approaches and/or spot any bugs in my implementation.

Did you think about performing a type assertion in the rw struct methods like this:

func (w *rw) Flush() {

    if f, ok := w.w.(http.Flusher); ok {

        f.Flush()

    }

}

I haven't tried it myself, but it's what came to mind when reading your Wrap "abomination" (as you call it in the code).

[Felix Geisendörfer]

Yes, I thought about it :).

Did you read the "Why this package exists” section of the README?

The problem with this approach is that an application’s behavior might change merely because the method is present. E.g. imagine an application doing this:

```

_, ok := w.(http.Flusher)

if ok {

  w.Write([]byte(“I don’t like talking to flushers!"))

  return

}

```

Perhaps I’m paranoid, but IMO introducing instrumentation into the most critical path of a large application is something one should not take too lightly. It’s really easy to break some obscure handler somewhere without realizing it.

Cheers

Felix

--
You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-nuts/-I5IZgJosYE/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-nuts...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Ian Davis]

On Thu, Nov 10, 2016, at 02:21 PM, Felix Geisendörfer wrote:

Yes, I thought about it :).

Did you read the "Why this package exists” section of the README?

Yes but obviously not closely enough :)

Kubernetes takes a hybrid approach:

https://github.com/kubernetes/kubernetes/blob/master/pkg/apiserver/metrics/metrics.go#L84

[Felix Geisendörfer]

There are a few problems with the Kubernetes implementation.

The first thing I noticed is that they seem to have copied their code from the prometheus go client:

https://github.com/prometheus/client_golang/blob/master/prometheus/http.go#L289

I don’t know if there was a copyright reassignment, but if not, it looks like they failed to honor the prometheus license.

The next problem is that they don’t seem to handle the io.ReaderFrom interface. Looking at the commit history of the prometheus code, this is somewhat confusing, because they don’t seem to have had a version where this was missing. So the Kubernetes developer may have intentionally removed it. It’d be curious why.

Last but not least, both the Kubernetes as well as the prometheus implementation only address 2 out of the 16 combinations of http.Flusher, http.CloseNotifier, http.Hijacker and io.ReaderFrom. Perhaps they determined that the go core is only using these 2 combinations. But that’s a bit too brittle of an invariant for me to rely on. Therefor my package does the painful thing and implements all 16 cases …

Cheers

Felix

On 10 Nov 2016, at 15:25, Ian Davis <m...@iandavis.com> wrote:

[Ian Davis]

Out of interest, do you have any examples of packages that perform a type check to reject an operation with a response writer? Your implementation is complex purely to address this scenario.

I would think that 99+% of all type checks are to determine whether the supplied object provides additional functionality that can be used. I'm struggling to think of a situation where you would type check for the Flush method and then refuse to use the main ResponseWriter methods if it were a Flusher.

[Felix Geisendörfer]

On 10 Nov 2016, at 15:59, Ian Davis <m...@iandavis.com> wrote:

Out of interest, do you have any examples of packages that perform a type check to reject an operation with a response writer? Your implementation is complex purely to address this scenario.

No, that was a bogus example.

However, I do have an application that will misbehave if CloseNotify is not implemented or not implemented properly.

And I think it’s also easy to imagine that it’s downright impossible to fake http.Hijacker if the underlaying ResponseWriter doesn’t support it.

I would think that 99+% of all type checks are to determine whether the supplied object provides additional functionality that can be used. I'm struggling to think of a situation where you would type check for the Flush method and then refuse to use the main ResponseWriter methods if it were a Flusher.

I agree. But verifying if you’re part of the 99% or 1% may be a very complex task, especially when using 3rd party http related packages.

So why would I want to worry about this invariant holding for a large amount of source code, if I can fix the problem with a comparably much smaller amount of boilerplate?

Cheers

Felix

[Ian Davis]

On Thu, Nov 10, 2016, at 03:21 PM, Felix Geisendörfer wrote:

I would think that 99+% of all type checks are to determine whether the supplied object provides additional functionality that can be used. I'm struggling to think of a situation where you would type check for the Flush method and then refuse to use the main ResponseWriter methods if it were a Flusher.

I agree. But verifying if you’re part of the 99% or 1% may be a very complex task, especially when using 3rd party http related packages.

So why would I want to worry about this invariant holding for a large amount of source code, if I can fix the problem with a comparably much smaller amount of boilerplate?

You can't in the general case. You cover the interfaces that you currently know about but you don't know what other interfaces people are relying on. Sipporting those becomes a combinatorial problem of whack-a-mole.

[Tong Sun]

That's very interesting Felix. Can it handle https requests as well? 

[Felix Geisendörfer]

On 10 Nov 2016, at 16:44, Ian Davis <m...@iandavis.com> wrote:

You can't in the general case. You cover the interfaces that you currently know about but you don't know what other interfaces people are relying on. Sipporting those becomes a combinatorial problem of whack-a-mole.

I think Go 1.7 allows for the general case to be solved using reflect.StructOf.

But I haven’t tried it yet, because I can’t use 1.7 until my new MBP arrives next week :(.

In the meantime, supporting 16 / N cases still seems better than supporting 2 / N cases. And to my knowledge there exists no alternative to httpsnoop right now that isn’t tied to a particular logging/metrics library.

Cheers

Felix

[1] https://github.com/golang/go/issues/16726

[Felix Geisendörfer]

Hi Tong,

the README example shows logging request related information (method, url) in addition to the response related stuff.

Most of the request related information is easy to get directly from the request object, so I didn’t implement any special support for that yet.

That being said, I could imagine adding a `Read` metrics field for the number of bytes read from the request’s body.

What sort of metrics did you have in mind?

Cheers

Felix

[Felix Geisendörfer]

Tong, I may have misread your e-mail. I missed the “s” in http(s).

I don’t see anything that would be special about http requests, so they should just work.

Cheers

Felix

[Tong Sun]

Yep, that's what I meant. Thanks. 

To unsubscribe from this group and all its topics, send an email to golang-nuts+unsubscribe@googlegroups.com.

[Hirotaka Yamamoto (ymmt2005)]

Hi,

I had implemented and blogged the technique to properly wrap http.ResponseWriter:

http://ymmt2005.hatenablog.com/entry/2016/09/01/How_to_hack_Go%27s_http_Server_correctly

type StdResponseWriter interface {

http.ResponseWriter

io.ReaderFrom

http.Flusher

http.CloseNotifier

http.Hijacker

WriteString(data string) (int, error)

}

I think an interface like this should be provided in net/http package to

make it easy to capture the response.

2016年11月10日木曜日 20時09分32秒 UTC+9 Felix Geisendoerfer:

[Felix Geisendörfer]

I think an interface like this should be provided in net/http package to

make it easy to capture the response.

Yeah, I was hoping the new http tracing stuff would allow for this kind of instrumentation, but unfortunately it’s targeted at different aspects of the request/response cycle.

Cheers

Felix

--
You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-nuts/-I5IZgJosYE/unsubscribe.

To unsubscribe from this group and all its topics, send an email to golang-nuts...@googlegroups.com.

[atd...@gmail.com]

I haven't thought too much about it but there is possibly an alternative logic by having the wrappers add a Wrap() http.ResponseWriter that would return the wrappee.

So first, one would test for that Wrapper interface, then return the wrappee if the test turns out to be positive. Then one could assert for various interfaces on this wrappee.

Something to think about.

[Felix Geisendörfer]

I haven't thought too much about it but there is possibly an alternative logic by having the wrappers add a Wrap() http.ResponseWriter that would return the wrappee.

So first, one would test for that Wrapper interface, then return the wrappee if the test turns out to be positive. Then one could assert for various interfaces on this wrappee.

Something to think about.

Maybe I’m misunderstanding your idea, but wouldn’t this require having to go over every handler used by the application in order to see where this is needed?

If yes, then I don’t like it, because that’s exactly what I was trying to avoid. The whole idea was to make the wrapping/instrumentation transparent to all handlers.

Cheers

Felix

[atd...@gmail.com]

The goal is to check whether a http.ResponseWriter is a wrapper around another http.ResponseWriter. If so, we can check what the wrappee implements besides ServeHTTP(...). Maybe it is itself a wrapper. Maybe it is a Hijacker or ReadCloser...

If this wrappee is itself a wrapper, the logic is recursive.

This way, the top wrapper, which is a http.ResponseWriter, does not need to bear assumptions about what any of the wrappee may have implemented. It just asserts for what it is interested in.

https://play.golang.org/p/cBeJHExtOh

Now, I don't know if it solves your issue but that's a quick thought.

[Felix Geisendörfer]

On 11 Nov 2016, at 12:57, atd...@gmail.com <atd...@gmail.com> wrote:

Now, I don't know if it solves your issue but that's a quick thought.

No, as explained in my previous e-mail, this doesn’t fit my requirements.

Anyway, thanks for your input. :)