-
Notifications
You must be signed in to change notification settings - Fork 101
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
Use cloud.google.com/go/monitoring/apiv3 for metrics APIs #200
base: master
Are you sure you want to change the base?
Use cloud.google.com/go/monitoring/apiv3 for metrics APIs #200
Conversation
Signed-off-by: Kyle Eckhart <[email protected]>
var descriptors []*metric.MetricDescriptor | ||
for { | ||
// There's nothing exposed in https://pkg.go.dev/google.golang.org/api/[email protected] which lets you | ||
// know an API call was initiated. You might think https://pkg.go.dev/google.golang.org/api/[email protected]#NewPager | ||
// could do it but the pageSize is a superficial page size that the consumer sets and has impact on API call paging. | ||
// If we know there are no items left in the current page and there's a non-empty page token then calling Next() is going to initiate an API call | ||
if it.PageInfo().Remaining() == 0 && it.PageInfo().Token != "" { | ||
apiCalls += 1.0 | ||
} | ||
descriptor, err := it.Next() | ||
if err == iterator.Done { | ||
break | ||
} | ||
if err != nil { | ||
errChannel <- fmt.Errorf("error while fetching descriptors for %s, %v", metricsTypePrefix, err) | ||
break | ||
} | ||
descriptors = append(descriptors, descriptor) | ||
} | ||
c.apiCallsTotalMetric.Add(apiCalls) | ||
|
||
if err := c.collectMetricsForDescriptors(ch, begun, descriptors); err != nil { | ||
errChannel <- fmt.Errorf("error while fetching descriptors for %s, %v", metricsTypePrefix, err) |
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.
This is a behavioral difference as the previous implementation submitted each page to collect metrics. There's no documented default page size on the MetricDescriptor API but I don't think this ever resulted in multiple pages.
I really wish there was an easier way to just get the results from a page vs needing to iterate. This implementation is going to cause more memory overhead as it reconstructs the slice which is just buried in the iterator.
The Pager
abstraction provided (https://pkg.go.dev/google.golang.org/api/[email protected]#NewPager) could help with the memory overhead but would further obfuscate the number of API calls.
it := c.metricClient.ListTimeSeries(ctx, request) | ||
|
||
var results []*monitoringpb.TimeSeries | ||
apiCalls := 1.0 | ||
for { | ||
// There's nothing exposed in https://pkg.go.dev/google.golang.org/api/[email protected] which lets you | ||
// know an API call was initiated. You might think https://pkg.go.dev/google.golang.org/api/[email protected]#NewPager | ||
// could do it but the pageSize is a superficial page size that the consumer sets and has impact on API call paging. | ||
// If we know there are no items left in the current page and there's a non-empty page token then calling Next() is going to initiate an API call | ||
if it.PageInfo().Remaining() == 0 && it.PageInfo().Token != "" { | ||
apiCalls += 1.0 | ||
} | ||
timeSeries, err := it.Next() | ||
if err == iterator.Done { | ||
break | ||
} | ||
if err != nil { | ||
level.Error(c.logger).Log("msg", "error retrieving Time Series metrics for descriptor", "descriptor", metricDescriptor.Type, "err", err) | ||
errChannel <- err | ||
break | ||
} | ||
results = append(results, timeSeries) | ||
} | ||
c.apiCallsTotalMetric.Add(apiCalls) | ||
|
||
if err := c.reportTimeSeriesMetrics(results, metricDescriptor, ch, begun); err != nil { | ||
level.Error(c.logger).Log("msg", "error reporting Time Series metrics for descriptor", "descriptor", metricDescriptor.Type, "err", err) | ||
errChannel <- err | ||
} |
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.
Similar behavior difference with iterating to collect all vs collecting by page. This one is more frustrating because the page limiter isn't the TimeSeries it's the amount of points returned which are embedded in the timeseries. So, even if using the Pager it's going to try to page by TimeSeries so setting the page size in the Pager implementation doesn't translate to the way the API pages.
Still working on this? |
Move to using
cloud.google.com/go/monitoring
for metrics APIs. This causes some breaking changes in CLI params and behavior as the new client uses grpc and does some awkward things with iterators. The behavioral differences are called out as comments.Opening as draft for the moment as I need to do more longer term testing on scrape time + resource usage.
Resolves #75