Clean up minor TODOs

This commit cleans up some TODOs where were done or no longer
applicable, and fixes a couple other minor TODOs, such as returning
proper errors.

pkg/custom-provider/metric_namer.go

@@ -84,7 +84,7 @@ func (r *basicSeriesRegistry) SetSeries(newSeries []prom.Series) error {
 		if strings.HasPrefix(series.Name, "container_") {
 			r.namer.processContainerSeries(series, newInfo)
 		} else if namespaceLabel, hasNamespaceLabel := series.Labels["namespace"]; hasNamespaceLabel && namespaceLabel != "" {
-			// TODO: handle metrics describing a namespace
+			// we also handle namespaced metrics here as part of the resource-association logic
 			if err := r.namer.processNamespacedSeries(series, newInfo); err != nil {
 				glog.Errorf("Unable to process namespaced series %q: %v", series.Name, err)
 				continue
@@ -123,7 +123,8 @@ func (r *basicSeriesRegistry) QueryForMetric(metricInfo provider.MetricInfo, nam
 	defer r.mu.RUnlock()
 
 	if len(resourceNames) == 0 {
-		// TODO: return error?  panic?
+		glog.Errorf("no resource names requested while producing a query for metric %s", metricInfo.String())
+		return 0, "", "", false
 	}
 
 	metricInfo, singularResource, err := r.namer.normalizeInfo(metricInfo)
@@ -214,7 +215,7 @@ type seriesSpec struct {
 // normalizeInfo takes in some metricInfo an "normalizes" it to ensure a common GroupResource form.
 func (r *metricNamer) normalizeInfo(metricInfo provider.MetricInfo) (provider.MetricInfo, string, error) {
 	// NB: we need to "normalize" the metricInfo's GroupResource so we have a consistent pluralization, etc
-	// TODO: move this to the boilerplate?
+	// TODO: move this to the boilerplate
 	normalizedGroupRes, err := r.mapper.ResourceFor(metricInfo.GroupResource.WithVersion(""))
 	if err != nil {
 		return provider.MetricInfo{}, "", err
@@ -247,7 +248,6 @@ func (n *metricNamer) processContainerSeries(series prom.Series, infos map[provi
 	}
 
 	info := provider.MetricInfo{
-		// TODO: is the plural correct?
 		GroupResource: schema.GroupResource{Resource: "pods"},
 		Namespaced:    true,
 		Metric:        name,
@@ -325,11 +325,9 @@ func (n *metricNamer) processRootScopedSeries(series prom.Series, infos map[prov
 // would return three GroupResources: "pods", "services", and "ingresses".
 // Returned MetricInfo is equilavent to the "normalized" info produced by normalizeInfo.
 func (n *metricNamer) groupResourcesFromSeries(series prom.Series) ([]schema.GroupResource, error) {
-	// TODO: do we need to cache this, or is ResourceFor good enough?
 	var res []schema.GroupResource
 	for label := range series.Labels {
 		// TODO: figure out a way to let people specify a fully-qualified name in label-form
-		// TODO: will this work when missing a group?
 		gvr, err := n.mapper.ResourceFor(schema.GroupVersionResource{Resource: string(label)})
 		if err != nil {
 			if apimeta.IsNoMatchError(err) {

pkg/custom-provider/provider.go

@@ -82,7 +82,7 @@ func NewPrometheusProvider(mapper apimeta.RESTMapper, kubeClient dynamic.ClientP
 
 		SeriesRegistry: &basicSeriesRegistry{
 			namer: metricNamer{
-				// TODO: populate this...
+				// TODO: populate the overrides list
 				overrides: nil,
 				mapper:    mapper,
 			},
@@ -133,8 +133,6 @@ func (p *prometheusProvider) metricsFor(valueSet pmodel.Vector, info provider.Me
 	}
 	res := []custom_metrics.MetricValue{}
 
-	// blech, EachListItem should pass an index --
-	// it's an implementation detail that it happens to be sequential
 	err := apimeta.EachListItem(list, func(item runtime.Object) error {
 		objUnstructured := item.(*unstructured.Unstructured)
 		objName := objUnstructured.GetName()
@@ -173,7 +171,7 @@ func (p *prometheusProvider) buildQuery(info provider.MetricInfo, namespace stri
 		fullQuery = prom.Selector(prom.Selector(fmt.Sprintf("rate(%s[%s])", baseQuery, pmodel.Duration(p.rateInterval).String())))
 	}
 
-	// TODO: too small of a rate interval will return no results...
+	// NB: too small of a rate interval will return no results...
 
 	// sum over all other dimensions of this query (e.g. if we select on route, sum across all pods,
 	// but if we select on pods, sum across all routes), and split by the dimension of our resource
@@ -183,7 +181,6 @@ func (p *prometheusProvider) buildQuery(info provider.MetricInfo, namespace stri
 	// TODO: use an actual context
 	queryResults, err := p.promClient.Query(context.Background(), pmodel.Now(), fullQuery)
 	if err != nil {
-		// TODO: interpret this somehow?
 		glog.Errorf("unable to fetch metrics from prometheus: %v", err)
 		// don't leak implementation details to the user
 		return nil, apierr.NewInternalError(fmt.Errorf("unable to fetch metrics"))
@@ -214,29 +211,24 @@ func (p *prometheusProvider) getSingle(info provider.MetricInfo, namespace, name
 
 func (p *prometheusProvider) getMultiple(info provider.MetricInfo, namespace string, selector labels.Selector) (*custom_metrics.MetricValueList, error) {
 	// construct a client to list the names of objects matching the label selector
-	// TODO: figure out version?
 	client, err := p.kubeClient.ClientForGroupVersionResource(info.GroupResource.WithVersion(""))
 	if err != nil {
 		glog.Errorf("unable to construct dynamic client to list matching resource names: %v", err)
-		// TODO: check for resource not found error?
 		// don't leak implementation details to the user
 		return nil, apierr.NewInternalError(fmt.Errorf("unable to list matching resources"))
 	}
 
 	// we can construct a this APIResource ourself, since the dynamic client only uses Name and Namespaced
-	// TODO: use discovery information instead
 	apiRes := &metav1.APIResource{
 		Name:       info.GroupResource.Resource,
 		Namespaced: info.Namespaced,
 	}
 
 	// actually list the objects matching the label selector
-	// TODO: work for objects not in core v1
 	matchingObjectsRaw, err := client.Resource(apiRes, namespace).
 		List(metav1.ListOptions{LabelSelector: selector.String()})
 	if err != nil {
 		glog.Errorf("unable to list matching resource names: %v", err)
-		// TODO: check for resource not found error?
 		// don't leak implementation details to the user
 		return nil, apierr.NewInternalError(fmt.Errorf("unable to list matching resources"))
 	}
@@ -323,9 +315,6 @@ func (l *cachingMetricsLister) RunUntil(stopChan <-chan struct{}) {
 func (l *cachingMetricsLister) updateMetrics() error {
 	startTime := pmodel.Now().Add(-1 * l.updateInterval)
 
-	// TODO: figure out a good way to add all Kubernetes-related metrics at once
-	// (i.e. how do we determine if something is a Kubernetes-related metric?)
-
 	// container-specific metrics from cAdvsior have their own form, and need special handling
 	containerSel := prom.MatchSeries("", prom.NameMatches("^container_.*"), prom.LabelNeq("container_name", "POD"), prom.LabelNeq("namespace", ""), prom.LabelNeq("pod_name", ""))
 	namespacedSel := prom.MatchSeries("", prom.LabelNeq("namespace", ""), prom.NameNotMatches("^container_.*"))