Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature][extension/encoding/otlpencoding] Encode arbitrary length of data instead of just 1 #37566

Open
constanca-m opened this issue Jan 29, 2025 · 7 comments
Labels
discussion needed Community discussion needed enhancement New feature or request extension/encoding/otlpencoding

Comments

@constanca-m
Copy link

Component(s)

extension/encoding/otlpencoding

Is your feature request related to a problem? Please describe.

Currently, the extension supports receiving just one data to unmarshall/marshall.

This is an issue if the data sent to the encoding extension contains multiple metrics/logs. For example, if using the encoding extension in a receiver, the receiver would have to have support for protocol as well, and then split the data into just one OTLP telemetry. Each of these would then be sent to the encoding extension.

Describe the solution you'd like

Since the encoding extension already has support for protocol, it would be useful if it could split the data. This way, there would be no need for redundancy when using it with other components, like the receiver example above.

Optionally, if protocol is JSON, maybe a new field delimiter would also be needed, to know where to split the data.

Describe alternatives you've considered

No response

Additional context

This issue was encountering while studying support for encoding extensions on awsfirehosereceiver (see issue). Since each record.Data inside each firehose request can contain more than one datum at each time.

@constanca-m constanca-m added enhancement New feature or request needs triage New item requiring triage labels Jan 29, 2025
Copy link
Contributor

Pinging code owners:

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

@axw
Copy link
Contributor

axw commented Jan 30, 2025

This issue was encountering while studying support for encoding extensions on awsfirehosereceiver (see issue). Since each record.Data inside each firehose request can contain more than one datum at each time.

In case it's not obvious to others, if/when the awsfirehose receiver supports the encoding extension, the proposed change here should enable using the otlpencoding extension to unmarshal metrics. As described at https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch-metric-streams-formats-opentelemetry-100.html, CloudWatch Metric Streams sends a length-delimited stream of metrics.

Maybe a solution here would be to add another protocol, like "length_delimited_proto"?

@VihasMakwana
Copy link
Contributor

This makes sense to me.

I have a question. When there are multiple metrics in a byte array (split by a separator or by length), how do we expect to concatenate them? I guess we will have one large pmetric.Metrics, with will the resource metrics combined? Please correct me if I'm wrong.

@VihasMakwana VihasMakwana added discussion needed Community discussion needed and removed needs triage New item requiring triage labels Jan 30, 2025
@constanca-m
Copy link
Author

constanca-m commented Jan 30, 2025

Using Merge metrics function. A bit like:

func (ex *otlpExtension) UnmarshalMetrics(buf []byte) (pmetric.Metrics, error) {
       <declare allMetrics variable>
        < for each metric> 
	  metrics = ex.metricUnmarshaler.UnmarshalMetrics(buf)
          allMetrics = Merge(pmetric.NewMetrics(), metrics)
        < done>
       return allMetrics
}

Or if it is too expensive to be merging in every for iteration, we could try to find a solution that involves only doing it at the end. Since the UnmarshalMetrics only has one metric (correct?), then we could be appending them all, before merging at the end.

@VihasMakwana
Copy link
Contributor

That is precisely what I thought. I had no idea such function existed (thanks!).

Or if it is too expensive to be merging in every for iteration

I think it will be okay.

@axw
Copy link
Contributor

axw commented Jan 31, 2025

IMO merging should be left to another component, and the unmarshaller should just append to the same pmetric.Metrics like in the Firehose receiver's internal unmarshaller:

req.Metrics().ResourceMetrics().MoveAndAppendTo(md.ResourceMetrics())

@VihasMakwana
Copy link
Contributor

I agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion needed Community discussion needed enhancement New feature or request extension/encoding/otlpencoding
Projects
None yet
Development

No branches or pull requests

3 participants