Add filter field to prometheus encoder #147

ronensc · 2022-03-15T15:02:17Z

Follow-up for #146. Please review that one before this.

eranra · 2022-03-16T12:44:23Z

pkg/api/encode_prom.go

 }

 type PromMetricsItems []PromMetricsItem
+
+type PromMetricsFilter struct {


Do we need the prefix Prom ... consider:: s/PromMetricsFilter /MetricsFilter
maybe we need that to make the API easier ... this is just a suggestion

I added the the Prom prefix to follow the convention in the file.

eranra · 2022-03-16T12:46:12Z

pkg/pipeline/encode/encode_prom.go

-	input      string
-	labelNames []string
+	input       string
+	filterKey   string


consider putting key and value in sub-struct (I just think that will happen if we decide to change the parameters of the filter)

eranra · 2022-03-16T12:47:54Z

pkg/pipeline/encode/encode_prom.go

@@ -102,9 +104,15 @@ func (e *encodeProm) EncodeMetric(metric config.GenericMap) []config.GenericMap
 	// TODO: We may need different handling for histograms
 	out := make([]config.GenericMap, 0)
 	for metricName, mInfo := range e.metrics {
+		val, keyFound := metric[mInfo.filterKey]
+		shouldKeepRecord := keyFound && val == mInfo.filterValue


A very fast "thing" that you can improve here is to say ... if there is a Key and the value equals to some N/A value we filter just on the basis of the existence of the Key and we do not care about the value of the value ... trivial change and adds a lot of flexibility

I see what you are suggesting and I agree that it will add flexibility. But, I think we should put more thought into that.
I don't like magic values that might collide with the user's values.

eranra · 2022-03-16T12:48:28Z

pkg/pipeline/encode/encode_prom.go

 		metricValue, ok := metric[mInfo.input]
 		if !ok {
-			log.Debugf("field %v is missing", metricName)
+			log.Errorf("field %v is missing", mInfo.input)


@KalmanMeth can you advise why we had originally Debug here?

I believe the reason for using Debug originally is that before introducing dedicated filter field, the filtering was done based on the input field. So it wasn't an error if the expected input field didn't exist. It probably indicated that the record didn't belong to the metric in context.
The input field basically had two roles:

hold the value we wanted to measure

filter records - associate a record with a metric.

because of this, prefixes were added to the input key.
This PR allows removing these prefixes and remove the 2nd role of the input field.

eranra · 2022-03-16T12:50:06Z

pkg/confgen/encode.go

-		}
-	}
-
+


Any other cross phases check that you think we can and should add here???

Consider the following configuration of a prom metric

- name: bandwidth_per_network_service type: counter filter: key: name value: bandwidth_network_service valuekey: recent_op_value labels: - by - aggregate buckets: []

If prom_encode follows extract_aggregate I can think of the following validations:

valuekey is one of the keys of the following GenericMap

flowlogs-pipeline/pkg/pipeline/extract/aggregate/aggregate.go

Lines 189 to 199 in c0f17db

"name": aggregate.Definition.Name,

"operation": aggregate.Definition.Operation,

"record_key": aggregate.Definition.RecordKey,

"by": strings.Join(aggregate.Definition.By, ","),

"aggregate": string(group.normalizedValues),

"total_value": fmt.Sprintf("%f", group.value),

"recentRawValues": group.RecentRawValues,

"total_count": fmt.Sprintf("%d", group.count),

"recent_op_value": group.recentOpValue,

"recent_count": group.recentCount,

strings.Join(aggregate.Definition.By, "_"): string(group.normalizedValues),

Same is true about labels and filter.key

if filter.key == name then we can validate that filter.value is defined.

regarding type
4.1. if type == gauge then valuekey is in [total_value, total_count]
4.2. if type == counter then valuekey is in [recent_op_value, recent_count]
4.3. if type == histogram then valuekey is recentRawValues

histograms have at least one bucket

I find it confusing to have an argument variable named "metric" and a data member named "metrics"

codecov-commenter · 2022-03-20T16:51:59Z

Codecov Report

Merging #147 (c97cb0c) into main (beb3c57) will increase coverage by 0.01%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##             main     #147      +/-   ##
==========================================
+ Coverage   58.44%   58.46%   +0.01%     
==========================================
  Files          51       51              
  Lines        2950     2949       -1     
==========================================
  Hits         1724     1724              
  Misses       1113     1113              
+ Partials      113      112       -1

Flag	Coverage Δ
unittests	`58.46% <93.33%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/api/encode_prom.go	`100.00% <ø> (ø)`
pkg/confgen/encode.go	`50.00% <ø> (+13.63%)`	⬆️
pkg/pipeline/encode/encode_prom.go	`81.08% <87.50%> (-0.70%)`	⬇️
pkg/pipeline/extract/aggregate/aggregate.go	`94.87% <100.00%> (-0.09%)`	⬇️
pkg/test/utils.go	`82.41% <100.00%> (-0.20%)`	⬇️
pkg/pipeline/ingest/ingest_collector.go	`62.50% <0.00%> (-1.67%)`	⬇️

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 beb3c57...c97cb0c. Read the comment docs.

ronensc self-assigned this Mar 15, 2022

ronensc requested review from eranra and KalmanMeth March 15, 2022 15:17

eranra reviewed Mar 16, 2022

View reviewed changes

eranra approved these changes Mar 16, 2022

View reviewed changes

ronensc mentioned this pull request Mar 20, 2022

Refactor extractor fields #146

Merged

ronensc added 12 commits March 20, 2022 17:24

Add filter to prom encoder

a4a1db8

Fix tests

1575576

Rename a variable metric->metricRecord

0e3f31e

I find it confusing to have an argument variable named "metric" and a data member named "metrics"

Remove prefix

98c7111

Remove unique ValueKey constraint

3c5ed3c

Update network_definitions

c02716e

Update api.md

058b781

Fix grammer

84f9513

Update docs

5e5ad80

Make linter happy

ee9acda

fixup

a0366ed

Address Eran's comment: extract key&value to sub-struct

9ad430c

ronensc force-pushed the add-prom-filter branch from df04ef1 to 9ad430c Compare March 20, 2022 16:43

Update docs

c97cb0c

ronensc merged commit 6668d30 into netobserv:main Mar 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add filter field to prometheus encoder #147

Add filter field to prometheus encoder #147

ronensc commented Mar 15, 2022 •

edited

Loading

eranra Mar 16, 2022 •

edited

Loading

ronensc Mar 20, 2022

eranra Mar 16, 2022

eranra Mar 16, 2022

ronensc Mar 20, 2022

eranra Mar 16, 2022

ronensc Mar 20, 2022

eranra Mar 16, 2022

ronensc Mar 20, 2022

codecov-commenter commented Mar 20, 2022 •

edited

Loading

	"name": aggregate.Definition.Name,
	"operation": aggregate.Definition.Operation,
	"record_key": aggregate.Definition.RecordKey,
	"by": strings.Join(aggregate.Definition.By, ","),
	"aggregate": string(group.normalizedValues),
	"total_value": fmt.Sprintf("%f", group.value),
	"recentRawValues": group.RecentRawValues,
	"total_count": fmt.Sprintf("%d", group.count),
	"recent_op_value": group.recentOpValue,
	"recent_count": group.recentCount,
	strings.Join(aggregate.Definition.By, "_"): string(group.normalizedValues),

Add filter field to prometheus encoder #147

Add filter field to prometheus encoder #147

Conversation

ronensc commented Mar 15, 2022 • edited Loading

eranra Mar 16, 2022 • 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

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

codecov-commenter commented Mar 20, 2022 • edited Loading

Codecov Report

ronensc commented Mar 15, 2022 •

edited

Loading

eranra Mar 16, 2022 •

edited

Loading

codecov-commenter commented Mar 20, 2022 •

edited

Loading