Add test data and data types for `awsecscontainermetricsreceiver` #1044

hossain-rayhan · 2020-09-17T01:02:25Z

Description:
This change adds metadata and types to support metric conversion logic for awsecscontainermetricsreceiver. The receiver gets the task metadata endpoint response and generates OT metrics from that.

Linking to existing issue
#457

Testing: Unit tests added.

Documentation: README file

codecov · 2020-09-17T01:15:42Z

Codecov Report

Merging #1044 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1044      +/-   ##
==========================================
+ Coverage   88.81%   88.85%   +0.04%     
==========================================
  Files         250      251       +1     
  Lines       11924    11952      +28     
==========================================
+ Hits        10590    10620      +30     
+ Misses        990      989       -1     
+ Partials      344      343       -1

Flag	Coverage Δ
#integration	`75.42% <ø> (ø)`
#unit	`87.87% <100.00%> (+0.04%)`	⬆️

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

Impacted Files	Coverage Δ
...nermetricsreceiver/awsecscontainermetrics/label.go	`100.00% <100.00%> (ø)`
receiver/k8sclusterreceiver/watcher.go	`97.64% <0.00%> (+2.35%)`	⬆️

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 bff19a2...d7ba9d5. Read the comment docs.

PettitWesley · 2020-09-17T01:28:39Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/docker_stats.go

+// ContainerStats defines the structure for container stats
+type ContainerStats struct {


Is this and the other structs for parsing the response from ECS Task Metadata?

If so... would it be a bad idea to import the structs from the ECS Agent? The ECS Agent is in Go and uses go code to generate the output that you are processing.

I guess importing that code might not be wise... ECS Agent isn't meant to be used as a library. Still you could add a link to those structures as a reference. Or a link to the ECS Task Metadata docs explaining where all of this comes from.

I think its here and here:

https://github.com/aws/amazon-ecs-agent/blob/master/agent/handlers/v4/stats_response.go

https://github.com/aws/amazon-ecs-agent/blob/master/agent/containermetadata/types.go

Looks like they actually mostly just import from Docker though... would it be a good idea to import the docker structs?

Thanks for noticing this! Agree to not import from a package not meant for consumption as a library, the ECS agent. But do definitely reuse docker structs where you can that's nice.

Thats a good idea. However, our docker-stats json structure is little bit different. It has an extra item (block) for network rate metrics which is added by ECS agent. In future, we will have another block of data for storage read/write metrics. So, creating a simple struct with the fields we care about only might be a straight-forward solution.

"networkspersec": { "eth0": [ { "timestamp": "2020-06-17T21:45:26.381091349Z", "rx_bytes_per_sec": 20, "tx_bytes_per_sec": 25 }, { "timestamp": "2020-06-17T21:45:26.481091349Z", "rx_bytes_per_sec": 10, "tx_bytes_per_sec": 55 } ] }

Just to confirm @hossain-rayhan you don't intend to import from the docker api types? I see most of these are pointers, which docker doesn't use, so I understand coercing them to match your usage may be undesired in the long run.

Also, if these are largely taken from the ECS agent or related lib, some attribution would be helpful/appreciated.

anuraaga

Aside from issue of sharing structs with upstream libraries where possible and one about leaving out labels from this receiver, generally looks fine to me. Thanks!

anuraaga · 2020-09-17T03:54:06Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/label.go

+	metricspb "github.com/census-instrumentation/opencensus-proto/gen-go/metrics/v1"
+)
+
+func containerLabelKeysAndValues(cm ContainerMetadata) ([]*metricspb.LabelKey, []*metricspb.LabelValue) {


Do we need to add labels in the receiver? We usually add resource information in a processor, such as here

https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/master/processor/k8sprocessor/kube/kube.go#L104

I think this would allow the same resource information to be added both to these metrics, and any other metrics / traces received by the collector.

This is for ECS, not k8s.

Also, while Task labels could be added by a ECS Metadata processor, I am not sure container labels could be, since by the time you reach the processor you lose information about which container is which (unless you add labels that identify the container, in which case, you might as well just add all of them in the receiver directly).

Also, another point- I am not sure the receiver can spit out container specific metrics without labels that identify which container they came from anyway. If you have 3 containers, and collect metrics for each, but don't differentiate them with something, then you are actually doing an aggregation across all the containers. At least that is my understanding.

I'll finally add that we've discussed this a bunch internally, and I think Rayhan has discussed it a bit externally as well. This implementation has been thought out.

I feel like we should keep them as close as to the source (producer). Also, it will help to keep the config simple for customers. Customer will configure the receiver and good to go.

I saw some other receivers did the same (like kubelestatsreceiver, radisreceiver and statsdreceiver).

This is for ECS, not k8s.

Yeah but k8s is an example with a presumably very similar pattern :)

Also, while Task labels could be added by a ECS Metadata processor

From my understanding, it's not a could be but a when will it be - aren't we planning on adding an ECS processor so the information is added to data published by the application as well to allow correlation between ecs metrics and app metrics?

container labels

Understood on the container labels - they do seem important to add in the receiver for those reasons. So yeah, the worst case is processor is also enabled and a small amount of extra overhead populating task information twice, but not a big deal.

I'll finally add that we've discussed this a bunch internally, and I think Rayhan has discussed it a bit externally as well. This implementation has been thought out.

This isn't helpful because

Internal discussions don't provide value when working in OSS. Going forward, it'll be good to move all related design discussions to GitHub or public Slack or gitter as well to faciliate AWS involvement in OpenTelemetry or other OSS

An implementation being thought out doesn't mean rubberstamped reviews or allow avoiding probing questions during reviews. It's what code reviews are there for

This isn't helpful because

Fair point 👍

it's not a could be but a when will it be - aren't we planning on adding an ECS processor so the information is added to data published by the application as well to allow correlation between ecs metrics and app metrics?

I'm still confused by that actually. I heard the plan is that the OT SDKs will add the metadata themselves, and so there's not a huge need for the processor. I was told that doing it in the SDK is better- from within the container a call to Task Metadata can tell you which container you are a part of, so you can add the container specific labels. A collector processor would have trouble with that since once it reaches the collector you potentially lose the knowledge of which container in the task the application is.

I was told though that there might be some value in having a processor for other types of metrics/traces not from the OT SDKs. Like OpenCensus or Prometheus, since those won't have a built-in integration with ECS Metadata.

So yeah, the worst case is processor is also enabled and a small amount of extra overhead populating task information twice, but not a big deal.

I agree. Let's start with the minimum (putting them into the receiver for now). If we feel a strong need to add a processor, we can do that later.

Yeah unfortunately the collector processor can't add docker container labels since the OTel collector is in a different container. But as long as its a sidecar it should be able to add the rest of the ECS labels. The current state is that the SDKs add container labels, which are standard since they're just docker - this can be provided by OTel in a generic way.

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/resource/semantic_conventions/container.md

They don't provide ECS-specific labels, mainly since they're not defined in the spec and aren't an SDK concern. We can add SDK extensions for ECS - and in the very long term, it makes sense to have SDK plugins for ECS labels too to support running the collector as a service instead of a sidecar. We see in practice that there is overlap between resource detection logic in the collector, like for GCP/EC2, and what's in the SDKs, primarily since collector as a sidecar provides an approach for underdeveloped or future languages while SDK implementations are more flexible in terms of deployment.

In the shorter term, if we restrict the conversation to sidecar as a first iteration, then implementing ECS labels as a processor would allow using from any app regardless of language, until ECS plugins could be added for each language too. For example, because Java and JS at least already populate the container info, if the collector has a processor for ECS tasks and above granularity, both languages would get the full set of ECS metadata automatically. So I think it makes sense to have a processor as the first implementation of ECS task metadata enrichment.

Hopefully that provides some overall context - but to avoid confusion, it's indeed unrelated to this PR as I realized that indeed it makes sense to additionally add them in the receiver :)

I went through dockerstat receiver. They are adding labels to the metrics for each container: close to the source as mentioned by @hossain-rayhan

ContainerStatsToMetrics:

opentelemetry-collector-contrib/receiver/dockerstatsreceiver/metrics.go

Line 52 in 104a272

var metrics []*metricspb.Metric

updateConfiguredResourceLabels:

opentelemetry-collector-contrib/receiver/dockerstatsreceiver/metrics.go

Line 80 in 104a272

func updateConfiguredResourceLabels(md *consumerdata.MetricsData, container *DockerContainer, config *Config) {

Right, these are associated with individual container stat requests so attaching them later would be generally impractical. Host correlation or similar features not tied to metric generation would occur in a relevant processor though.

anuraaga · 2020-09-17T05:50:12Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/label.go

+	metricspb "github.com/census-instrumentation/opencensus-proto/gen-go/metrics/v1"
+)
+
+func containerLabelKeysAndValues(cm ContainerMetadata) ([]*metricspb.LabelKey, []*metricspb.LabelValue) {


This is for ECS, not k8s.

Yeah but k8s is an example with a presumably very similar pattern :)

Also, while Task labels could be added by a ECS Metadata processor

From my understanding, it's not a could be but a when will it be - aren't we planning on adding an ECS processor so the information is added to data published by the application as well to allow correlation between ecs metrics and app metrics?

container labels

Understood on the container labels - they do seem important to add in the receiver for those reasons. So yeah, the worst case is processor is also enabled and a small amount of extra overhead populating task information twice, but not a big deal.

I'll finally add that we've discussed this a bunch internally, and I think Rayhan has discussed it a bit externally as well. This implementation has been thought out.

This isn't helpful because

Internal discussions don't provide value when working in OSS. Going forward, it'll be good to move all related design discussions to GitHub or public Slack or gitter as well to faciliate AWS involvement in OpenTelemetry or other OSS

An implementation being thought out doesn't mean rubberstamped reviews or allow avoiding probing questions during reviews. It's what code reviews are there for

anuraaga · 2020-09-17T05:54:58Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/label.go

+	labelKeys := make([]*metricspb.LabelKey, 0, 3)
+	labelValues := make([]*metricspb.LabelValue, 0, 3)
+
+	labelKeys = append(labelKeys, &metricspb.LabelKey{Key: "ecs.container-name"})


Ack the need for the container labels, let's use the OTel semantic conventiosn for the container ones

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/resource/semantic_conventions/container.md

container.name
container.id

See https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/master/receiver/kubeletstatsreceiver/kubelet/metadata.go#L30

And I think docker name is the host.name

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/resource/semantic_conventions/host.md

If not, ecs.docker-name for that is fine too.

PettitWesley

LGTM

rmfitzpatrick · 2020-09-18T19:13:49Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/label.go

+	labelKeys := make([]*metricspb.LabelKey, 0, 3)
+	labelValues := make([]*metricspb.LabelValue, 0, 3)
+
+	labelKeys = append(labelKeys, &metricspb.LabelKey{Key: "container.name"})


I've been told using the convention constants directly where applicable is preferred: https://github.com/open-telemetry/opentelemetry-collector/blob/master/translator/conventions/opentelemetry.go

Good point. Used the default convention. Also, transferred some of my values to a constant file.

rmfitzpatrick · 2020-09-18T19:26:10Z

receiver/awsecscontainermetricsreceiver/testdata/task_metadata.json

@@ -0,0 +1,106 @@
+{


Is/can this test data being used anywhere in these changes or existing structure? Not immediately clear why it's being introduced.

Actually, it was a big PR. Later we decided to to split it into 3 PRs to make the review process faster. In this PR we are mainly adding the structs and test data. This is being used by our code which are coming with next PR.
Yea, we can debate but I think it's fine.

rmfitzpatrick · 2020-09-18T19:31:04Z

Unfamiliar with ECS subtleties, this looks good to me. Just a few nits/questions, mainly around the test data and potentially required attribution(s).

rmfitzpatrick · 2020-09-18T19:33:19Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/label_test.go

+		DockerName:    "docker-container-1",
+	}
+	k, v := containerLabelKeysAndValues(cm)
+	require.EqualValues(t, 3, len(k))


Would having more detailed label content checks be helpful with these cases or will they be prone to change?

I think it's fine as we will always have correct values if we come to this point. Also, I made a little change- moved the hardcoded length to a constant file.

Additionally, I added some minor checks.

rmfitzpatrick · 2020-09-18T19:35:19Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/label.go

+}
+
+func getTaskIDFromARN(arn string) string {
+	if !strings.HasPrefix(arn, "arn:aws") || arn == "" {


curious if there's a convention in having the empty check last, as it would potentially avoid a function call if it were first.

Good catch. I shuffled their position, thanks.

hossain-rayhan · 2020-09-18T21:39:38Z

Thanks @rmfitzpatrick for your review. I pushed an update based on your feedback.

hossain-rayhan · 2020-09-21T17:54:26Z

@tigrannajaryan Can we get it merged?

* Replace `WithSyncer` with `WithBatcher` in examples * update CHANGELOG Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

Add data types to support metrics calculation

c0f5367

hossain-rayhan requested a review from a team September 17, 2020 01:02

hossain-rayhan changed the title ~~Add data types to support metrics calculation~~ Add test data and data types for awsecscontainermetricsreceiver Sep 17, 2020

PettitWesley reviewed Sep 17, 2020

View reviewed changes

anuraaga reviewed Sep 17, 2020

View reviewed changes

hossain-rayhan mentioned this pull request Sep 17, 2020

[Add] metric conversion logic in the "AWSECSContainerMetricsReceiver" #978

Closed

anuraaga reviewed Sep 17, 2020

View reviewed changes

Rename container labels following OT convention

5a8adfe

PettitWesley approved these changes Sep 17, 2020

View reviewed changes

tigrannajaryan assigned rmfitzpatrick Sep 17, 2020

anuraaga approved these changes Sep 17, 2020

View reviewed changes

Resolve conflict in receiver go.sum file

477cd26

rmfitzpatrick reviewed Sep 18, 2020

View reviewed changes

Make use of opentelemetry constant for label attributes

51ccd85

hossain-rayhan force-pushed the datatype branch from ce2f739 to 51ccd85 Compare September 18, 2020 21:34

Add minor checkings in label test cases

d7ba9d5

rmfitzpatrick approved these changes Sep 21, 2020

View reviewed changes

bogdandrutu approved these changes Sep 21, 2020

View reviewed changes

bogdandrutu merged commit ba5fced into open-telemetry:master Sep 21, 2020

anuraaga mentioned this pull request Sep 29, 2020

[AwsEcsMetricsReceiver] Add codes to read data from ECS endpoint #1148

Merged

dyladan referenced this pull request in dynatrace-oss-contrib/opentelemetry-collector-contrib Jan 29, 2021

Add Wandera to ADOPTERS (#1044)

87bd97e

ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022

Replace WithSyncer with WithBatcher in examples (#1044)

f9ba15f

* Replace `WithSyncer` with `WithBatcher` in examples * update CHANGELOG Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

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

Pin bleach to 4.1.0 (#1044)

d760668

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test data and data types for `awsecscontainermetricsreceiver` #1044

Add test data and data types for `awsecscontainermetricsreceiver` #1044

hossain-rayhan commented Sep 17, 2020

codecov bot commented Sep 17, 2020 •

edited

Loading

PettitWesley Sep 17, 2020

anuraaga Sep 17, 2020

hossain-rayhan Sep 17, 2020

rmfitzpatrick Sep 18, 2020

anuraaga left a comment

anuraaga Sep 17, 2020

PettitWesley Sep 17, 2020 •

edited

Loading

hossain-rayhan Sep 17, 2020

anuraaga Sep 17, 2020

PettitWesley Sep 17, 2020

hossain-rayhan Sep 17, 2020

anuraaga Sep 17, 2020

lubingfeng Sep 17, 2020

rmfitzpatrick Sep 18, 2020

anuraaga Sep 17, 2020

anuraaga Sep 17, 2020

hossain-rayhan Sep 17, 2020

PettitWesley left a comment

rmfitzpatrick Sep 18, 2020

hossain-rayhan Sep 18, 2020

rmfitzpatrick Sep 18, 2020

hossain-rayhan Sep 18, 2020

rmfitzpatrick commented Sep 18, 2020

rmfitzpatrick Sep 18, 2020

hossain-rayhan Sep 18, 2020

hossain-rayhan Sep 18, 2020

rmfitzpatrick Sep 18, 2020

hossain-rayhan Sep 18, 2020

hossain-rayhan commented Sep 18, 2020

hossain-rayhan commented Sep 21, 2020

		// ContainerStats defines the structure for container stats
		type ContainerStats struct {

Add test data and data types for awsecscontainermetricsreceiver #1044

Add test data and data types for awsecscontainermetricsreceiver #1044

Conversation

hossain-rayhan commented Sep 17, 2020

codecov bot commented Sep 17, 2020 • edited Loading

Codecov Report

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

PettitWesley Sep 17, 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

PettitWesley 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

rmfitzpatrick commented Sep 18, 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

hossain-rayhan commented Sep 18, 2020

hossain-rayhan commented Sep 21, 2020

Add test data and data types for `awsecscontainermetricsreceiver` #1044

Add test data and data types for `awsecscontainermetricsreceiver` #1044

codecov bot commented Sep 17, 2020 •

edited

Loading

PettitWesley Sep 17, 2020 •

edited

Loading