[metrics_transform_processor] Add filtering capabilities matching metric label values for applying changes #3201

hossain-rayhan · 2021-04-21T20:59:56Z

Description:
This change adds a new feature which will support to filter metrics and apply changes matching metric names and label values together. Earlier, we were able to filter metrics using only metric names and apply our transforms to all the metrics. This enhanced filtering feature will give a new option to apply changes to a certain set of metrics where we have a match for metric name and specific set of metric labels.

Can you share a use case?
Yes. Using prometheus receiver in Kubernetes, we get same metrics (same name) for containers as well as pods. We can differentiate the metrics using metric label values. In our use cases we need to rename some of the pod level metrics without affecting the container level metrics. Hence, we need an option for applying changes to metircs based on metric names and metric label values together.

Why did you put this change in metricstransformprocessor?
Metrics transform processor already have the insert and update functions. It applies changes to metrics matching the metric name. Its super easy and meaningful to add couple of lines to the filtering options to match metrics using metric label values besides metric names.

Testing:
Wrote Unit tests and tested manually on local machine.

Documentation:
Updated README with proper config examples.

codecov · 2021-04-21T21:20:52Z

Codecov Report

Merging #3201 (a4d48a5) into main (48eaccc) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head a4d48a5 differs from pull request most recent head 9c29946. Consider uploading reports for the commit 9c29946 to get more accurate results

@@            Coverage Diff             @@
##             main    #3201      +/-   ##
==========================================
- Coverage   91.92%   91.92%   -0.01%     
==========================================
  Files         493      493              
  Lines       23946    23973      +27     
==========================================
+ Hits        22013    22037      +24     
- Misses       1427     1429       +2     
- Partials      506      507       +1

Flag	Coverage Δ
integration	`63.35% <ø> (-0.06%)`	⬇️
unit	`90.94% <100.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
processor/metricstransformprocessor/config.go	`100.00% <ø> (ø)`
processor/metricstransformprocessor/factory.go	`98.86% <100.00%> (+0.04%)`	⬆️
...stransformprocessor/metrics_transform_processor.go	`99.39% <100.00%> (+0.10%)`	⬆️
receiver/k8sclusterreceiver/watcher.go	`95.29% <0.00%> (-2.36%)`	⬇️
processor/groupbytraceprocessor/event.go	`95.96% <0.00%> (-0.81%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48eaccc...9c29946. Read the comment docs.

bogdandrutu · 2021-04-21T22:49:45Z

Thanks for this PR @hossain-rayhan, unfortunately we don't want to add new functionality to the metrics transform processor until we change it to use the new pdata instead of the old opencensus

hossain-rayhan · 2021-04-21T22:54:23Z

Thanks for this PR @hossain-rayhan, unfortunately we don't want to add new functionality to the metrics transform processor until we change it to use the new pdata instead of the old opencensus

Hi @bogdandrutu, this is a small addition. I understand the concern but this change truely belongs to this metricstransformprocessor. I can give you my word to contribute for changing this part when rewriting the processor. Maybe I can create an issue and assign it to me for this. But for now, this is the fasted path forward for us. Also, you can tag me if we have any open issue for re-writing this processor. Waiting for your opinion. Thanks.

hossain-rayhan · 2021-04-23T17:14:15Z

Hi @bogdandrutu, expecting a second thought from you considering my reply on your comment. Let me know if it sounds convincing.

hossain-rayhan · 2021-04-28T17:22:52Z

According to our todays (04/28) Collector SIG meetings decision, we will move forward with this PR. I created an issue to rewrite this part to use OTLP metrics instead of OpenCensus metric type. I will work on this when we plan to rewrite the metricstransformprocessor. Let's review this.

Issue Link to track: #3269
@alolita can you please assign the issue to me?

Thanks @bogdandrutu and @alolita

tigrannajaryan · 2021-04-30T16:58:16Z

@bogdandrutu re-assigning to you since you discussed this in SIG meeting.

hossain-rayhan · 2021-05-03T18:19:37Z

Hi @bogdandrutu its blocking our release. A review would be highly appreciated.

Aneurysm9 · 2021-05-03T21:48:38Z

processor/metricstransformprocessor/metrics_transform_processor.go

@@ -88,15 +91,18 @@ func (f internalFilterStrict) getSubexpNames() []string {
 }

 type internalFilterRegexp struct {
-	include *regexp.Regexp
+	include     *regexp.Regexp
+	matchLabels map[string]string


Suggested change

matchLabels map[string]string

matchLabels map[string]*regexp.Regexp

Presuming that this internalFilterRegexp is re-used, it would be better to transform the label matching map values to *regexp.Regexp once up-front rather than every time we call labelMatched().

Aneurysm9 · 2021-05-03T21:53:40Z

processor/metricstransformprocessor/metrics_transform_processor.go

+			keyFound = true
+			for _, timeseries := range metric.Timeseries {
+				if isRegexp {
+					re := regexp.MustCompile(value)


This should be done in the outer loop, not multiple times per filtered label. Ideally, as mentioned above, it would be done once on initialization and re-used.

Aneurysm9 · 2021-05-03T21:56:20Z

processor/metricstransformprocessor/metrics_transform_processor.go

+		for idx, label := range metric.MetricDescriptor.LabelKeys {
+			if label.Key != key {
+				continue
+			}


Can the same label key appear multiple times? If not, it might make sense to build a map from label keys to indexes once at the top of this function and look up indexes directly rather than iterating over the label keys array for every filtered label.

Technically it won't repeat. But as its not a map, we cannot garuntee I think.

Aneurysm9 · 2021-05-03T22:54:29Z

processor/metricstransformprocessor/metrics_transform_processor.go

+		}
+
+		// if a label-key is not found and the label-value is non-empty, return false
+		if !keyFound && !(value == "" || isEmptyExp(value)) {


I don't think this logic matches the description in the comment. It looks like a non-empty label value that is matched by a regexp that could also match an empty label value will hit this condition. What is intended to be tested here?

We have two different use case here. Say, in our config file we have the following labels to match,

{"container": "my-container"} where the value of the key container is non-empty.

{"container": ""} where the value of the key container is empty-string.

For first case, if we don't find a key, we return false. But for the second case, if we don't find the key, we need to return true which is expected.

As we don't have a way to make sure a given key is not present, I wrote this logic as alternative. If we want to confirm that a given label is not present, we set the value as empty-string.

Hope it helps to explain.

Updated the comment to make it more clear.

mxiamxia

LGTM!

hossain-rayhan · 2021-05-05T01:52:58Z

@alolita can you please add the ready_to_get_merged tag on this. Got approval from Anthony and Min. Thanks.

anuraaga · 2021-05-05T03:16:20Z

processor/metricstransformprocessor/metrics_transform_processor.go

+				if timeseries.LabelValues[idx].Value != value {
+					return false
+				}
+				break


Because of this break, isn't only the first time series checked?

Yes. That's I checked for. If the first timeseries matches the label, we will add the metric.

Better to be explicit about that than use a loop then, loop is for looping

Can you please rephrase for me? I don't think I understand what you meant.

If you only want to check the first one can't you use metric.Timeseries[0]?

Oh I see. Makes sense. Will update.

Updated, thanks.

hossain-rayhan · 2021-05-05T20:35:05Z

Hi @anuraaga I need your approval. Would you please have another look. Thanks.

hossain-rayhan · 2021-05-06T14:56:08Z

@tigrannajaryan can we get this merged! Thanks.

tigrannajaryan · 2021-05-06T17:51:22Z

@bogdandrutu what's the resolution on this? I was not present in the SIG meeting where you discussed this (the meeting notes say that it was discussed, but doesn't tell what was decided).

hossain-rayhan · 2021-05-10T13:52:44Z

@bogdandrutu We need to get this merged today (05/10). Can you please merge this. Thanks.
cc: @alolita

Signed-off-by: Rayhan Hossain <[email protected]>

hossain-rayhan · 2021-05-10T16:22:47Z

I just rebased the code so that it passes windows/trace unit tests.

jmacd · 2021-05-10T17:14:14Z

👍

…le (#3201) Signed-off-by: Bogdan Drutu <[email protected]>

hossain-rayhan requested a review from a team April 21, 2021 20:59

github-actions bot assigned tigrannajaryan Apr 21, 2021

This was referenced Apr 28, 2021

Design: proposing new metricsgenerationprocessor #2722

Closed

metricstransformprocessor: for filtering with label use OTLP metric type instead of OpenCensus type #3269

Closed

tigrannajaryan assigned bogdandrutu and unassigned tigrannajaryan Apr 30, 2021

Aneurysm9 reviewed May 3, 2021

View reviewed changes

Aneurysm9 approved these changes May 4, 2021

View reviewed changes

mxiamxia approved these changes May 5, 2021

View reviewed changes

anuraaga reviewed May 5, 2021

View reviewed changes

Aneurysm9 mentioned this pull request May 5, 2021

Track @Aneurysm9 interest and contributions for approver open-telemetry/opentelemetry-collector#2820

Closed

anuraaga approved these changes May 5, 2021

View reviewed changes

hossain-rayhan added 6 commits May 10, 2021 08:47

Add metric filtering config matching label values

2b08492

Signed-off-by: Rayhan Hossain <[email protected]>

Add unit tests for filtering using metric labels

c22e854

Signed-off-by: Rayhan Hossain <[email protected]>

Update README for metric filtering config with metric labels

47d8daa

Signed-off-by: Rayhan Hossain <[email protected]>

Process regexp once earlier and use every time when matching labels

6d74696

Signed-off-by: Rayhan Hossain <[email protected]>

fix linter issue

23ce51a

Signed-off-by: Rayhan Hossain <[email protected]>

Add unit test to improve coverage

e30e0af

Signed-off-by: Rayhan Hossain <[email protected]>

hossain-rayhan added 2 commits May 10, 2021 08:47

nit: refactor code following PR feedback

7fa7ee8

Signed-off-by: Rayhan Hossain <[email protected]>

Prefix match_labels feature with experimental_

894f54e

Signed-off-by: Rayhan Hossain <[email protected]>

hossain-rayhan force-pushed the metrics_transform_filter branch from 9c29946 to 894f54e Compare May 10, 2021 15:51

bogdandrutu merged commit abea898 into open-telemetry:main May 10, 2021

alexperez52 referenced this pull request in open-o11y/opentelemetry-collector-contrib Aug 18, 2021

Change fileexporter to use the new SharedComponents because of the fi…

1096792

…le (#3201) Signed-off-by: Bogdan Drutu <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[metrics_transform_processor] Add filtering capabilities matching metric label values for applying changes #3201

[metrics_transform_processor] Add filtering capabilities matching metric label values for applying changes #3201

hossain-rayhan commented Apr 21, 2021 •

edited

Loading

codecov bot commented Apr 21, 2021 •

edited

Loading

bogdandrutu commented Apr 21, 2021

hossain-rayhan commented Apr 21, 2021 •

edited

Loading

hossain-rayhan commented Apr 23, 2021 •

edited

Loading

hossain-rayhan commented Apr 28, 2021

tigrannajaryan commented Apr 30, 2021

hossain-rayhan commented May 3, 2021

Aneurysm9 May 3, 2021

hossain-rayhan May 4, 2021

Aneurysm9 May 3, 2021

hossain-rayhan May 4, 2021

Aneurysm9 May 3, 2021

hossain-rayhan May 4, 2021

Aneurysm9 May 3, 2021

hossain-rayhan May 3, 2021

hossain-rayhan May 4, 2021

mxiamxia left a comment

hossain-rayhan commented May 5, 2021

anuraaga May 5, 2021

hossain-rayhan May 5, 2021

anuraaga May 5, 2021 •

edited

Loading

hossain-rayhan May 5, 2021

anuraaga May 5, 2021

hossain-rayhan May 5, 2021

hossain-rayhan May 5, 2021

hossain-rayhan commented May 5, 2021

hossain-rayhan commented May 6, 2021

tigrannajaryan commented May 6, 2021

hossain-rayhan commented May 10, 2021

hossain-rayhan commented May 10, 2021

jmacd commented May 10, 2021

	matchLabels map[string]string
	matchLabels map[string]*regexp.Regexp

[metrics_transform_processor] Add filtering capabilities matching metric label values for applying changes #3201

[metrics_transform_processor] Add filtering capabilities matching metric label values for applying changes #3201

Conversation

hossain-rayhan commented Apr 21, 2021 • edited Loading

codecov bot commented Apr 21, 2021 • edited Loading

Codecov Report

bogdandrutu commented Apr 21, 2021

hossain-rayhan commented Apr 21, 2021 • edited Loading

hossain-rayhan commented Apr 23, 2021 • edited Loading

hossain-rayhan commented Apr 28, 2021

tigrannajaryan commented Apr 30, 2021

hossain-rayhan commented May 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxiamxia left a comment

Choose a reason for hiding this comment

hossain-rayhan commented May 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga May 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hossain-rayhan commented May 5, 2021

hossain-rayhan commented May 6, 2021

tigrannajaryan commented May 6, 2021

hossain-rayhan commented May 10, 2021

hossain-rayhan commented May 10, 2021

jmacd commented May 10, 2021

hossain-rayhan commented Apr 21, 2021 •

edited

Loading

codecov bot commented Apr 21, 2021 •

edited

Loading

hossain-rayhan commented Apr 21, 2021 •

edited

Loading

hossain-rayhan commented Apr 23, 2021 •

edited

Loading

anuraaga May 5, 2021 •

edited

Loading