Extend Metrics Transform to be able to add a label to an existing metric #441

ccaraman · 2020-07-14T19:24:16Z

Description: The current metrics transformation processor is extended to support adding a label to an existing metric.
The configuration is.

 - metric_name: some_name
      action: update
      operation:
        - action: add_label
          new_label: mylabel
          new_value: myvalue

Link to tracking Issue: N/A

Testing: Added tests to cover configuration requirements and the logic around adding a label.

Documentation: Readme has been updated.

ccaraman · 2020-07-14T19:24:31Z

cc @JingboWangGoogle as a fyi

codecov · 2020-07-14T19:34:29Z

Codecov Report

Merging #441 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #441      +/-   ##
==========================================
+ Coverage   84.71%   84.74%   +0.02%     
==========================================
  Files         174      174              
  Lines        9389     9404      +15     
==========================================
+ Hits         7954     7969      +15     
  Misses       1113     1113              
  Partials      322      322

Flag	Coverage Δ
#integration	`71.09% <ø> (ø)`
#unit	`84.53% <100.00%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
processor/metricstransformprocessor/factory.go	`100.00% <100.00%> (ø)`
...stransformprocessor/metrics_transform_processor.go	`100.00% <100.00%> (ø)`

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 66db97a...775aa30. Read the comment docs.

JingboWangGoogle

It seems like this operation is simply adding a label into the labelKeys list in the selected metric's descriptor. So how about the label values in the timeseries, do all timeseries just have an empty value for that label? I might be missing something, but some clarification on this would be great.

JingboWangGoogle · 2020-07-14T19:54:20Z

processor/metricstransformprocessor/config.go

@@ -80,7 +83,8 @@ type Operation struct {
 	// AggregatedValues is a list of label values to aggregate away.
 	AggregatedValues []string `mapstructure:"aggregated_values"`

-	// NewValue indicates what is the value called when the AggregatedValues are aggregated into one.
+	// NewValue indicates what is the value called when the AggregatedValues
+	// are aggregated into one or a new label is added to an existing metric.
 	NewValue string `mapstructure:"new_value"`


The NewValue field here used to mean the new label value for the label values being aggregated together, but it seems like in this action add_label, this field means the description for the new label. Please correct me if I am wrong, but this seems like a pretty big difference for this one field in two different actions. Maybe consider having a new field called newDescription or something?

Side note: we probably need to break this struct up for different operation types at some point.

This is just an error in setting the label values at the metric descriptor and not at the time series. Since the intent of hte pr is to update the label values they have the same goal (either in add a new label or when it gets aggregated). To make it cleared, i'll update the comment to say

// NewValue is used to set a new label value either when the operation is `AggregatedValues` or `AddLabel`.

james-bebbington

I have a similar question to Jingbo. Each data point in the metric will have a set of labels - presumably you would you want to add the label with the same value to each data point?

james-bebbington · 2020-07-15T07:07:05Z

processor/metricstransformprocessor/factory.go

+			}
+			if op.Action == AddLabel && op.NewValue == "" {
+				return fmt.Errorf("missing required field %q while %q is %v in the %vth operation", NewValueFieldName, ActionFieldName, AddLabel, i)
+			}


^ Duplicated

Not quite duplicated but I can condense it to say that both NewLabel/NewValue are required. It would make the error message less precise as it won't say what is missing.

My bad 🤦‍♂️

No worries :)

james-bebbington · 2020-07-15T07:07:36Z

processor/metricstransformprocessor/config.go

@@ -80,7 +83,8 @@ type Operation struct {
 	// AggregatedValues is a list of label values to aggregate away.
 	AggregatedValues []string `mapstructure:"aggregated_values"`

-	// NewValue indicates what is the value called when the AggregatedValues are aggregated into one.
+	// NewValue indicates what is the value called when the AggregatedValues
+	// are aggregated into one or a new label is added to an existing metric.
 	NewValue string `mapstructure:"new_value"`


Side note: we probably need to break this struct up for different operation types at some point.

ccaraman · 2020-07-15T22:22:47Z

It seems like this operation is simply adding a label into the labelKeys list in the selected metric's descriptor. So how about the label values in the timeseries, do all timeseries just have an empty value for that label? I might be missing something, but some clarification on this would be great.

That is good point. It should be for the time series labels. That reminds me the latest spec for metrics doesn't have labels on MetricDescriptors. The ascii art here is outdated but the message definition for Descriptor is up to date.. I'll also post this to the open pr for the metrics transform processor.

I'll update the pr to reflect updating the timeseries labels and not the metric descriptors. Thank you for the catch

james-bebbington

LGTM

james-bebbington · 2020-07-17T06:20:04Z

processor/metricstransformprocessor/testdata/config_full.yaml

+          operations:
+            - action: add_label
+              new_label: mylabel
+              new_value: myvalue


Would be good to add this to config_test.go as well

… append (#441) Co-authored-by: Rahul Patel <rghetia@yahoo.com>

Extend Metrics Transform to be able to add a label to an existing metric

eb3e734

ccaraman requested a review from a team as a code owner July 14, 2020 19:24

project-bot bot added this to In progress in Collector Jul 14, 2020

JingboWangGoogle reviewed Jul 14, 2020

View reviewed changes

james-bebbington reviewed Jul 15, 2020

View reviewed changes

ccaraman mentioned this pull request Jul 15, 2020

Aggregation across labels and label values in metrics transform processor #417

Merged

Update label values on timeseries

87c67dd

james-bebbington approved these changes Jul 17, 2020

View reviewed changes

Collector automation moved this from In progress to Reviewer approved Jul 17, 2020

Add missing config test

775aa30

bogdandrutu merged commit 4deae62 into open-telemetry:master Jul 17, 2020

Collector automation moved this from Reviewer approved to Done Jul 17, 2020

ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022

Send span.kind to jaeger and overwrite grpc metadata instead of using…

8fcdf8b

… append (#441) Co-authored-by: Rahul Patel <rghetia@yahoo.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend Metrics Transform to be able to add a label to an existing metric #441

Extend Metrics Transform to be able to add a label to an existing metric #441

ccaraman commented Jul 14, 2020

ccaraman commented Jul 14, 2020

codecov bot commented Jul 14, 2020 •

edited

JingboWangGoogle left a comment

JingboWangGoogle Jul 14, 2020

james-bebbington Jul 15, 2020

ccaraman Jul 15, 2020

james-bebbington left a comment

james-bebbington Jul 15, 2020

ccaraman Jul 15, 2020

james-bebbington Jul 15, 2020

ccaraman Jul 15, 2020

james-bebbington Jul 15, 2020

ccaraman commented Jul 15, 2020

james-bebbington left a comment

james-bebbington Jul 17, 2020

Extend Metrics Transform to be able to add a label to an existing metric #441

Extend Metrics Transform to be able to add a label to an existing metric #441

Conversation

ccaraman commented Jul 14, 2020

ccaraman commented Jul 14, 2020

codecov bot commented Jul 14, 2020 • edited

Codecov Report

JingboWangGoogle left a 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

james-bebbington left a 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

ccaraman commented Jul 15, 2020

james-bebbington left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 14, 2020 •

edited