pkg/otlp/metrics: fix otel.* namespaced metrics #142

gbbr · 2023-08-02T14:19:38Z

This change fixes a problem where sytem.* and process.* metrics were correctly namespaced with otel.*, but the original metric was not kept.

songy23 · 2023-08-02T15:21:29Z

pkg/otlp/metrics/metrics_remapping.go

@@ -70,7 +70,9 @@ func remapSystemMetrics(all pmetric.MetricSlice, m pmetric.Metric) {
 		copyMetric(all, m, "system.disk.in_use", 1)
 	}
 	// process.* and system.* metrics need to be prepended with the otel.* namespace
-	m.SetName("otel." + m.Name())
+	newm := all.AppendEmpty()


Chatted with @liustanley , only the metrics from the previous extractSystemMetrics should be kept: https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/23445/files#diff-59a3a2cae4395f15cad4800806f9dc7680aed0f303dd896c0ca8663e0fd0ac35L56

For process.* metrics, only the ones with otel.* prefix are expected, original ones are expected to be overriden.

Sorry but I don't see the difference between this implementation and the one in the linked code. All metrics that start with system.* and process.* seem to get the prefix. I don't see any special case for process.*

Both of system.* and process.* metrics get the otel. prefix - that's right. In addition to that, the original system.* metrics (no otel. prefix) should be preserved / copied, while process.* metrics don't.

I understand, but I don't see that in the old code. The behaviour seems the same for both process and system

Either way, in my latest commit, I think that I have applied what you meant. PTAL

songy23 · 2023-08-04T13:42:28Z

pkg/otlp/metrics/metrics_remapping.go

@@ -69,7 +69,12 @@ func remapSystemMetrics(all pmetric.MetricSlice, m pmetric.Metric) {
 	case "system.filesystem.utilization":
 		copyMetric(all, m, "system.disk.in_use", 1)
 	}
-	// process.* and system.* metrics need to be prepended with the otel.* namespace
+	if strings.HasPrefix(name, "system.") {
+		// we keep the original metric for system.* metrics


I looked again at the old code, I think the behavior is still different: we prepend otel. to the original names, but we keep the remapped names with no otel. prefix. https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/23445/files#diff-59a3a2cae4395f15cad4800806f9dc7680aed0f303dd896c0ca8663e0fd0ac35L115-L129

For example, in the old code:

if we have "system.filesystem.utilization", first we remap it to "system.disk.in_use" and append the new one to slice

then we prepend otel. to "system.filesystem.utilization" but not
"system.disk.in_use"

in the end we got "otel.system.filesystem.utilization" and "system.disk.in_use"

vs.

Before this PR, in the end we got "otel.system.filesystem.utilization" and "otel.system.disk.in_use"

With the current PR, in the end we got "system.filesystem.utilization", "system.disk.in_use", "otel.system.filesystem.utilization" and "otel.system.disk.in_use"

Both of ^ are wrong. In short, we prepend otel. to the original metrics, but don't prepend it to the remapped metrics.

Sorry for the confusion, I was also trying to understand the old behavior.

I think you are misunderstanding the current PR. It works exactly like you expect (in your "old code" example above). I have simplified the tests for you to make it easier for you to understand. Feel free to set up a call if it's still not clear.

Now, in the tests:

The in value contains the original metric

The out value contains any new (!) metrics added. In the case of system.* metrics you will notice there is a copy of the original.

The test asserts that the original metric (from in, not out) is prepended with otel.* when needed.

If I'm not mistaken: using the same example, now after remapMetrics we're getting 3 metrics: "otel.system.filesystem.utilization", "system.disk.in_use" and "system.filesystem.utilization"?

If so, the last one "system.filesystem.utilization" shouldn't be there

dineshg13 · 2023-08-07T19:31:30Z

pkg/otlp/metrics/metrics_remapping_test.go

+			in: metric("system.cpu.load_average.1m", point{f: 1}),
+			out: []pmetric.Metric{
+				metric("system.load.1", point{f: 1}),
+				metric("system.cpu.load_average.1m", point{f: 1}),


shouldn't this be otel.system.cpu.load_average.1m ?

songy23 · 2023-08-07T20:06:54Z

pkg/otlp/metrics/metrics_remapping_test.go

-			require.Equal(t, out, dest.At(dest.Len()-len(tt.out)+i))
-		}
+		t.Run("", func(t *testing.T) {
+			dest := pmetric.NewMetricSlice()


It would be more clear if in is in dest which is consistent with:

opentelemetry-mapping-go/pkg/otlp/metrics/metrics_translator.go

Line 661 in 3d07b09

md := metricsArray.At(k)

opentelemetry-mapping-go/pkg/otlp/metrics/metrics_translator.go

Line 682 in 3d07b09

remapMetrics(metricsArray, md)

songy23 · 2023-08-07T20:11:05Z

pkg/otlp/metrics/metrics_remapping.go

@@ -69,7 +69,12 @@ func remapSystemMetrics(all pmetric.MetricSlice, m pmetric.Metric) {
 	case "system.filesystem.utilization":
 		copyMetric(all, m, "system.disk.in_use", 1)
 	}
-	// process.* and system.* metrics need to be prepended with the otel.* namespace
+	if strings.HasPrefix(name, "system.") {
+		// we keep the original metric for system.* metrics


If I'm not mistaken: using the same example, now after remapMetrics we're getting 3 metrics: "otel.system.filesystem.utilization", "system.disk.in_use" and "system.filesystem.utilization"?

If so, the last one "system.filesystem.utilization" shouldn't be there

songy23 · 2023-08-08T03:18:09Z

pkg/otlp/metrics/metrics_remapping_test.go

 			},
 		},
 		{
-			in:  metric("system.filesystem.utilization", point{f: 15}),
-			out: []pmetric.Metric{metric("system.disk.in_use", point{f: 15})},


Hmmm I feel I'm getting more confused now - looks like the previous test is doing the the right thing, but how comes the resulting metric in datadog exporter got an otel. prefix? open-telemetry/opentelemetry-collector-contrib#23445 (comment)

songy23 · 2023-08-08T04:23:41Z

I finally found the root cause - the bug isn't in this file metrics_remapping.go, but in metrics_translator.go:

opentelemetry-mapping-go/pkg/otlp/metrics/metrics_translator.go

Lines 660 to 683 in 3d07b09

    
           for k := 0; k < metricsArray.Len(); k++ { 
        
           	md := metricsArray.At(k) 
        
           	if v, ok := runtimeMetricsMappings[md.Name()]; ok { 
        
           		metadata.Languages = extractLanguageTag(md.Name(), metadata.Languages) 
        
           		for _, mp := range v { 
        
           			if mp.attributes == nil { 
        
           				// duplicate runtime metrics as Datadog runtime metrics 
        
           				cp := metricsArray.AppendEmpty() 
        
           				md.CopyTo(cp) 
        
           				cp.SetName(mp.mappedName) 
        
           				break 
        
           			} 
        
           			if md.Type() == pmetric.MetricTypeSum { 
        
           				mapSumRuntimeMetricWithAttributes(md, metricsArray, mp) 
        
           			} else if md.Type() == pmetric.MetricTypeGauge { 
        
           				mapGaugeRuntimeMetricWithAttributes(md, metricsArray, mp) 
        
           			} else if md.Type() == pmetric.MetricTypeHistogram { 
        
           				mapHistogramRuntimeMetricWithAttributes(md, metricsArray, mp) 
        
           			} 
        
           		} 
        
           	} 
        
           	if t.cfg.withRemapping { 
        
           		remapMetrics(metricsArray, md) 
        
           	}

The for loop above is unfortunately an anti-pattern: you are appending more elements to metricsArray while iterating over it. For example,

say initially metricsArray == [system.filesystem.utilization]
in the first iteration, you called remapMetrics so system.disk.in_use gets appended to metricsArray, and system.filesystem.utilization gets renamed to otel.system.filesystem.utilization. Now metricsArray == [otel.system.filesystem.utilization, system.disk.in_use]. If we stop here, everything is correct
however the outer for-loop will continue, so in the second iteration, you called remapMetrics again on system.disk.in_use so it gets prepended with otel. too. Now metricsArray == [otel.system.filesystem.utilization, otel.system.disk.in_use]. The second metrics is now incorrect.

Sent out #148 to fix.

pkg/otlp/metrics: fix otel.* namespaced metrics

fc0e1f4

This change fixes a problem where sytem.* and process.* metrics were correctly namespaced with otel.*, but the original metric was not kept.

gbbr requested a review from a team as a code owner August 2, 2023 14:19

gbbr requested a review from songy23 August 2, 2023 14:19

songy23 approved these changes Aug 2, 2023

View reviewed changes

songy23 requested changes Aug 2, 2023

View reviewed changes

songy23 requested a review from liustanley August 2, 2023 15:21

Address comments

92b29c7

gbbr requested a review from songy23 August 4, 2023 12:41

songy23 reviewed Aug 4, 2023

View reviewed changes

Simplify tests

bb28975

dineshg13 reviewed Aug 7, 2023

View reviewed changes

songy23 reviewed Aug 7, 2023

View reviewed changes

songy23 reviewed Aug 8, 2023

View reviewed changes

songy23 mentioned this pull request Aug 8, 2023

pkg/otlp/metrics: fix incorrect for-loop #148

Merged

gbbr closed this Aug 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/otlp/metrics: fix otel.* namespaced metrics #142

pkg/otlp/metrics: fix otel.* namespaced metrics #142

gbbr commented Aug 2, 2023

songy23 Aug 2, 2023 •

edited

Loading

gbbr Aug 3, 2023

songy23 Aug 3, 2023

gbbr Aug 4, 2023

gbbr Aug 4, 2023

songy23 Aug 4, 2023 •

edited

Loading

gbbr Aug 7, 2023 •

edited

Loading

songy23 Aug 7, 2023

dineshg13 Aug 7, 2023

songy23 Aug 7, 2023

songy23 Aug 7, 2023

songy23 Aug 8, 2023

songy23 commented Aug 8, 2023

pkg/otlp/metrics: fix otel.* namespaced metrics #142

pkg/otlp/metrics: fix otel.* namespaced metrics #142

Conversation

gbbr commented Aug 2, 2023

songy23 Aug 2, 2023 • 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

songy23 Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

gbbr Aug 7, 2023 • 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

songy23 commented Aug 8, 2023

songy23 Aug 2, 2023 •

edited

Loading

songy23 Aug 4, 2023 •

edited

Loading

gbbr Aug 7, 2023 •

edited

Loading