-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
adds instrumentation to azure object client #5022
Conversation
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.
LGTM 🙂
Just some small comments
// Latency seems to range from a few ms to a few secs and is | ||
// important. So use 6 buckets from 5ms to 5s. |
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.
// Latency seems to range from a few ms to a few secs and is | |
// important. So use 6 buckets from 5ms to 5s. | |
// Latency seems to range from a few ms to a few secs and is | |
// important. So use 6 buckets from 5ms to 5s. |
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.
LGTM 👍
}, []string{"operation", "status_code"})) | ||
|
||
func init() { | ||
requestDuration.Register() |
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.
Nit: we're trying to avoid global registration across the board. No need to remove it, just a comment.
I don't specifically like this instrument package.
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.
Yeah, I largely copied this from our s3 client :(
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.
LGTM.
I think it's hard to make this generics for all clients because you need to do it on the transport level, at which point you don't really know what is a delete/get/list and trying to find this with http information might be a pain.
We could though allow to inject and abstract those operationName via the context. Assuming the context is well propagated.
We could do this:
ctx := metrics.WithOperationName(ctx,"azure.GetObject")
rc, err = b.getObject(ctx, objectKey)
return err
Then the transport middleware could get that out, figure automatically the latency and the status code from the http response.
GCS uses this technique but map only Method to OperationName which could be enough if all those API are correctly resty ? But then you don't know across multiple cluster which cloud provider it is See https://github.com/grafana/loki/blob/main/pkg/storage/chunk/gcp/instrumentation.go#L83
type instrumentedTransport struct {
observer prometheus.ObserverVec
next http.RoundTripper
}
func (i instrumentedTransport) RoundTrip(req *http.Request) (*http.Response, error) {
start := time.Now()
resp, err := i.next.RoundTrip(req)
if err == nil {
i.observer.WithLabelValues(req.Method, strconv.Itoa(resp.StatusCode)).Observe(time.Since(start).Seconds())
}
return resp, err
}
I'm rooting for adding all the above using a new transport package in chunk package.
package transport
// NewTransport creates a new Transport for the given cloud provider name.
func NewTransport(providerName string) *http.Transport {}
nice catch! Co-authored-by: Susana Ferreira <ssncferreira@gmail.com>
Eventually, I'd like to make a higher order utility for instrumented object clients, but the plumbing seemed a bit tedious. Instead, I've just added azure specific instrumentation here.