[extension][ecsobserver] Init ECS service discovery using docker label #2734

pingleig · 2021-03-18T00:40:51Z

Description:

This is the first (divided) implementation PR for #1395. In order to reduce the size of the pr, the feature is incomplete.

mock api client is used, no actual aws api calls
Only docker label based filter (matcher) is included (others are just place holders)
Extension impl under ecsobserver is excluded, it's a thin wrapper.

Those excluded logic are tested in other branches, so it works with real API end to end (eventually).

~~The package is located under internal/aws/ecssd~~ (relocated to extension because currently there is no other aws related component reusing it)

Link to tracking Issue:

Feature request [Feature Request] [Extension] ECS Service Discovery for Prometheus #1395
Config PR (merged) [extension][ecsobserver] Init ECS Observer README #2377

Testing:

unit test

Documentation:

No new doc, see #2377 for existing doc

Known issues

customise job in service discovery output will cause scrape error, Prometheus Job and Instance labels opentelemetry-collector#575 so a new config is added and default to prometheus_job

linux-foundation-easycla · 2021-03-18T00:40:53Z

The committers are authorized under a signed CLA.

✅ Pinglei Guo (b70159f)

This is the first (divided) implementation PR for #1395. In order to reduce the size of the pr, the feature is incomplete. - mock api client is used, no actual aws api calls - Only docker label based filter/matcher is included - Extension impl under `ecsobserver` is excluded Those excluded logic are tested in other branches, so it works with real API end to end (eventually). The package is located under internal/aws/ecssd because - it can be used by other aws specific packages - we may extract it out to a standalone repo in the future as it can run as a binary and support other prometheus compatiable collectors as a sidecar

pingleig · 2021-03-18T00:44:00Z

I guess the bot should have assigned it to @jrcamp

The lint error is strange

make[1]: Entering directory '/home/circleci/project'
golangci-lint run --allow-parallel-runners
ERRO Running error: context loading failed: no go files to analyze 
make[1]: *** [Makefile.Common:93: lint] Error 5
make[1]: Leaving directory '/home/circleci/project'

codecov · 2021-03-18T01:01:36Z

Codecov Report

Merging #2734 (5d200bd) into main (f8c550f) will increase coverage by 0.88%.
The diff coverage is 81.28%.

@@            Coverage Diff             @@
##             main    #2734      +/-   ##
==========================================
+ Coverage   90.53%   91.42%   +0.88%     
==========================================
  Files         444      457      +13     
  Lines       21984    22450     +466     
==========================================
+ Hits        19904    20525     +621     
+ Misses       1618     1434     -184     
- Partials      462      491      +29

Flag	Coverage Δ
integration	`69.18% <ø> (?)`
unit	`90.37% <81.28%> (-0.17%)`	⬇️

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

Impacted Files	Coverage Δ
extension/observer/ecsobserver/factory.go	`46.66% <46.66%> (ø)`
extension/observer/ecsobserver/fetcher.go	`50.00% <50.00%> (ø)`
extension/observer/ecsobserver/task.go	`68.00% <68.00%> (ø)`
extension/observer/ecsobserver/matcher.go	`74.07% <74.07%> (ø)`
extension/observer/ecsobserver/extension.go	`80.00% <80.00%> (ø)`
extension/observer/ecsobserver/filter.go	`82.14% <82.14%> (ø)`
extension/observer/ecsobserver/sd.go	`82.14% <82.14%> (ø)`
extension/observer/ecsobserver/target.go	`82.35% <82.35%> (ø)`
extension/observer/ecsobserver/exporter.go	`84.90% <84.90%> (ø)`
extension/observer/ecsobserver/config.go	`100.00% <100.00%> (ø)`
... and 25 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 f8c550f...5d200bd. Read the comment docs.

- ignore *.actual.yaml and only check in *.expected.yaml

jrcamp · 2021-03-19T20:17:02Z

@pingleig overall looks good. Can somebody at AWS review first? Quite a lot of code.

pingleig · 2021-03-19T21:00:14Z

@jrcamp thanks! I just asked in internal channel for review. There should be some follow up PRs, those should be smaller because most interface and large struct definitions are in this PR.

internal/aws/ecssd/config.go

mxiamxia · 2021-03-23T20:52:23Z

internal/aws/ecssd/docker_label.go

+func (d *DockerLabelConfig) Init() error {
+	// It's possible to support it in the future, but for now just fail at config,
+	// so user don't need to wonder which port is used in the exported target.
+	if len(d.MetricsPorts) != 0 {


if it is not supported now, can we remove CommonExporterConfig it in DockerLabelConfig? will it misleading customers by just looking Config structure?

I don't want to split the shared config because docker label is just one of three matchers, though its behaviour is pretty different from service and task based matcher so we could give it another struct like ExportConfigWithoutPorts. I plan to delay the change for next pr when service & task are added.

Actually I think since we are going to have open-telemetry/opentelemetry-collector#2802 it's better to put the validate logic here instead of removing it from struct. Because if user use metrics_ports for docker label and it's not in the struct, it will throw unmarshal error without telling why it is not there while it is supported for other filters. Though in the long term I think docker label should become part of common export config as well, you can discover based on service name and pick port based on docker label in task definition, we are just trying to be consistent with cloudwatch agent's behaviour here at this moment.

internal/aws/ecssd/filter.go

There is no plan to reuse it in other aws related components

Also fix go fmt problems ...

pingleig · 2021-03-24T00:37:22Z

oops, windows test failed because of \r\n and \n when comparing generated file https://app.circleci.com/pipelines/github/open-telemetry/opentelemetry-collector-contrib/11473/workflows/8898ac3b-3231-431e-a457-625ca5ca6442/jobs/97574 I will grab a windows pc and test it (if I don't end up playing games)

It seems when git checkout, the file \n is replaced with \r\n https://circleci.com/blog/circleci-config-teardown-how-we-write-our-circleci-config-at-circleci/#main:~:text=Line%20endings

The expected file is converted to \r\n during git checkout, however the generated file is still using \n. This only happens on circle ci's windows environemnt. See https://circleci.com/blog/circleci-config-teardown-how-we-write-our-circleci-config-at-circleci/#main:~:text=Line%20endings

mxiamxia

LGTM in general. pls address the comments. Thanks!

extension/observer/ecsobserver/config.go

extension/observer/ecsobserver/fetcher.go

extension/observer/ecsobserver/matcher.go

extension/observer/ecsobserver/filter.go

- Fix typo - Add more comment on how the matcher merge logic works

pingleig · 2021-03-26T21:38:56Z

@mxiamxia addressed comments in 5d200bd and @jrcamp PTAL (btw: the code grows a bit bigger because the extension interface is implemented compared w/ last time you look at it)

mxiamxia

LGTM! Thanks!

jrcamp · 2021-03-29T19:16:12Z

@pingleig Can we find a way to split this up more? I just don't have the cycles to digest such a large PR all in one.

pingleig · 2021-03-29T19:19:49Z

@jrcamp it's doable, but in that case I don't think I can meet the requirement for test coverage because they will not work end to end (even in unit test). If that's ok I can create a new pr for just structs.

pingleig · 2021-03-29T21:28:51Z

@jrcamp created #2939

github-actions · 2021-04-06T05:21:22Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

pingleig · 2021-04-06T17:08:54Z

Close it since half of the PR is split into #2939 and I plan create a new PR for the remaining stuff instead of rebasing on this branch.

Split from open-telemetry#2734 - Get port number from label value - Check if the port number exists in container definition

Split from #2734 - Get port number from label value - Check if the port number exists in container definition

pingleig requested a review from a team March 18, 2021 00:40

github-actions bot assigned jpkrohling Mar 18, 2021

pingleig force-pushed the ext-ecssd-docker-label branch from cf211b3 to b70159f Compare March 18, 2021 00:42

aws: ecssd Remove unused code

a3c1a0f

- ignore *.actual.yaml and only check in *.expected.yaml

jrcamp assigned jrcamp and unassigned jpkrohling Mar 18, 2021

pingleig mentioned this pull request Mar 21, 2021

[feature][prometheus] ECS service discovery extension for prometheus aws-observability/aws-otel-collector#412

Closed

4 tasks

mxiamxia reviewed Mar 23, 2021

View reviewed changes

pingleig added 4 commits March 23, 2021 15:56

extension: Move ecssd into extension/observer/ecsobeserver

68a8e72

There is no plan to reuse it in other aws related components

extension: Init go mod and makefile for ecssd

3fcd395

internal: Revert go mod for internal/aws

dbd6551

extension: ecssd implement extension interface

d124986

pingleig requested a review from jrcamp as a code owner March 23, 2021 23:45

extension: ecssd Add docker label to default config

b6674f4

Also fix go fmt problems ...

mxiamxia reviewed Mar 26, 2021

View reviewed changes

extension: ecssd Make default config values internal

5d200bd

- Fix typo - Add more comment on how the matcher merge logic works

mxiamxia approved these changes Mar 26, 2021

View reviewed changes

pingleig mentioned this pull request Mar 29, 2021

[extension][ecsobserver] Init config and target definition #2939

Merged

github-actions bot added the Stale label Apr 6, 2021

pingleig closed this Apr 6, 2021

pingleig mentioned this pull request Apr 15, 2021

[extension/ecsobserver] Add get ip and port from task #3133

Merged

pingleig mentioned this pull request Apr 28, 2021

[extension/ecsobserver] Add docker label based matcher #3276

Merged

pingleig mentioned this pull request Apr 30, 2021

[extension/ecsobserver] Init service client and mock #3284

Merged

pingleig mentioned this pull request May 6, 2021

[extension/ecsobserver] Add exporter to convert task to target #3333

Merged

bogdandrutu pushed a commit that referenced this pull request May 11, 2021

ext: ecsobserver Add docker label based matcher (#3276)

c7591c8

Split from #2734 - Get port number from label value - Check if the port number exists in container definition

pingleig mentioned this pull request May 14, 2021

[extension/ecsobserver] Add service and task definition based matcher #3386

Merged

pingleig mentioned this pull request May 24, 2021

[extension/ecsobserver] Add task definition, ec2 and service fetcher #3503

Merged

pingleig mentioned this pull request Jun 14, 2021

[extension/ecsobserver] Write discovered targets as Prometheus file sd #3785

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[extension][ecsobserver] Init ECS service discovery using docker label #2734

[extension][ecsobserver] Init ECS service discovery using docker label #2734

pingleig commented Mar 18, 2021 •

edited

Loading

linux-foundation-easycla bot commented Mar 18, 2021 •

edited

Loading

pingleig commented Mar 18, 2021 •

edited

Loading

codecov bot commented Mar 18, 2021 •

edited

Loading

jrcamp commented Mar 19, 2021

pingleig commented Mar 19, 2021 •

edited

Loading

mxiamxia Mar 23, 2021

pingleig Mar 23, 2021

pingleig Mar 26, 2021

pingleig commented Mar 24, 2021 •

edited

Loading

mxiamxia left a comment

pingleig commented Mar 26, 2021

mxiamxia left a comment

jrcamp commented Mar 29, 2021

pingleig commented Mar 29, 2021

pingleig commented Mar 29, 2021

github-actions bot commented Apr 6, 2021

pingleig commented Apr 6, 2021 •

edited

Loading

[extension][ecsobserver] Init ECS service discovery using docker label #2734

[extension][ecsobserver] Init ECS service discovery using docker label #2734

Conversation

pingleig commented Mar 18, 2021 • edited Loading

Known issues

linux-foundation-easycla bot commented Mar 18, 2021 • edited Loading

pingleig commented Mar 18, 2021 • edited Loading

codecov bot commented Mar 18, 2021 • edited Loading

Codecov Report

jrcamp commented Mar 19, 2021

pingleig commented Mar 19, 2021 • edited Loading

mxiamxia Mar 23, 2021

Choose a reason for hiding this comment

pingleig Mar 23, 2021

Choose a reason for hiding this comment

pingleig Mar 26, 2021

Choose a reason for hiding this comment

pingleig commented Mar 24, 2021 • edited Loading

mxiamxia left a comment

Choose a reason for hiding this comment

pingleig commented Mar 26, 2021

mxiamxia left a comment

Choose a reason for hiding this comment

jrcamp commented Mar 29, 2021

pingleig commented Mar 29, 2021

pingleig commented Mar 29, 2021

github-actions bot commented Apr 6, 2021

pingleig commented Apr 6, 2021 • edited Loading

pingleig commented Mar 18, 2021 •

edited

Loading

linux-foundation-easycla bot commented Mar 18, 2021 •

edited

Loading

pingleig commented Mar 18, 2021 •

edited

Loading

codecov bot commented Mar 18, 2021 •

edited

Loading

pingleig commented Mar 19, 2021 •

edited

Loading

pingleig commented Mar 24, 2021 •

edited

Loading

pingleig commented Apr 6, 2021 •

edited

Loading