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

Feat: Added AWS ECS Plugins Resource Detector #1404

Merged
merged 21 commits into from
Sep 23, 2020
Merged

Feat: Added AWS ECS Plugins Resource Detector #1404

merged 21 commits into from
Sep 23, 2020

Conversation

EdZou
Copy link
Contributor

@EdZou EdZou commented Aug 7, 2020

Which problem is this PR solving?

Added AWS ECS plugins resource detector.

Short description of the changes

  1. Added AWS ECS plugins resource detector
  2. Added corresponding unit test for AWS ECS detector
  3. Added "container ID" attribute for resource constants.
    Explanation of this part can be referred to issue I opened: Add container ID attribute for Resource constants #1394
    Since there is no response, I just add this part and made a PR.

If you would like to review @dyladan @anuraaga
Really appreciate it!

A Question for ECS and Beanstalk detector PRs

Since we have decided to put vendor's resource detector to js-contrib repository:
I am not so sure when will these code be moved to js-contrib and my internship has limited time to go.
May we first merge these PRs so we could move them together to js-contrib afterwards?

@EdZou EdZou changed the title Ecs detector Feat: Added AWS Ecs Plugins Resource Detector Aug 7, 2020
@codecov
Copy link

codecov bot commented Aug 7, 2020 •
edited
Loading

Codecov Report

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

@@            Coverage Diff             @@
##           master    #1404      +/-   ##
==========================================
+ Coverage   93.11%   93.15%   +0.04%     
==========================================
  Files         154      155       +1     
  Lines        4807     4839      +32     
  Branches      971      980       +9     
==========================================
+ Hits         4476     4508      +32     
  Misses        331      331
Impacted Files Coverage Δ
packages/opentelemetry-resources/src/constants.ts 100.00% <ø> (ø)
...ource-detector-aws/src/detectors/AwsEcsDetector.ts 100.00% <100.00%> (ø)
...metry-resource-detector-aws/src/detectors/index.ts 100.00% <100.00%> (ø)

@EdZou EdZou changed the title Feat: Added AWS Ecs Plugins Resource Detector Feat: Added AWS ECS Plugins Resource Detector Aug 7, 2020
@EdZou EdZou marked this pull request as ready for review August 11, 2020 15:52
@EdZou
Copy link
Contributor Author

EdZou commented Aug 11, 2020

Also, for the container ID attribute can be found in specification document: https://github.com/open-telemetry/opentelemetry-specification/tree/b338f9f63dbf02ff8ebd100e8a847e7bf43e2682/specification/resource/semantic_conventions#container

obecny
obecny approved these changes Aug 26, 2020
View reviewed changes
Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm

michaelgoin
michaelgoin reviewed Aug 27, 2020
View reviewed changes
Copy link
Contributor

@michaelgoin michaelgoin left a comment

Choose a reason for hiding this comment

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

I see this is grabbing the last 64 characters of the first valid (long-enough) line in /proc/self/cgroup, to retrieve the container ID. While it appears the content in /proc/self/cgroup has changed before (docker -> ecs) it seems unlikely future changes would break this approach anytime soon. And AWS folks would know much better than I would.

Did notice a few small things on the testing side for consideration.

packages/opentelemetry-resource-detector-aws/test/detectors/AwsEcsDetector.test.ts Outdated
});

sandbox.assert.calledOnce(hostStub);
// sandbox.assert.calledOnce(readStub);
Copy link
Contributor

Choose a reason for hiding this comment

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

stray commented-out line.

packages/opentelemetry-resource-detector-aws/test/detectors/AwsEcsDetector.test.ts
hostStub = sandbox.stub(os, 'hostname').returns(hostNameData);
readStub = sandbox
.stub(AwsEcsDetector, 'readFileAsync' as any)
.resolves(multiValidCgroupData);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this actually tests what it says.

each of the lines in multiValidCgroupData contain the same ID as the last 64 chars, so it doesn't necessarily prove it grabbed the first 'valid' (length > 64) one. it is minor, i can tell the code is working but perhaps a more accurate test would be to have the first line be 64 chars of a different/unexpected ID as well as having the last line be a valid, but unexpected ID to show you are grabbing the item that is expected (the middle one).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in practice, multiple lines contain container ID in the same time. This testcase is to verify the logic here because we only grab the first valid container ID to reduce time cost.
I also think I can add an unexpected ID here. Thank you for pointing out!

packages/opentelemetry-resource-detector-aws/test/detectors/AwsEcsDetector.test.ts Outdated
});

sandbox.assert.calledOnce(hostStub);
// sandbox.assert.calledOnce(readStub);
Copy link
Contributor

Choose a reason for hiding this comment

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

stray commented-out line.

@EdZou
Copy link
Contributor Author

EdZou commented Sep 3, 2020

@dyladan Hello Daniel. Today is almost the last day of internship. I really appreciate you reviewing and comments on all the PR and the instructions you gave me! Without you, I can never ensure the code quality and satisfy the standard of Typescript. Really Really appreciate it!
I learned a lot and it is a great honor to work with you!
Also thanks you all for reviewing my code and making comments!

@EdZou
Copy link
Contributor Author

EdZou commented Sep 3, 2020

BTW, since I am off-boarding, I am wondering when can this PR be merged?

@dyladan
Copy link
Member

dyladan commented Sep 4, 2020

BTW, since I am off-boarding, I am wondering when can this PR be merged?

We're just waiting on one more @open-telemetry/javascript-approvers and it can be merged

@obecny obecny added enhancement New feature or request feature-request and removed enhancement New feature or request labels Sep 8, 2020
@dyladan
Copy link
Member

dyladan commented Sep 9, 2020

@open-telemetry/javascript-approvers Please take a look at this PR so it can be merged.

mwear
mwear approved these changes Sep 23, 2020
View reviewed changes
Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

LGTM

@dyladan dyladan added the enhancement New feature or request label Sep 23, 2020
@dyladan dyladan merged commit 8b13b07 into open-telemetry:master Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dyladan dyladan dyladan approved these changes

@anuraaga anuraaga anuraaga left review comments

@michaelgoin michaelgoin michaelgoin approved these changes

@mwear mwear mwear approved these changes

@obecny obecny obecny approved these changes

@legendecas legendecas Awaiting requested review from legendecas

@markwolff markwolff Awaiting requested review from markwolff

@mayurkale22 mayurkale22 Awaiting requested review from mayurkale22

@naseemkullah naseemkullah Awaiting requested review from naseemkullah

@OlivierAlbertini OlivierAlbertini Awaiting requested review from OlivierAlbertini

@vmarchaud vmarchaud Awaiting requested review from vmarchaud

Assignees
No one assigned
Labels
enhancement New feature or request feature-request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@EdZou @dyladan @anuraaga @obecny @mwear @michaelgoin