From 4b8952155a9da1d01ac176610e908b3a8caf7e88 Mon Sep 17 00:00:00 2001 From: Cristian Greco Date: Mon, 31 Jul 2023 19:32:32 +0200 Subject: [PATCH] Fix maxdimassociator to loop through all mappings Mappings in the new associator are sorted by decreasing number of dimensions, e.g. [ Mapping_Dim_A_Dim_B, Mapping_Dim_A_Dim_C, Mapping_Dim_A ] If trying to map a metric with Dim_A, Dim_B and Dim_C, the current implementation would pic the first mapping (Mapping_Dim_A_Dim_B) because it already matches part of the metric dimensions and would try to find a match for dimensions names and values. However, if values don't match it would give up and mark the metric to be skipped. The new implementation continues to loop through the regex mappings to check if there another one that contains the metric dimensions. In the example above, Mapping_Dim_A_Dim_C would match again and the algo would try again to find a match for dimensions names and values. This should fix cases where ListMetrics returns metrics with more dimensions than can be extracted from the ARN regex. E.g. for ApiGateway the ARN contains only the ApiId dimension, but we should now be able to match metrics that contains both ApiId and Method (as long as the ApiId value matches). --- pkg/job/maxdimassociator/associator.go | 112 +++++++++++------- .../associator_api_gateway_test.go | 3 +- 2 files changed, 72 insertions(+), 43 deletions(-) diff --git a/pkg/job/maxdimassociator/associator.go b/pkg/job/maxdimassociator/associator.go index bf6c6e602..7d7e557c2 100644 --- a/pkg/job/maxdimassociator/associator.go +++ b/pkg/job/maxdimassociator/associator.go @@ -35,6 +35,23 @@ type dimensionsRegexpMapping struct { dimensionsMapping map[uint64]*model.TaggedResource } +func (rm dimensionsRegexpMapping) toString() string { + sb := strings.Builder{} + sb.WriteString("{dimensions=[") + for _, dim := range rm.dimensions { + sb.WriteString(dim) + } + sb.WriteString("], dimensions_mappings={") + for sign, res := range rm.dimensionsMapping { + sb.WriteString(fmt.Sprintf("%d", sign)) + sb.WriteString("=") + sb.WriteString(res.ARN) + sb.WriteString(",") + } + sb.WriteString("}}") + return sb.String() +} + // NewAssociator builds all mappings for the given dimensions regexps and list of resources. func NewAssociator(logger logging.Logger, dimensionRegexps []*regexp.Regexp, resources []*model.TaggedResource) Associator { assoc := Associator{ @@ -78,7 +95,17 @@ func NewAssociator(logger logging.Logger, dimensionRegexps []*regexp.Regexp, res mappedResources[idx] = true } - assoc.mappings = append(assoc.mappings, m) + if len(m.dimensionsMapping) > 0 { + assoc.mappings = append(assoc.mappings, m) + } + + // The mapping might end up as empty in cases e.g. where + // one of the regexps defined for the namespace doesn't match + // against any of the tagged resources. This might happen for + // example when we define multiple regexps (to capture sibling + // or sub-resources) and one of them doesn't match any resource. + // This behaviour is ok, we just want to debug log to keep track of it. + logger.Debug("unable to define a regex mapping", "regex", regex.String()) } // sort all mappings by decreasing number of dimensions names @@ -89,17 +116,8 @@ func NewAssociator(logger logging.Logger, dimensionRegexps []*regexp.Regexp, res }) if logger.IsDebugEnabled() { - for idx, m := range assoc.mappings { - sb := strings.Builder{} - sb.WriteString("{") - for k, v := range m.dimensionsMapping { - sb.WriteString(fmt.Sprintf("%d", k)) - sb.WriteString("=") - sb.WriteString(v.ARN) - sb.WriteString(",") - } - sb.WriteString("}") - logger.Debug("associator mapping", "idx", idx, "dimensions", strings.Join(m.dimensions, ","), "dimensions_mapping", sb.String()) + for idx, regexpMapping := range assoc.mappings { + logger.Debug("associator mapping", "mapping_idx", idx, "mapping", regexpMapping.toString()) } } @@ -111,8 +129,10 @@ func NewAssociator(logger logging.Logger, dimensionRegexps []*regexp.Regexp, res // In case a map can't be found, the second return parameter indicates whether the metric should be // ignored or not. func (assoc Associator) AssociateMetricToResource(cwMetric *model.Metric) (*model.TaggedResource, bool) { + logger := assoc.logger.With("metric_name", cwMetric.MetricName) + if len(cwMetric.Dimensions) == 0 { - assoc.logger.Debug("metric has no dimensions", "metric", cwMetric.MetricName) + logger.Debug("metric has no dimensions, don't skip") // Do not skip the metric (create a "global" metric) return nil, false @@ -123,41 +143,49 @@ func (assoc Associator) AssociateMetricToResource(cwMetric *model.Metric) (*mode dimensions = append(dimensions, dimension.Name) } - if assoc.logger.IsDebugEnabled() { - assoc.logger.Debug("associating metric", "metric_name", cwMetric.MetricName, "dimensions", strings.Join(dimensions, ",")) + if logger.IsDebugEnabled() { + logger.Debug("associate loop start", "dimensions", strings.Join(dimensions, ",")) } - // Find the mapping which contains the most + // Attempt to find the regex mapping which contains the most // (but not necessarily all) the metric's dimensions names. - // Mappings are sorted by decreasing number of dimensions names. - var regexpMapping *dimensionsRegexpMapping - for idx, m := range assoc.mappings { - if containsAll(dimensions, m.dimensions) { - assoc.logger.Debug("matched metric with mapping", "mapping_idx", idx) - regexpMapping = m - break - } - } - - if regexpMapping == nil { - // if no mapping is found, it means the ListMetrics API response - // did not contain any subset of the metric's dimensions names. - // Do not skip the metric though (create a "global" metric). - return nil, false - } - - // A mapping has been found. The metric has all (and possibly more) - // the dimensions computed for the mapping. Pick only exactly - // the dimensions of the mapping to build a labels signature. - labels := buildLabelsMap(cwMetric, regexpMapping) - signature := prom_model.LabelsToSignature(labels) + // Regex mappings are sorted by decreasing number of dimensions names, + // which favours find the mapping with most dimensions. + mappingFound := false + for idx, regexpMapping := range assoc.mappings { + if containsAll(dimensions, regexpMapping.dimensions) { + logger.Debug("found mapping", "mapping_idx", idx, "mapping", regexpMapping.toString()) + + // A regex mapping has been found. The metric has all (and possibly more) + // the dimensions computed for the mapping. Now compute a signature + // of the labels (names and values) of the dimensions of this mapping. + mappingFound = true + labels := buildLabelsMap(cwMetric, regexpMapping) + signature := prom_model.LabelsToSignature(labels) - if resource, ok := regexpMapping.dimensionsMapping[signature]; ok { - return resource, false + // Check if there's an entry for the labels (names and values) of the metric, + // and return the resource in case. + if resource, ok := regexpMapping.dimensionsMapping[signature]; ok { + logger.Debug("resource matched", "signature", signature) + return resource, false + } else { + // Otherwise, continue iterating across the rest of regex mappings + // to attempt to find another one with fewer dimensions. + logger.Debug("resource not matched", "signature", signature) + } + } } - // if there's no mapping entry for this resource, skip it - return nil, true + // At this point, we haven't been able to match the metric against + // any resource based on the dimensions the associator knows. + // If a regex mapping was ever found in the loop above but no entry + // (i.e. matching labels names and values) matched the metric dimensions, + // skip the metric altogether. + // Otherwise, if we didn't find any regex mapping it means we can't + // correctly map the dimensions names to a resource arn regex, + // but we still want to keep the metric and create a "global" metric. + logger.Debug("associate loop end", "skip", mappingFound) + return nil, mappingFound } // buildLabelsMap returns a map of labels names and values. diff --git a/pkg/job/maxdimassociator/associator_api_gateway_test.go b/pkg/job/maxdimassociator/associator_api_gateway_test.go index e35121f9d..4183dbe29 100644 --- a/pkg/job/maxdimassociator/associator_api_gateway_test.go +++ b/pkg/job/maxdimassociator/associator_api_gateway_test.go @@ -128,7 +128,8 @@ func TestAssociatorAPIGateway(t *testing.T) { }, }, }, - expectedSkip: true, + expectedSkip: false, + expectedResource: apiGatewayV1, }, }