-
Notifications
You must be signed in to change notification settings - Fork 10
Conversation
cc: @grayside if you have any experience with Stackdriver RPCs, might be good to take a look. otherwise no worries. |
This adds a function to the Metrics interface and adds an implementation in stackdriver to obtain the number of requests for a given offset. Signed-off-by: Getulio Valentin Sánchez <valentin2507@gmail.com>
Signed-off-by: Getulio Valentin Sánchez <gvso@google.com>
Signed-off-by: Getulio Valentin Sánchez <gvso@google.com>
Signed-off-by: Getulio Valentin Sánchez <gvso@google.com>
Signed-off-by: Getulio Valentin Sánchez <gvso@google.com>
internal/stackdriver/stackdriver.go
Outdated
AggregationGroupByFields("resource.labels.service_name"). | ||
AggregationCrossSeriesReducer("REDUCE_SUM") | ||
|
||
logger.Debug("querying request count from Cloud Monitoring API") |
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.
you might find it useful to add values here.
imagine we query latency for a service 3 times.
but you see same log 3 times, you won't show what the each query was for.
but if you add some specifics (e.g. reducer, or aligner), you can have a better time distinguishing them.
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.
Since reducer and aligner are very "abstract" concepts about how Cloud Monitoring works (and are always the same), what about using intervalStartTime
and intervalEndTime
, which are more useful and change depending on when you call the function?
I'll add percentile in the case of latency, btw. But that will be in some other PR, keeping track at #24
Signed-off-by: Getulio Valentin Sánchez <gvso@google.com>
// aligner and cross series reducer. The result is in milliseconds. | ||
// | ||
// Error rate gets all the server responses. It calculates the error rate by | ||
// performing the operation (5xx responses / all responses). | ||
type Metrics interface { |
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.
From the standpoint of making the metrics in use flexible, I'm surprised to see Metrics
as an interface.
I expected to see:
var metrics []Metric
type Metric interface
type RequestCount Metric
Will leave this as a future consideration, on second glance it looks like this was a previous decision :)
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 imo this should be named MetricsProvider, as it is meant to be an interface for Stackdriver and Prometheus like apis
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.
Oh, I think the naming is what make it confusing. The interface is meant to facilitate the creation of different types of metrics providers (Stackdriver, Prometheus, etc). Might be worth renaming this interface. What do you think?
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.
Signed-off-by: Getulio Valentin Sánchez <gvso@google.com>
This adds a function to the Metrics interface and adds an implementation in stackdriver to obtain the number of requests for a given offset.