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 request] [exporter] resource_attributes_to_metric_labels - common utility for all Non-OTLP exporters #1359

Closed
hossain-rayhan opened this issue Oct 23, 2020 · 14 comments
Labels
enhancement New feature or request

Comments

@hossain-rayhan
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Customer needs to convert resource attributes to metric labels and its a pretty common use case. In Wednesdays (10/22) OpenTelemetry Metrics SIG meeting, we took a decision to implement a common utility for all Non-OTLP exporters which will convert all resource attributes to metric labels by default. If customers need to exclude any resource attributes, they can use the resource processor.

Describe the solution you'd like
@bogdandrutu will help to provide detailed guidelines.

Describe alternatives you've considered
Do it in the metricstransform processor- PR [#1337 ].
Do it in the receiver- PR [#1284 ].

@hossain-rayhan
Copy link
Contributor Author

@bogdandrutu would you add your recommendation (we discussed in the SIG meeting) here?

@lubingfeng
Copy link

@bogdandrutu We would like to get this completed by middle November. Appreciate your help in advance.

@bogdandrutu
Copy link
Member

So we have 2 problems if I understand correctly:

  1. Manipulating the Resource.attributes
  2. Some of the exporters need to convert Resources into labels.
    • I suggest we implement a consumer.[Trace|Metrics|Logs]Consumer (for traces will be Span.attributes, for logs will be LogRecord.Attributes, for metrics will be lables) that has this capability, and it will be possible to plugin this in the exporterhelper.

We can start with a consumer that transforms all the Resource.attributes into labels, example given only for Metrics, but should support all signals:

type ResourceToLabels struct {
  next consumer.MetricsConsumer
}

func (rl *ResourceToLabels) ConsumeMetrics(_ context.Context, md pdata.Metrics) error {
        newMd := pdata.NewMetrics()
	// initialize newMd with 1 resource (empty), 1 library metrics (empty), and md.MetricsCount() metrics
	num := 0
	rms := td.ResourceMetrics()
	for i := 0; i < rms.Len(); i++ {
		resource := rms.At(i).Resource()
		if resource.IsNil() {
			continue
		}
		ilms := rss.At(i).InstrumentationLibraryMetrics()
		for j := 0; j < ilms.Len(); j++ {
			ilm := ilms.At(j)
			if ilm.IsNil() {
				continue
			}
			metrics := ils.Metrics()
			library := ils.InstrumentationLibrary()
			for k := 0; k < metrics.Len(); k++ {
				metric := metrics.At(k)
				if metric.IsNil() {
					continue
				}
				resourceToLables(resource.Attributes(), metric, dest.At(num))
				num++
			}
		}
	}
	return rl.next(md)
}

This approach as you can see does require a copy of the metrics, because exporters are not allowed to change the incoming metrics data.

An alternative is to provide just a helper func in the exporterhelper

resourceAndLibraryToLables(resource pdata.Resource, instrumentationInfo pdata.InstrumentationLibrary, metric pdata.Metric) pdata.Metric {
  // Returns a copy of the pdata.Metric with all new added labels
  // Again important to notice that needs to be a copy because the exporter cannot change metric data.
}

Happy to explore more ideas.

@hossain-rayhan
Copy link
Contributor Author

Hi @bogdandrutu , thanks for the quick suggestions. If I understand correctly, you described two different approaches. I am exploring them. Do you have any preference between these two?

Also, I saw a comment from @tigrannajaryan in gitter, regarding converting all the resource attributes to labels (to many labels). Are we still on plan to convert all the resource attributes to labels by default.

Screen Shot 2020-10-26 at 10 31 07 AM

@tigrannajaryan
Copy link
Member

Some of the exporters need to convert Resources into labels.

@bogdandrutu To add to this. I think all exporters that do not use a format that natively supports a Resource-equivalent concept have to do this. I wonder if my assertion is correct or there need to be exceptions or this behavior needs to be configurable per exporter.

@bogdandrutu
Copy link
Member

@bogdandrutu To add to this. I think all exporters that do not use a format that natively supports a Resource-equivalent concept have to do this. I wonder if my assertion is correct or there need to be exceptions or this behavior needs to be configurable per exporter.

OTLP and Jaeger don't need this (Jaeger has process tags). But in general yes all of them except these two.

@hossain-rayhan
Copy link
Contributor Author

hossain-rayhan commented Oct 28, 2020

Working on this. Maybe you can assign this to me.

@mxiamxia
Copy link
Member

@bogdandrutu To add to this. I think all exporters that do not use a format that natively supports a Resource-equivalent concept have to do this. I wonder if my assertion is correct or there need to be exceptions or this behavior needs to be configurable per exporter.

OTLP and Jaeger don't need this (Jaeger has process tags). But in general yes all of them except these two.

I think we should make it configurable. In CloudWatch Metrics Exporter, we only need to add resource labels for ECS Metrics (@hossain-rayhan is working on in this thread) but for the other use cases like customer application metrics, we don't want to enable it by default. CloudWatch only allows maximum 10 dimensions/labels for each metric. We like it to be able to be configured for combining. :)

@tigrannajaryan
Copy link
Member

CloudWatch only allows maximum 10 dimensions/labels for each metric.

Given that the limitations are specific to destinations we likely need this to be a user-definable configuration option for exporters, possibly also maximum limits enforced by each exporter where such limits exist.

@hossain-rayhan
Copy link
Contributor Author

I think we should make it configurable. In CloudWatch Metrics Exporter, we only need to add resource labels for ECS Metrics (@hossain-rayhan is working on in this thread) but for the other use cases like customer application metrics, we don't want to enable it by default.

As I mentioned in the PR description, we had a discussion in our first conversation:
In Wednesdays (10/22) OpenTelemetry Metrics SIG meeting, we (@bogdandrutu and others) got a suggestion to implement a common utility for all Non-OTLP exporters which will convert all resource attributes to metric labels by default. If customers need to exclude any resource attributes, they can use the resourceprocessor.

From yesterdays (10/29) SIG meeting, this feature can be an opt-in option for exporters or maybe we can make if default (not finalized yet). However, I feel like exporter specific logics (limitation to labels/dimensions) should go under exporter configuration. Maybe @bogdandrutu can add more.

@jmacd
Copy link
Contributor

jmacd commented Oct 30, 2020

I believe this has reached the level of a specification-level problem. We agree that generally resource attributes should ideally be preserved somehow when exporting metrics, and we acknowledge that: (1) some protocols have a place for resources, which is nice, and (2) some services have a limit on the number of metric attributes (labels) making it infeasible to recommend that, by default, all resources automatically become metric attributes (labels).

In metrics systems, it is common to apply resource labels downstream. For example, Prometheus applies target labels associated with metadata from scrape events, and Statsd packets are often aggregated inside an agent with defined resources. In these cases, we do not want to apply resources that will be applied later. However, if using bare statsd to a remote endpoint, resources should be included.

Thus, "all resources" is not a good default, but "no resources" is also not a good default. Note that Prometheus just encourages configurability here: Prometheus relabel_rules are how the user decides which resources become labels and which are dropped. We probably want the same, both inside SDKs and inside the Collector, but should we try to settle on a default that is somewhere between "all resources" and "no resources"? This seems contentious.

@hossain-rayhan
Copy link
Contributor Author

hossain-rayhan commented Oct 30, 2020

@jmacd thanks for some good points. Reading all these comments, wondering would it be a good idea to have another look to our previous proposed solution (#1337) which extends the metricstransform processor?

It does not modify existing resource attributes rather adds some new metric labels (values are taken from resources). Also, user will have the control to choose which resource attributes they want to convert to metric labels.

I don't know if it goes totally against our specification.

Or, should we consider writing a whole new processor (resourcetometricsattributesprocessor) only for this purpose (as @tigrannajaryan 's suggestion.

@hossain-rayhan
Copy link
Contributor Author

Also, to be more specific I am listing all the available options to us-

[1] Extend metricstransform processor which creates new metric labels from selected resource attributes (#1337). This does not modify any resource attributes.

[2] Write a whole new processor (resourcetometricsattributes) to convert some selected resource attributes to metrics attributes (labels). This also does not modify any resource attributes.

[3] Write a common utility for all exporters which converts all the resource attributes to metric labels by default. (currently following this one)

[4] Write a common utility for all exporters which converts all the resource attributes to metric labels and has the option to select specific resource attributes.

[5] Put it in the exporter. Every exporter should decide how they want to make it configureable. one single setting also may help for default option (convert_resource_to_labels=true).

I guess these are good options to us and we need to pick one.

@hossain-rayhan
Copy link
Contributor Author

Addressed by this PR 2060

@andrewhsu andrewhsu added the enhancement New feature or request label Jan 6, 2021
dyladan referenced this issue in dynatrace-oss-contrib/opentelemetry-collector-contrib Jan 29, 2021
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
ljmsc referenced this issue in ljmsc/opentelemetry-collector-contrib Feb 21, 2022
And update .gitignore, so the built binary does not show up in git
status.
codeboten pushed a commit that referenced this issue Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants