Skip to content
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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kgeckhart
Copy link
Contributor

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

Comment on lines +245 to +267
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)
Copy link
Contributor Author

@kgeckhart kgeckhart Feb 4, 2023

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.

Comment on lines +347 to +375
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
}
Copy link
Contributor Author

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.

@SuperQ
Copy link
Contributor

SuperQ commented Oct 25, 2023

Still working on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Google API library
2 participants