POC tracking job metrics calling DB Api #102
Conversation
storey247
commented
Nov 1, 2019
•
storey247
commented
- Attempting to track extra metrics for prometheus
- POC so not ready for review yet
There was a problem hiding this comment.
This looks like a great start, awesome work.
Thinking out loud, may well be a bad plan, as we look to add more metrics to the other controllers I was maybe considering using something like the Azure Go SDK does with spans and defer to close out the call.
// NextWithContext advances to the next page of values. If there was an error making
// the request the page does not advance and the error is returned.
func (page *ResourceProviderOperationListPage) NextWithContext(ctx context.Context) (err error) {
if tracing.IsEnabled() {
ctx = tracing.StartSpan(ctx, fqdn+"/ResourceProviderOperationListPage.NextWithContext")
defer func() {
sc := -1
if page.Response().Response.Response != nil {
sc = page.Response().Response.Response.StatusCode
}
tracing.EndSpan(ctx, sc, err)
}()
}
next, err := page.fn(ctx, page.rpol)
if err != nil {
return err
}
page.rpol = next
return nil
}| } | ||
|
|
||
| func trackMetric(startTime time.Time, histogram prometheus.Histogram) { | ||
| endTime := float64(time.Now().Sub(startTime).Nanoseconds() / int64(time.Millisecond)) |
There was a problem hiding this comment.
nit: The sub method on time produces a duration which has the method Milliseconds. I'd favor using that over returning nanosec and dividing or I might have misunderstood what this is doing.
There was a problem hiding this comment.
Finally got to the bottom of this, yes there is a Milliseconds method, but only in golang v1.13 and this project currently targets 1.12 in its dev container. https://golang.org/doc/go1.13#time
There was a problem hiding this comment.
nit: In time.Now().Sub(startTime).Nanoseconds() I think you can replace it with time.Now().Sub(startTime).
nit: I don't think the int64 conversion is needed if you don't call Nanoseconds
nit: the variable is called endTime but looks more like duration?
Also, is the float conversion designed to allow sub-millisecond resolution in the metrics? If so, I'm not sure that the conversion on the outside will achieve that.
| endTime := float64(time.Now().Sub(startTime).Nanoseconds() / int64(time.Millisecond)) | |
| duration := float64(time.Now().Sub(startTime) / time.Millisecond) |
Co-Authored-By: Stuart Leeks <stuart@leeks.net>
|
Closing this PR as it is a POC and want to extend for more metrics |
