segmentio / stats

Go package for abstracting stats collection
https://godoc.org/github.com/segmentio/stats
MIT License
208 stars 32 forks source link

Add support for per request tags #100

Closed maxcnunes closed 5 years ago

maxcnunes commented 5 years ago

As reported on https://github.com/segmentio/stats/issues/93 and documented in Go api it seems to be a bad practice using a new client/transport instance on every request.

https://golang.org/pkg/net/http/#Client

The Client's Transport typically has internal state (cached TCP connections), so Clients should be reused instead of created as needed. Clients are safe for concurrent use by multiple goroutines.

and

Clients and Transports are safe for concurrent use by multiple goroutines and for efficiency should only be created once and re-used.

So, a solution that came to my mind is passing the tags by Context and creating the stats engine with thos provided tags during the round trip.

I would like to get a feedback if segmentio team is interested on this changes before adding the proper documentation and tests for this change.

maxcnunes commented 5 years ago

cc @achille-roussel @rjenkins

rjenkins commented 5 years ago

@maxcnunes thank you for this. @achille-roussel I'm wondering if it would be cleaner to extend the API to allow the user to provide a func that will get called back with the necessary data they might need to compute the tags? However, this might expose some internals.

rjenkins commented 5 years ago

@maxcnunes FYI I talked to @achille-roussel offline and he said he'll be commenting on the approach shortly.

maxcnunes commented 5 years ago

hey @achille-roussel any news on this? I was thinking, another approach would be to extract that logic in the RoadTrip into something like a span which we call before running the request and providing the request instance as an argument and after the response had been executed and then providing the response instance to it. It would require more steps to use it, but at least it would provide an API to report an outgoing request metric. Something like this:

span := httpstats.StartOutgoingRequestSpan(req)
res, err := client.Do(req)
span.Finish(res) // span.FinishWithTags(res, customTags)

Also, I'm worried what is the memory cost for creating a new engine withWithTags for every request just so I can add tags such as endpoint, request_source, direction:outgoing. Wouldn't be more eficient if we made requestBody and responseBody public and they had meta interface{} field so we could add custom tags to it using the field tags?

maxcnunes commented 5 years ago

Btw, I willing to help implementing any solution :)

adamdicarlo commented 5 years ago

@achille-roussel have you had a chance to give this some thought?

mnichols commented 5 years ago

Hi I reissued this PR from the fork we used at InVisionApp's repo: https://github.com/segmentio/stats/pull/104. It was updated with these changes.

abraithwaite commented 5 years ago

Did #104 work for you over at InvisionApp @mnichols @maxcnunes ? Can we close this?

abraithwaite commented 5 years ago

Going to close this for now. I think both #104 and #111 together should satisfy this use case. Thanks!