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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Jun 9, 2021
Merged

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

merged 3 commits into from
Jun 9, 2021

Conversation

pingleig
Copy link
Contributor

Description:

  • get ECS task definition
  • get EC2 instance if it's ECS EC2
  • get ECS service and map them to tasks

Most code is in the mock and they looks very similar. Not sure if there is a better way when it comes to generating mock data of specific struct(s) in a for loop.

Link to tracking Issue:

Pending PRs

Previous PRs

Testing:

unit test

Documentation:

No new doc. See existing doc

@pingleig pingleig requested a review from jrcamp as a code owner May 24, 2021 20:48
@pingleig pingleig requested a review from a team May 24, 2021 20:48
@pingleig
Copy link
Contributor Author

pingleig commented May 24, 2021 •
edited
Loading

cc @Aneurysm9

hmm: it seems the bot picked Anuraag

@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2021

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

@github-actions github-actions bot added the Stale label Jun 1, 2021
@pingleig pingleig force-pushed the ecssd-fetcher-def branch from a9cd9fc to c7671df Compare June 1, 2021 17:28
mxiamxia
mxiamxia approved these changes Jun 1, 2021
View reviewed changes
Copy link
Member

@mxiamxia mxiamxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pingleig pingleig force-pushed the ecssd-fetcher-def branch from c7671df to 0aeada8 Compare June 1, 2021 20:41
anuraaga
anuraaga reviewed Jun 2, 2021
View reviewed changes
extension/observer/ecsobserver/fetcher.go
continue
}
var def *ecs.TaskDefinition
if cached, ok := f.taskDefCache.Get(arn); ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it ok for this not to be atomic? Does the LRU have some sort of construct like computeIfAbsent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is only single go routine running, so there is no concurrent access to the LRU cache. The cache is not go routine safe but its only callback is onEvict and we are not using it.

Old code to show how the fetcher get called by a single ticker

@alolita alolita removed the Stale label Jun 3, 2021
@alolita alolita assigned Aneurysm9 and unassigned anuraaga Jun 3, 2021
@pingleig pingleig force-pushed the ecssd-fetcher-def branch 2 times, most recently from f27df83 to b5d336b Compare June 7, 2021 17:58
@Aneurysm9
Copy link
Member

Please ensure the dependencies are up-to-date:

diff --git a/receiver/awscontainerinsightreceiver/go.mod b/receiver/awscontainerinsightreceiver/go.mod
index a13801999..1a7ca5ea1 100644
--- a/receiver/awscontainerinsightreceiver/go.mod
+++ b/receiver/awscontainerinsightreceiver/go.mod
@@ -3,9 +3,9 @@ module github.com/open-telemetry/opentelemetry-collector-contrib/receiver/awscon
 go 1.16
 
 require (
-	github.com/aws/aws-sdk-go v1.38.51
+	github.com/aws/aws-sdk-go v1.38.55
 	github.com/open-telemetry/opentelemetry-collector-contrib/internal/aws/awsutil v0.0.0-00010101000000-000000000000
-	github.com/shirou/gopsutil v3.21.4+incompatible
+	github.com/shirou/gopsutil v3.21.5+incompatible
 	github.com/stretchr/testify v1.7.0
 	go.opentelemetry.io/collector v0.27.1-0.20210603182316-5369d7e9e83e
 	go.uber.org/zap v1.17.0
Generated code is out of date, please run "make generate" and commit the changes in this PR.

Exited with code exit status 1

@pingleig
Copy link
Contributor Author

pingleig commented Jun 7, 2021 •
edited
Loading

The dependency error should be fixed by #3735 I think unit test and lint are breaking for all PRs now.

I will rebase after the fix is merged.

@bogdandrutu
Copy link
Member

Fix is merged please rebase

@alolita alolita added the ready to merge Code review completed; ready to merge by maintainers label Jun 8, 2021
@pingleig pingleig force-pushed the ecssd-fetcher-def branch from b5d336b to 031c586 Compare June 8, 2021 18:59
@pingleig
Copy link
Contributor Author

pingleig commented Jun 8, 2021 •
edited
Loading

@bogdandrutu rebased, thx.

btw: #3333 is also ready to merge.

@bogdandrutu bogdandrutu merged commit 7c3c4c4 into open-telemetry:main Jun 9, 2021
@pingleig pingleig mentioned this pull request Jun 14, 2021
Merged
alexperez52 referenced this pull request in open-o11y/opentelemetry-collector-contrib Aug 18, 2021
@bogdandrutu
Remove usage of Translator/Encoder in OTLP json/protobuf (#3503)
ad5e50c 
This removes unnecessary checks for interface{} types.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxiamxia mxiamxia mxiamxia approved these changes

@Aneurysm9 Aneurysm9 Aneurysm9 approved these changes

@anuraaga anuraaga anuraaga approved these changes

@jrcamp jrcamp Awaiting requested review from jrcamp

Assignees

@Aneurysm9 Aneurysm9

Labels
ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pingleig @Aneurysm9 @bogdandrutu @anuraaga @mxiamxia @alolita