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

[hossain-rayhan]

Description:
This change fixes a small bug for newly added experimental_match_label feature in the metricstransform processor. Earlier we used to check only the first datapoint while matching the metric label values. But that case is not always true and the feature is not working to support our use case. This change checks all the data points and search for proper metric label value match.

Testing:
Unit tests added.
Manually tested.

Documentation:
README

[anuraaga]

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

[hossain-rayhan]

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.

[sfc-gh-rnishant]

@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 ?

[anuraaga]

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

[Aneurysm9]

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.

[hossain-rayhan]

Tried to follow your suggestion. Pushed an update.

[hossain-rayhan]

Copied your suggested code @Aneurysm9 . Please check.

[Aneurysm9]

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.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[hossain-rayhan]

Will address the PR feedbacks.

[hossain-rayhan]

Hi @Aneurysm9 thanks for your suggestion. I copied your code to update the map conversion and used regex.Compile() instead of regex.MustCompile(). Please have another look.

[hossain-rayhan]

Hi @tigrannajaryan and @bogdandrutu can we get this small bug fix merged?