Add helper functions for metric conversion [awsecscontainermetricsreceiver] #1089

hossain-rayhan · 2020-09-22T05:25:09Z

Description:
This change adds helper functions for converting ECS resources metrics to OT metrics.

Link to tracking Issue:
#457

Testing:
Unit test added.

Documentation:
README.md

codecov · 2020-09-22T05:40:45Z

Codecov Report

Merging #1089 into master will increase coverage by 0.15%.
The diff coverage is 99.11%.

@@            Coverage Diff             @@
##           master    #1089      +/-   ##
==========================================
+ Coverage   88.84%   89.00%   +0.15%     
==========================================
  Files         251      254       +3     
  Lines       11979    12161     +182     
==========================================
+ Hits        10643    10824     +181     
+ Misses        992      991       -1     
- Partials      344      346       +2

Flag	Coverage Δ
#integration	`75.42% <ø> (-0.11%)`	⬇️
#unit	`88.04% <99.11%> (+0.17%)`	⬆️

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

Impacted Files	Coverage Δ
...sreceiver/awsecscontainermetrics/metrics_helper.go	`97.36% <97.36%> (-2.64%)`	⬇️
...ricsreceiver/awsecscontainermetrics/accumulator.go	`100.00% <100.00%> (ø)`
...metricsreceiver/awsecscontainermetrics/resource.go	`100.00% <100.00%> (ø)`
...tricsreceiver/awsecscontainermetrics/translator.go	`100.00% <100.00%> (ø)`
...nermetricsreceiver/awsecscontainermetrics/utils.go	`100.00% <100.00%> (ø)`
receiver/k8sclusterreceiver/watcher.go	`95.29% <0.00%> (-2.36%)`	⬇️
... and 2 more

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 6d6d534...91dac3d. Read the comment docs.

asuresh4 · 2020-09-22T19:53:26Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/utils.go

+)
+
+// GenerateDummyMetrics generates two dummy metrics
+func GenerateDummyMetrics() consumerdata.MetricsData {


Can this method be moved to a test file? Same with createGaugeIntMetric below.

That's a good idea. Planing to totally remove it in our next PR when we will get our original Metrics generation code. Keeping it for now as its being used by previous code. Added a TODO note on top of it.

asuresh4 · 2020-09-22T20:11:54Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/constant.go

+	ContainerPrefix                   = "container."
+	ResourceAttributeServiceNameKey   = "service.name"
+	ResourceAttributeServiceNameValue = "awsecscontainermetrics"
+	MetricResourceType                = "aoc.ecs"


Just curious, what does aoc stand for?

AWS Observability Collector-> Amazon distribution of OpenTelemetry.

FWIW, resource type doesn't exist in OTel protocol but is there right now for metrics since it still seems to use opencensus. So this value will generally be dropped

Noted. Even if it gets utilized we want to use "aoc.ecs" to differentiate our OT metrics from ECS backend metrics.

I point it out because it will go away - so if you do have an expectation of having it, it won't be there :P But I think if we put the receiver name in something like telemetry.sdk than the information should still be preserved, in a way that matches in some sense what our apps send.

asuresh4 · 2020-09-22T20:18:25Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/accumulator.go

+		containerMetrics.MemoryReserved = *containerMetadata.Limits.Memory
+		containerMetrics.CPUReserved = *containerMetadata.Limits.CPU
+
+		taskMemLimit += containerMetrics.MemoryReserved


Looks like these values aren't being used anywhere?

asuresh4 · 2020-09-22T20:34:18Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/accumulator.go

+}
+
+func (acc *metricDataAccumulator) accumulate(
+	startTime *timestamp.Timestamp,


From usages of accumulate method, I think the first parameter being passed in is the timestamp of a reporting interval? This should instead be the time when a cumulative metric was reset to 0. Sere comment here for details.

Yea. I looked into the dockerstatsreceiver and it's not setting the value I think. We do have PullStartedAt timestamp and I think we can utilize it. However, need more thoughts on this. For now, not setting this value to get the default behavior. Will send a separate PR for this.

asuresh4 · 2020-09-22T20:45:51Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/accumulator.go

+	resourceAttributes[ResourceAttributeServiceNameKey] = ResourceAttributeServiceNameValue
+
+	r := &resourcepb.Resource{
+		Type:   MetricResourceType,


Looks like this same type is being used for all resources from which metrics are being collected, both containers and tasks. Container metrics should be associated with a container resource and similarly a task resource for task metrics.

As @anuraaga mentioned, this will be dropped eventually. However, if it does get utilized, we prefer to use aoc.ecs for all metrics we are receiving from this receiver.

asuresh4 · 2020-09-22T20:48:51Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/accumulator.go

+		taskMemLimit += containerMetrics.MemoryReserved
+		taskCPULimit += containerMetrics.CPUReserved
+
+		labelKeys, labelValues := containerLabelKeysAndValues(containerMetadata)


Labels/Attributes that describe a resource (container/task) should be collected as attributes on the resource object. Same for labels collected by taskLabelKeysAndValues.

Hi @asuresh4 , I am little bit confused here. Can you explain a bit more? These labels describes the properties to differentiate the metrics. Then when/what should I set as metric labels. Also, these labels are supposed to be converted into metric dimensions.

The kubelet_stats receiver should be good example to understand the concept. It collects metrics from different types of resources (containers, pods, nodes). The properties of resources are added as labels on the resource. An exporter exporting the metric would treat labels from the resource and the metric as dimensions.

Thanks. Added those as Resource Attributes.

asuresh4 · 2020-09-22T21:10:20Z

receiver/awsecscontainermetricsreceiver/go.mod

 	github.com/stretchr/testify v1.6.1
 	go.opentelemetry.io/collector v0.10.1-0.20200917170114-639b9a80ed46
 	go.uber.org/zap v1.16.0
+	google.golang.org/protobuf v1.25.0


Looks like these are from importinggitpro.ttaallkk.top/golang/protobuf/ptypes/timestamp and google.golang.org/protobuf/types/known/timestamppb. Do you need both? Could probably just use timestamppb in both places.

anuraaga

Left some flyby comments, but basically LGTM with @asuresh4's comments, thanks.

anuraaga · 2020-09-23T08:44:11Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/accumulator.go

+	"time"
+
+	metricspb "github.com/census-instrumentation/opencensus-proto/gen-go/metrics/v1"
+	resourcepb "github.com/census-instrumentation/opencensus-proto/gen-go/resource/v1"


@asuresh4 @bogdandrutu Are metrics receivers still using opencensus proto?

No, we changed most of the core to use the otlp and internal structs. Completely recommend for new components to avoid oc.

@hossain-rayhan you need to start using pdata.Metrics

Hi @bogdandrutu , before sending the data to next consumer I am using internaldata.OCToMetrics(md) to convert our metrics to pdata.Metrics. Wondering, isn't that enough like other receivers in the repo or we should strictly get rid of it now?

That is a temporary solution to make progress and not have to change all components once. And decided to use that for some old components that we did not have time to chnage

@hossain-rayhan Yeah so basically you should only be converting at the last moment when passing down, but here in this sort of receiver-specific logic we want to be using pdata, the OTel format. Or we just have to rewrite it right away. We're also having data-model issues because of using the old format (Resource type for example) and we want to make sure the model is right

Thanks @bogdandrutu and @anuraaga. I understand we need to use pdata to convert everything to OTel format eventually. I was planning to move forward with this to meet our internal deadline (9/30/2020). We can send a different PR after October 15th I guess. How do you guys feel about it?

As long as you create an issue and assign to you and @anuraaga I am fine. I trust that you will fix it. I will let @anuraaga make the final call here.

Issue created: #1122

anuraaga · 2020-09-23T08:46:04Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/constant.go

+	ContainerPrefix                   = "container."
+	ResourceAttributeServiceNameKey   = "service.name"
+	ResourceAttributeServiceNameValue = "awsecscontainermetrics"
+	MetricResourceType                = "aoc.ecs"


FWIW, resource type doesn't exist in OTel protocol but is there right now for metrics since it still seems to use opencensus. So this value will generally be dropped

anuraaga · 2020-09-23T08:46:54Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/constant.go

+	TaskPrefix                        = "ecs.task."
+	ContainerPrefix                   = "container."
+	ResourceAttributeServiceNameKey   = "service.name"
+	ResourceAttributeServiceNameValue = "awsecscontainermetrics"


The service name corresponds with an application, not a backend, so for example AuthService, SearchFrontend, etc. We could fill this in with the ECS service name, or otherwise we shouldn't fill it since this isn't the correct semantics

Yea, got it. But, for all of our AWS receivers, we are using the receiver name as service.name. Because, this field will be utilized by our CW EMFExporter to generate different rules for different receivers. Especially for Container Insights.

What do you mean by AWS receivers? I think the only one we have is xray, which doesn't do this, and definitely shouldn't since we need to make sure the app's service name is used.

I'm not sure what you mean exactly by the rules, but anyways we can't just fill a semantic convention attribute with something that doesn't follow the spec. If anything, the telemetry.sdk matches closer to what this sort of receiver is doing. @bogdandrutu @tigrannajaryan any suggestion on that?

Also @hossain-rayhan it's important to take a step back and remember what this receiver is here for - it's to translate the container metrics data into the OpenTelemetry format / specification. This is because this data seems useful to users regardless of if they use cloudwatch or not. While we may need some, but hopefully not much, consideration for specific vendors like cloudwatch, that's not the intent here. If you haven't yet, you should go through in detail at least the Resource and Metrics semantics conventions of OTel spec before proceeding and make sure you are aligned with it https://github.com/open-telemetry/opentelemetry-specification/tree/master/specification/resource. That doesn't mean we want to block data that's needed, but it's important to follow the spec as much as possible.

This receiver generates ECS container Metrics itself but not receiving any metrics from outside of OTel Collector. For the metrics generated inside the receiver, the idea is to put receiver name in service.name attribute on these metrics. It's similar to the idea Prometheus receiver uses job_name as service.name for metrics it scrapes.

I'm not sure what you mean exactly by the rules, but anyways we can't just fill a semantic convention attribute with something that doesn't follow the spec. If anything, the telemetry.sdk matches closer to what this sort of receiver is doing. @bogdandrutu @tigrannajaryan any suggestion on that?

+1. We should not use "service.name" for receiver name. That is not the purpose of "service.name". "service.name" is supposed to describe the source that emits the metrics. Collector is just collecting the metric, it is an intermediary, it is not the source. Nor is "telemetry.sdk" intended for that.

The source that emits the metrics is the container here. If we know the name of the service that runs in the container we should set that. If we don't know we should not record it at all.

I do not know why we want to record the receiver name, perhaps you can clarify the use case. This can then be added as a semantic convention for OpenTelemetry as a whole or just for the Collector and will possibly end up in the "otel" namespace.

@hossain-rayhan If you can give some more detail about this usage that would be great. I think filling in wrong information is blocking this PR, so the easiest way to proceed would be to just remove setting the service name for now and we can figure out a way to handle what you need in a separate PR.

Thanks @anuraaga . I am removing it for now as this should not block the receiver. This is more related to the exporter logic as it's being utilized for supporting special customer use cases. If needed I can support it in a separate PR after further discussion.

anuraaga · 2020-09-23T08:47:59Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/translator.go

+	"google.golang.org/protobuf/types/known/timestamppb"
+)
+
+func convertToOTMetrics(prefix string, m ECSMetrics, labelKeys []*metricspb.LabelKey, labelValues []*metricspb.LabelValue, timestamp *timestamppb.Timestamp) []*metricspb.Metric {


Suggested change

func convertToOTMetrics(prefix string, m ECSMetrics, labelKeys []*metricspb.LabelKey, labelValues []*metricspb.LabelValue, timestamp *timestamppb.Timestamp) []*metricspb.Metric {

func convertToOCMetrics(prefix string, m ECSMetrics, labelKeys []*metricspb.LabelKey, labelValues []*metricspb.LabelValue, timestamp *timestamppb.Timestamp) []*metricspb.Metric {

We're actually converting to opencensus metrics here

hossain-rayhan · 2020-09-23T19:05:18Z

Thanks @asuresh4 and @anuraaga thanks for your review. I pushed an update based on the review. Will appreciate another look.

anuraaga · 2020-09-24T05:02:03Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/accumulator.go

+	"time"
+
+	metricspb "github.com/census-instrumentation/opencensus-proto/gen-go/metrics/v1"
+	resourcepb "github.com/census-instrumentation/opencensus-proto/gen-go/resource/v1"


@hossain-rayhan Yeah so basically you should only be converting at the last moment when passing down, but here in this sort of receiver-specific logic we want to be using pdata, the OTel format. Or we just have to rewrite it right away. We're also having data-model issues because of using the old format (Resource type for example) and we want to make sure the model is right

anuraaga · 2020-09-24T05:03:47Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/constant.go

+
+	TaskPrefix                        = "ecs.task."
+	ContainerPrefix                   = "container."
+	ResourceAttributeServiceNameKey   = "service.name"


Use convention

https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/master/receiver/awsxrayreceiver/internal/translator/aws.go#L21

How about this? conventions.AttributeServiceName

anuraaga · 2020-09-24T05:06:04Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/constant.go

+	BytesInMiB = 1024 * 1024
+
+	TaskPrefix                        = "ecs.task."
+	ContainerPrefix                   = "container."


Why do we need to prefix the metrics with container? If they have container label, they're container metrics right?

I'm not sure if this part of the OTel convention but given that other receivers follow this approach, I think we should do the same here for consistency.

Yea, I found some other receivers are doing the same like kubeletstatsreceiver and dockerstatsreceiver.

anuraaga · 2020-09-24T05:12:35Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/constant.go

+	TaskPrefix                        = "ecs.task."
+	ContainerPrefix                   = "container."
+	ResourceAttributeServiceNameKey   = "service.name"
+	ResourceAttributeServiceNameValue = "awsecscontainermetrics"


What do you mean by AWS receivers? I think the only one we have is xray, which doesn't do this, and definitely shouldn't since we need to make sure the app's service name is used.

I'm not sure what you mean exactly by the rules, but anyways we can't just fill a semantic convention attribute with something that doesn't follow the spec. If anything, the telemetry.sdk matches closer to what this sort of receiver is doing. @bogdandrutu @tigrannajaryan any suggestion on that?

Also @hossain-rayhan it's important to take a step back and remember what this receiver is here for - it's to translate the container metrics data into the OpenTelemetry format / specification. This is because this data seems useful to users regardless of if they use cloudwatch or not. While we may need some, but hopefully not much, consideration for specific vendors like cloudwatch, that's not the intent here. If you haven't yet, you should go through in detail at least the Resource and Metrics semantics conventions of OTel spec before proceeding and make sure you are aligned with it https://github.com/open-telemetry/opentelemetry-specification/tree/master/specification/resource. That doesn't mean we want to block data that's needed, but it's important to follow the spec as much as possible.

anuraaga · 2020-09-24T05:14:43Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/constant.go

+	ContainerPrefix                   = "container."
+	ResourceAttributeServiceNameKey   = "service.name"
+	ResourceAttributeServiceNameValue = "awsecscontainermetrics"
+	MetricResourceType                = "aoc.ecs"


I point it out because it will go away - so if you do have an expectation of having it, it won't be there :P But I think if we put the receiver name in something like telemetry.sdk than the information should still be preserved, in a way that matches in some sense what our apps send.

anuraaga

Thanks!

hossain-rayhan · 2020-09-25T16:26:55Z

Hi @bogdandrutu @tigrannajaryan @asuresh4 can we get this merged?

asuresh4 · 2020-09-25T16:54:15Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/accumulator.go

+
+	taskMetrics := ECSMetrics{}
+	timestamp := timestampProto(time.Now())
+	taskResources := taskResources(metadata)


nit: taskResource would be more accurate. Same with containerResources (-> containerResource)

asuresh4 · 2020-09-25T16:55:20Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/accumulator.go

+
+	acc.accumulate(
+		taskResources,
+		convertToOCMetrics(TaskPrefix, taskMetrics, nil, nil, timestamp),


If the 3rd and 4th parameters to this method are always nil, I would remove those parameters.

I also thought about it while writing this piece of code. Here, I kept the skeleton ready and the same method can be utilized to set metric labels. In our next PRs, we can just pass the LabelKeys and LabelValues and we are done. If we really don't utilize, I will remove them.

asuresh4

LGTM, apart from couple of minor comments.

tigrannajaryan · 2020-09-25T17:19:33Z

I'll merge once the last comments from @asuresh4 are addressed.

* Split out processor READMEs * Split out exporter READMEs * Split out extension READMEs * Split out receiver READMEs * Add new line at end of READMEs

* Prepare for releasing v0.11.0 * Update CHANGELOG.md to reflect scope of v0.11.0 release * Update CHANGELOG.md Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

* Use a shorter timeout for AWS EC2 metadata requests Fix #1088 According to the docs, the value for `timeout` is in seconds: https://docs.python.org/3/library/urllib.request.html#urllib.request.urlopen. 1000 seconds seems slow and in some cases can block the startup of the program being instrumented (see #1088 as an example), because the request will hang indefinitely in non-AWS environments. Using a much shorter 1 second timeout seems like a reasonable workaround for this. * add changelog entry for timeout change * use 5s timeout for ECS and EKS, update changelog Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>

Add helper functions for metric conversion

ba6ee8b

hossain-rayhan requested a review from a team September 22, 2020 05:25

Add metrics accumulator to gather metrics from endpoint response

8d991e3

tigrannajaryan assigned asuresh4 Sep 22, 2020

asuresh4 reviewed Sep 22, 2020

View reviewed changes

anuraaga reviewed Sep 23, 2020

View reviewed changes

Use label as Resource Attributes and address PR feedbacks

88cd8ca

Add a TODO for removing GenerateDummyMetrics()

d6de8ca

anuraaga reviewed Sep 24, 2020

View reviewed changes

hossain-rayhan mentioned this pull request Sep 24, 2020

Use pdata.Metrics instead of OpenCensus Metrics in awsecscontainermetricsreceiver #1122

Closed

Remove misleading 'service.name' resource attribute for now

0e8cba2

anuraaga approved these changes Sep 25, 2020

View reviewed changes

asuresh4 reviewed Sep 25, 2020

View reviewed changes

asuresh4 approved these changes Sep 25, 2020

View reviewed changes

Rename func and variables to address PR feedback

91dac3d

hossain-rayhan force-pushed the helper branch from cf75a60 to 91dac3d Compare September 25, 2020 17:50

tigrannajaryan merged commit ca0feff into open-telemetry:master Sep 25, 2020

codeboten pushed a commit that referenced this pull request Nov 23, 2022

Comment out botocore test (#1089)

c26f783

codeboten pushed a commit that referenced this pull request Nov 23, 2022

Comment out botocore test (#1089)

5cdb626

	func convertToOTMetrics(prefix string, m ECSMetrics, labelKeys []metricspb.LabelKey, labelValues []metricspb.LabelValue, timestamp timestamppb.Timestamp) []metricspb.Metric {
	func convertToOCMetrics(prefix string, m ECSMetrics, labelKeys []metricspb.LabelKey, labelValues []metricspb.LabelValue, timestamp timestamppb.Timestamp) []metricspb.Metric {

Add helper functions for metric conversion [awsecscontainermetricsreceiver] #1089

Add helper functions for metric conversion [awsecscontainermetricsreceiver] #1089

Conversation

hossain-rayhan commented Sep 22, 2020

codecov bot commented Sep 22, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

hossain-rayhan Sep 22, 2020 • 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

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

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

anuraaga 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

Choose a reason for hiding this comment

hossain-rayhan Sep 24, 2020 • 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

Choose a reason for hiding this comment

mxiamxia Sep 24, 2020 • 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

Choose a reason for hiding this comment

hossain-rayhan commented Sep 23, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

hossain-rayhan commented Sep 25, 2020

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

asuresh4 left a comment

Choose a reason for hiding this comment

tigrannajaryan commented Sep 25, 2020

codecov bot commented Sep 22, 2020 •

edited

Loading

hossain-rayhan Sep 22, 2020 •

edited

Loading

hossain-rayhan Sep 24, 2020 •

edited

Loading

mxiamxia Sep 24, 2020 •

edited

Loading