-
Notifications
You must be signed in to change notification settings - Fork 66
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 instrumentation to http client dial #107
Conversation
srivastavankit
commented
May 23, 2017
- first attempt at adding instrumentation only for http dial
BaseURL string | ||
} | ||
|
||
// NewHTTPClient will allocate a http client. | ||
func NewHTTPClient( | ||
gateway *Gateway, baseURL string, | ||
) *HTTPClient { | ||
scope := gateway.MetricScope.SubScope("") // TODO: get http client name | ||
timer := scope.Timer("dial") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: a more specific metric name like http-client.dial
might be nicer.
func getInstrumentedDial(timer tally.Timer, successCtr, errorCtr tally.Counter) func(string, string) (net.Conn, error) { | ||
return func(n, a string) (conn net.Conn, err error) { | ||
stat := instrument(timer, successCtr, errorCtr) | ||
defer func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: i find defers hard to read because of unwinding.
stat := instrument(timer, successCtr, errorCtr);
conn, err := net.Dial(n, a)
stat(err)
return conn, err
is easier to read.
|
||
// HTTPClient defines a http client. | ||
type HTTPClient struct { | ||
gateway *Gateway | ||
|
||
Client *http.Client | ||
Logger *zap.Logger | ||
Scope *tally.Scope | ||
BaseURL string | ||
} | ||
|
||
// NewHTTPClient will allocate a http client. | ||
func NewHTTPClient( | ||
gateway *Gateway, baseURL string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can add an extra argument here for httpClientName.
Then update the template ( https://github.com/uber/zanzibar/blob/master/codegen/templates/http_client.tmpl#L37 ), should be "{{.Name}}"
PR is stale. Will close for now. Feel free to re-open if still wanted. |