Skip to content

[small bug fix] [metrics transform processor] check all data points for matching metric label values #3435

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jun 8, 2021
Merged

[small bug fix] [metrics transform processor] check all data points for matching metric label values #3435

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
88ee805
bug fix: check all datapoints for matching metric label values
hossain-rayhan May 20, 2021
ac12527
Address PR feedback, reduce duplicate code
hossain-rayhan Jun 2, 2021
f751932
convert the concrete types to interface once and use regex.Compile()
hossain-rayhan Jun 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 67 additions & 23 deletions processor/metricstransformprocessor/metrics_transform_processor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ type internalOperation struct {
type internalFilter interface {
getMatches(toMatch metricNameMapping) []*match
getSubexpNames() []string
labelMatched(metric *metricspb.Metric) bool
labelMatched(metric *metricspb.Metric) *metricspb.Metric
}

type match struct {
Expand All @@ -78,8 +78,9 @@ func (f internalFilterStrict) getMatches(toMatch metricNameMapping) []*match {
if metrics, ok := toMatch[f.include]; ok {
matches := make([]*match, 0, 10)
for _, metric := range metrics {
if f.labelMatched(metric) {
matches = append(matches, &match{metric: metric})
matchedMetric := f.labelMatched(metric)
if matchedMetric != nil {
matches = append(matches, &match{metric: matchedMetric})
}
}
return matches
Expand All @@ -92,9 +93,13 @@ func (f internalFilterStrict) getSubexpNames() []string {
return nil
}

func (f internalFilterStrict) labelMatched(metric *metricspb.Metric) bool {
func (f internalFilterStrict) labelMatched(metric *metricspb.Metric) *metricspb.Metric {
metricWithMatchedLabel := proto.Clone(metric).(*metricspb.Metric)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to clone? Can you avoid it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than cloning the whole metric object, now I am copying the Resource and MetricDescriptor. We don't want to modify the incoming pdata.Metric, thats way generating a new Metric and returning it. Pushed an update.

Copy link

@sfc-gh-rnishant sfc-gh-rnishant Mar 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hossain-rayhan Was there any specific reasons to clone the objects. Like were you seeing an error or test failures ?
I am asking because cloning seems to be reason for why updates are not working with experimental_match_label.
cc @anuraaga @Aneurysm9

My observation is as follows - When we returns a cloned "MetricDescriptor" object, the update at this point fails -
https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/processor/metricstransformprocessor/metrics_transform_processor.go#L423

This is because, the above line is updating a cloned object rather than updating the original "MetricDescriptor" object.
one such example is here - #7071

Please let me know what do you think ?

var timeSeriesWithMatchedLabel []*metricspb.TimeSeries
labelIndexValueMap := make(map[int]string)

if len(f.matchLabels) == 0 {
return true
return metric
}

for key, value := range f.matchLabels {
Expand All @@ -106,18 +111,35 @@ func (f internalFilterStrict) labelMatched(metric *metricspb.Metric) bool {
}

keyFound = true
if len(metric.Timeseries) > 0 && metric.Timeseries[0].LabelValues[idx].Value != value {
return false
}
labelIndexValueMap[idx] = value
}

// if a label-key is not found then return false only if the given label-value is non-empty. If a given label-value is empty
// and the key is not found then return true. In this approach we can make sure certain key is not present which is a valid use case.
// if a label-key is not found then return nil only if the given label-value is non-empty. If a given label-value is empty
// and the key is not found then move forward. In this approach we can make sure certain key is not present which is a valid use case.
if !keyFound && value != "" {
return false
return nil
}
}

for _, timeseries := range metric.Timeseries {
allValuesMatched := true
for index, value := range labelIndexValueMap {
if timeseries.LabelValues[index].Value != value {
allValuesMatched = false
break
}
}
if allValuesMatched {
timeSeriesWithMatchedLabel = append(timeSeriesWithMatchedLabel, timeseries)
}
}
return true

if len(timeSeriesWithMatchedLabel) == 0 {
return nil
}

metricWithMatchedLabel.Timeseries = timeSeriesWithMatchedLabel
return metricWithMatchedLabel
}

type internalFilterRegexp struct {
Expand All @@ -130,8 +152,9 @@ func (f internalFilterRegexp) getMatches(toMatch metricNameMapping) []*match {
for name, metrics := range toMatch {
if submatches := f.include.FindStringSubmatchIndex(name); submatches != nil {
for _, metric := range metrics {
if f.labelMatched(metric) {
matches = append(matches, &match{metric: metric, pattern: f.include, submatches: submatches})
matchedMetric := f.labelMatched(metric)
if matchedMetric != nil {
matches = append(matches, &match{metric: matchedMetric, pattern: f.include, submatches: submatches})
}
}
}
Expand All @@ -143,9 +166,13 @@ func (f internalFilterRegexp) getSubexpNames() []string {
return f.include.SubexpNames()
}

func (f internalFilterRegexp) labelMatched(metric *metricspb.Metric) bool {
func (f internalFilterRegexp) labelMatched(metric *metricspb.Metric) *metricspb.Metric {
metricWithMatchedLabel := proto.Clone(metric).(*metricspb.Metric)
var timeSeriesWithMatchedLabel []*metricspb.TimeSeries
labelIndexValueMap := make(map[int]*regexp.Regexp)

if len(f.matchLabels) == 0 {
return true
return metric
}

for key, value := range f.matchLabels {
Expand All @@ -157,18 +184,35 @@ func (f internalFilterRegexp) labelMatched(metric *metricspb.Metric) bool {
}

keyFound = true
if len(metric.Timeseries) > 0 && !value.MatchString(metric.Timeseries[0].LabelValues[idx].Value) {
return false
}
labelIndexValueMap[idx] = value
}

// if a label-key is not found then return false only if the given label-value is non-empty. If a given label-value is empty
// and the key is not found then return true. In this approach we can make sure certain key is not present which is a valid use case.
// if a label-key is not found then return nil only if the given label-value is non-empty. If a given label-value is empty
// and the key is not found then move forward. In this approach we can make sure certain key is not present which is a valid use case.
if !keyFound && !value.MatchString("") {
return false
return nil
}
}

for _, timeseries := range metric.Timeseries {
allValuesMatched := true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there's a lot of duplicated code, can you clean it up?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I think much of this commonality could be extracted by defining a StringMatcher interface:

type StringMatcher interface {
	MatchString(string) bool
}

All of the values in internalFilterRegexp will already satisfy it since Regexp has a MatchString(string) method. internalFilterStrict can be made to utilize it by defining a value type:

type strictMatcher string
func (s strictMatcher) MatchString(cmp string) bool {
	return string(s) == cmp
)

At that point the labelMatched() logic can be extracted and updated to this signature:

labelMatched(labelMatchers []StringMatcher, metric *metricspb.Metric) *metricspb.Metric {...}

Since the only value used from the receiver in either of the labelMatched implementations is the list of match targets, this should neatly eliminate the duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to follow your suggestion. Pushed an update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied your suggested code @Aneurysm9 . Please check.

for index, value := range labelIndexValueMap {
if !value.MatchString(timeseries.LabelValues[index].Value) {
allValuesMatched = false
break
}
}
if allValuesMatched {
timeSeriesWithMatchedLabel = append(timeSeriesWithMatchedLabel, timeseries)
}
}
return true

if len(timeSeriesWithMatchedLabel) == 0 {
return nil
}

metricWithMatchedLabel.Timeseries = timeSeriesWithMatchedLabel
return metricWithMatchedLabel
}

type metricNameMapping map[string][]*metricspb.Metric
Expand Down
61 changes: 61 additions & 0 deletions processor/metricstransformprocessor/metrics_transform_processor_testcases_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -707,6 +707,67 @@ var (
addInt64Point(0, 3, 2).build(),
},
},
{
name: "metric_name_insert_with_match_label_regexp_two_datapoints_positive",
transforms: []internalTransform{
{
MetricIncludeFilter: internalFilterRegexp{include: regexp.MustCompile("metric1"), matchLabels: map[string]*regexp.Regexp{"label1": regexp.MustCompile("value3")}},
Action: Insert,
NewName: "new/metric1",
},
},
in: []*metricspb.Metric{
metricBuilder().setName("metric1").
setLabels([]string{"label1", "label2"}).
setDataType(metricspb.MetricDescriptor_GAUGE_INT64).
addTimeseries(1, []string{"value1", "value2"}).
addTimeseries(2, []string{"value3", "value4"}).
addInt64Point(0, 3, 2).
addInt64Point(1, 3, 2).build(),
},
out: []*metricspb.Metric{
metricBuilder().setName("metric1").
setLabels([]string{"label1", "label2"}).
setDataType(metricspb.MetricDescriptor_GAUGE_INT64).
addTimeseries(1, []string{"value1", "value2"}).
addTimeseries(2, []string{"value3", "value4"}).
addInt64Point(0, 3, 2).
addInt64Point(1, 3, 2).build(),
metricBuilder().setName("new/metric1").
setLabels([]string{"label1", "label2"}).
setDataType(metricspb.MetricDescriptor_GAUGE_INT64).
addTimeseries(2, []string{"value3", "value4"}).
addInt64Point(0, 3, 2).build(),
},
},
{
name: "metric_name_insert_with_match_label_regexp_two_datapoints_negative",
transforms: []internalTransform{
{
MetricIncludeFilter: internalFilterRegexp{include: regexp.MustCompile("metric1"), matchLabels: map[string]*regexp.Regexp{"label1": regexp.MustCompile("value3")}},
Action: Insert,
NewName: "new/metric1",
},
},
in: []*metricspb.Metric{
metricBuilder().setName("metric1").
setLabels([]string{"label1", "label2"}).
setDataType(metricspb.MetricDescriptor_GAUGE_INT64).
addTimeseries(1, []string{"value1", "value2"}).
addTimeseries(2, []string{"value11", "value22"}).
addInt64Point(0, 3, 2).
addInt64Point(1, 3, 2).build(),
},
out: []*metricspb.Metric{
metricBuilder().setName("metric1").
setLabels([]string{"label1", "label2"}).
setDataType(metricspb.MetricDescriptor_GAUGE_INT64).
addTimeseries(1, []string{"value1", "value2"}).
addTimeseries(2, []string{"value11", "value22"}).
addInt64Point(0, 3, 2).
addInt64Point(1, 3, 2).build(),
},
},
{
name: "metric_name_insert_with_match_label_regexp_with_full_value",
transforms: []internalTransform{
Expand Down