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

hossain-rayhan · 2020-09-28T19:42:20Z

Description:
This change adds codes to read actual metrics data from Amazon ECS task metadata endpoint.

Link to tracking Issue:
#457

Testing:
Unit tests added.

Documentation:
README added.

codecov · 2020-09-28T19:53:58Z

Codecov Report

Merging #1148 into master will increase coverage by 0.05%.
The diff coverage is 96.55%.

@@            Coverage Diff             @@
##           master    #1148      +/-   ##
==========================================
+ Coverage   89.59%   89.65%   +0.05%     
==========================================
  Files         272      276       +4     
  Lines       13240    13321      +81     
==========================================
+ Hits        11863    11943      +80     
- Misses       1011     1012       +1     
  Partials      366      366

Flag	Coverage Δ
#integration	`73.57% <ø> (ø)`
#unit	`88.78% <96.55%> (+0.06%)`	⬆️

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

Impacted Files	Coverage Δ
...eceiver/awsecscontainermetricsreceiver/receiver.go	`93.75% <92.85%> (+2.84%)`	⬆️
...ermetricsreceiver/awsecscontainermetrics/client.go	`94.44% <94.44%> (ø)`
...rmetricsreceiver/awsecscontainermetrics/metrics.go	`100.00% <100.00%> (ø)`
...ricsreceiver/awsecscontainermetrics/rest_client.go	`100.00% <100.00%> (ø)`
...sreceiver/awsecscontainermetrics/stats_provider.go	`100.00% <100.00%> (ø)`
receiver/awsecscontainermetricsreceiver/factory.go	`100.00% <100.00%> (ø)`
...eiver/awsxrayreceiver/internal/udppoller/poller.go	`97.61% <0.00%> (-2.39%)`	⬇️
... and 3 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 cd5dacb...482f1f2. Read the comment docs.

anuraaga · 2020-09-29T05:07:27Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/client.go

+	}
+
+	if resp.StatusCode != http.StatusOK {
+		return nil, fmt.Errorf("task metadata endpoint request GET %s failed - %q, response: %q",


Suggested change

return nil, fmt.Errorf("task metadata endpoint request GET %s failed - %q, response: %q",

return nil, fmt.Errorf("request GET %s failed - %q, response: %q",

Don't think this class is specific to task metadata

anuraaga · 2020-09-29T05:08:25Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/client.go

+	}()
+	body, err := ioutil.ReadAll(resp.Body)
+	if err != nil {
+		return nil, errors.WithMessage(err, "failed to read response body")


Suggested change

return nil, errors.WithMessage(err, "failed to read response body")

return nil, fmt.Errorf("failed to read response body %w", err)

Use native wrapping instead of errors

anuraaga · 2020-09-29T05:18:20Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/stats_provider.go

+}
+
+// GetStats calls the ecs task metadata endpoint and unmarshals the data
+func (p *StatsProvider) GetStats() (map[string]ContainerStats, TaskMetadata, error) {


I think in #1044 we agreed that container metadata needs to be in the receiver, but task metadata I still think we want in a processor that adds to Resource, so it can be used for this, traces, and other metrics. Was it not the case that for task attributes that would be ok? Currently we're not planning on adding these to every SDK since we get more bang-for-buck with a collector-first implementation (this was just decided today though)

Hi @anuraaga , I believe that discussion was more related to setting labels/dimensions/metadata to task level metrics, and later we agreed to add them in the receiver for now. We also know that, if you implement a process later, we will do some redundant works. However, regardless of the label settings, I need to parse this TaskMetadata for calculating some task level metrics value. We cannot skip it. For example, for calculating the value for MemoryReserved, first we check the task level memory which comes from this TaskMetadata. If that is not defined, we get the sum for all containers.

anuraaga · 2020-09-29T05:19:08Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/stats_provider_test.go

+	"io/ioutil"
+	"testing"
+
+	"github.com/pkg/errors"


Avoid errors throughout

anuraaga · 2020-09-29T05:19:27Z

receiver/awsecscontainermetricsreceiver/factory.go

 	typeStr = "awsecscontainermetrics"

-	// Default collection interval
+	// Default collection interval. In every 20s the receiver will collect metrics from Amazon ECS Task Metadata Endpoint


Suggested change

// Default collection interval. In every 20s the receiver will collect metrics from Amazon ECS Task Metadata Endpoint

// Default collection interval. Every 20s the receiver will collect metrics from Amazon ECS Task Metadata Endpoint

Is it task metadata endpoint or container metrics endpoint?

Amazon ECS Task Metadata Endpoint

anuraaga · 2020-09-29T05:22:10Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/constant.go

@@ -30,6 +30,10 @@ const (
 	ContainerPrefix    = "container."
 	MetricResourceType = "aoc.ecs"

+	ENDPOINT         = "ECS_CONTAINER_METADATA_URI_V4"


EndpointEnvKey

hossain-rayhan · 2020-09-29T17:45:59Z

Hi @anuraaga thanks for your fast review. I pushed an update to address your feedbacks.

hossain-rayhan · 2020-09-29T17:49:10Z

Hi @asuresh4 , will appreciate a fast review on this.

asuresh4 · 2020-09-29T19:42:26Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/client.go

+	tr := defaultTransport()
+	return &clientImpl{
+		baseURL:    endpoint,
+		httpClient: http.Client{Transport: tr},


Can you take a look at config.HTTPClientSettings? Looks like right now, there are no config options for the HTTP client but in the future when you add them, it'll be easier to extend if you have use the helpers on the struct. There's a method on HTTPClientSettings, ToClient which returns a HTTP client.

Added a TODO for now.

asuresh4 · 2020-09-29T19:48:33Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/client.go

+	}
+
+	if resp.StatusCode != http.StatusOK {
+		return nil, fmt.Errorf("request GET %s failed - %q, response: %q",


Would logging out the response body make this error too verbose?

asuresh4 · 2020-09-29T19:52:42Z

receiver/awsecscontainermetricsreceiver/factory.go

 ) (component.MetricsReceiver, error) {
-	rCfg := cfg.(*Config)
-	return newAwsEcsContainerMetricsReceiver(params.Logger, rCfg, nextConsumer)
+	ecsTaskMetadataEndpointV4 := os.Getenv(awsecscontainermetrics.EndpointEnvKey)


Would collect the env into a variable of type url.URL, thought should help validate the URL as well, I think.

asuresh4 · 2020-09-29T19:56:01Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/client.go

+var _ Client = (*clientImpl)(nil)
+
+type clientImpl struct {
+	baseURL    string


Would consider making this of type, url.URL.

asuresh4 · 2020-09-29T19:58:09Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/metrics.go

+	"go.opentelemetry.io/collector/consumer/consumerdata"
+)
+
+func MetricsData(containerStatsMap map[string]ContainerStats, metadata TaskMetadata, typeStr string) []*consumerdata.MetricsData {


Is the last parameter here required? Doesn't seem to be used.

asuresh4 · 2020-09-29T20:00:52Z

receiver/awsecscontainermetricsreceiver/awsecscontainermetrics/stats_provider.go

+
+	taskStats, taskMetadata, err := p.rc.EndpointResponse()
+	if err != nil {
+		return stats, metadata, err


Do you think providing the context of the error here would help? Same with the below error.

asuresh4 · 2020-09-29T20:56:01Z

receiver/awsecscontainermetricsreceiver/receiver.go

+		return err
+	}
+
+	mds := awsecscontainermetrics.MetricsData(stats, metadata, typeStr)


Please add a TODO to add self metrics about the receiver that needs to be collected using obsreport.

asuresh4 · 2020-09-29T20:58:00Z

receiver/awsecscontainermetricsreceiver/factory_test.go

@@ -40,6 +55,7 @@ func TestCreateMetricsReceiver(t *testing.T) {
 	)
 	require.NoError(t, err)
 	require.NotNil(t, metricsReceiver)
+


Suggested change

asuresh4 · 2020-09-29T20:58:13Z

receiver/awsecscontainermetricsreceiver/factory_test.go

 	require.Nil(t, metricsReceiver)
-	require.Equal(t, err, componenterror.ErrNilNextConsumer)
+


Suggested change

hossain-rayhan · 2020-09-29T23:34:56Z

Hi @asuresh4 , thanks for your review. I pushed an update based on your review. Expecting another look.

hossain-rayhan · 2020-09-30T16:06:19Z

Hi @tigrannajaryan can we get this merged, please.

* Enable "new-metrics" by default This change switches the defaults between "new-metrics" and "legacy-metrics". Now the defaults are: - "new-metrics" ON by default - "legacy-metrics" OFF by default - "add-instance-id" ON by default * Use Prometheus parser in tests

#1148) * Bump google.golang.org/grpc from 1.31.1 to 1.32.0 in /exporters/stdout Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.31.1 to 1.32.0. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.31.1...v1.32.0) Signed-off-by: dependabot[bot] <[email protected]> * Auto-fix go.sum changes in dependent modules Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com> Co-authored-by: Anthony Mirabella <[email protected]>

Add codes to read data from ECS endpoint

515af63

hossain-rayhan requested a review from a team September 28, 2020 19:42

tigrannajaryan assigned asuresh4 Sep 28, 2020

anuraaga reviewed Sep 29, 2020

View reviewed changes

Add more unit tests, rename variables and address PR feedbacks

bef5254

asuresh4 reviewed Sep 29, 2020

View reviewed changes

Use url.URL instead of string as endpoint type and address PR feedbacks

482f1f2

anuraaga approved these changes Sep 29, 2020

View reviewed changes

asuresh4 approved these changes Sep 30, 2020

View reviewed changes

tigrannajaryan merged commit 7721b29 into open-telemetry:master Sep 30, 2020

	return nil, fmt.Errorf("task metadata endpoint request GET %s failed - %q, response: %q",
	return nil, fmt.Errorf("request GET %s failed - %q, response: %q",

	return nil, errors.WithMessage(err, "failed to read response body")
	return nil, fmt.Errorf("failed to read response body %w", err)

	// Default collection interval. In every 20s the receiver will collect metrics from Amazon ECS Task Metadata Endpoint
	// Default collection interval. Every 20s the receiver will collect metrics from Amazon ECS Task Metadata Endpoint

		require.Nil(t, metricsReceiver)
		require.Equal(t, err, componenterror.ErrNilNextConsumer)

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

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

Uh oh!

Conversation

hossain-rayhan commented Sep 28, 2020

Uh oh!

codecov bot commented Sep 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hossain-rayhan Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hossain-rayhan commented Sep 29, 2020

Uh oh!

hossain-rayhan commented Sep 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hossain-rayhan commented Sep 29, 2020

Uh oh!

codecov bot commented Sep 28, 2020 •

edited

Loading

hossain-rayhan Sep 29, 2020 •

edited

Loading