groupbyattrsprocessor drops metric metadata

[braydonk]

Component(s)

processor/groupbyattrs

What happened?

Description

When groupbyattrsprocessor makes a new metric, it does not copy the metric metadata. I assume this is because Metadata is a relatively new field and isn't fully respected everywhere yet.

Steps to Reproduce

Found this by testing the new untyped metric support in Prometheus. It adds a Metadata key called prometheus.type. So that's the easiest way to see the effect. Create a pipeline from prometheusreceiver to groupbyattrsprocessor to debugexporter and have it scrape some manner of metrics.

Expected Result

The prometheus.type metadata should still be present when the value is seen in debugexporter.

Actual Result

It's gone.

Other notes

Collector version

v0.102.0

Environment information

Environment

OS: Debian 12
Compiler(if manually compiled): go 1.22.3

OpenTelemetry Collector configuration

No response

Log output

Nothing of note.

Additional context

Should there be an actual API in pdata for making full metric copies like this? What the groupbyattrsprocessor has to do here is pretty brittle for exactly this reason, and I'm not sure if there are other processors doing something similar.

[github-actions]

Pinging code owners:

• processor/groupbyattrs: @rnishtala-sumo

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[braydonk]

FYI @dashpole @ridwanmsharif

[dashpole]

There is a CopyTo function for metrics, but i'm not sure if that is what is needed here.

[braydonk]

For reference, this is the spot that does not copy metadata:

opentelemetry-collector-contrib/processor/groupbyattrsprocessor/processor.go

Lines 204 to 208 in dd2e45a

	// We're here, which means that we haven't found our metric, so we need to create a new one, with the same name and type
	metric := ilm.Metrics().AppendEmpty()
	metric.SetDescription(searchedMetric.Description())
	metric.SetName(searchedMetric.Name())
	metric.SetUnit(searchedMetric.Unit())

[rnishtala-sumo]

@braydonk looks like this could done using - https://pkg.go.dev/go.opentelemetry.io/collector/[email protected]/pmetric#Metric.CopyTo

as suggested previously. I think the ask here makes sense. Please go ahead If you'd like to make the change. I can review the changes when ready.

[crobert-1]

Removing needs triage based on code owner feedback.

[odubajDT]

If nobody is working on this issue, I would like to take it cc @evan-bradley

[odubajDT]

@braydonk looks like this could done using - https://pkg.go.dev/go.opentelemetry.io/collector/[email protected]/pmetric#Metric.CopyTo

as suggested previously. I think the ask here makes sense. Please go ahead If you'd like to make the change. I can review the changes when ready.

After some investigation, using Metric.CopyTo() function is problematic here. The function copies also the datapoints from the original metric which goes exactly against the purpose of this processor, where datapoints are moved in between metrics. Therefore when creating new metric here (with empty datapoint slice) and using the CopyTo() function with it will lead to adding datapoints which are in the input and should not be present on output.

Therefore I would suggest adding the missing CopyTo() function only for metadata here unless we want to do a major refactoring of the code of the processor.

I am opened for suggestions

[braydonk]

Thanks for taking on this issue!

Might be naive, but perhaps the datapoints could just be deleted from the copy of the metric? If that won't work then it's fine to just do the metadata copy. Just would be nice if the full metric CopyTo function would work just to avoid this kind of thing happening again.

[odubajDT]

Thanks for taking on this issue!

Might be naive, but perhaps the datapoints could just be deleted from the copy of the metric? If that won't work then it's fine to just do the metadata copy. Just would be nice if the full metric CopyTo function would work just to avoid this kind of thing happening again.

This should work, but when deleting datapoints, we still need to first find out, what type are we dealing with (sum/gauge/histogram...) and firstly then delete the appropiate datapoints (these are also different types - numeric value/ histogram value/...).

Therefore I do not see any improvement in the logic here, since still the logic determining the type for the metric needs to stay in place and instead of creating empty metrics, we will copy them and them delete parts of them.

[braydonk]

Ah okay I understand the issue now. Probably fine to just use the metadata copy then; it's probably sufficiently rare for that metric proto to change much for this kind of thing to happen again. Thanks!

[github-actions]

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

• processor/groupbyattrs: @rnishtala-sumo

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[dashpole]

Fixed by #33781