feat: Implement experimental AWS ECS resource attributes #1083

mmanciop · 2022-07-06T16:33:45Z

Which problem is this PR solving?

The resource attributes for AWS ECS returned by the AwsEcsDetector are missing all the fun stuff like ARNs, LaunchType and references to the TaskDefinition (a.k.a. "Family").

Short description of the changes

Implement the experimental AWS ECS resource attributes [1] using the Metadata v4 Endpoint.

Checklist

Ran npm run test-all-versions for the edited package(s) on the latest commit if applicable.

[1] https://opentelemetry.io/docs/reference/specification/resource/semantic_conventions/cloud_provider/aws/ecs/

NathanielRN · 2022-07-06T17:15:34Z

Tagging @vasireddy99 and @bryan-aguilar to review this PR for it's changes to the AWS Resource Detectors.

mmanciop · 2022-07-22T09:21:35Z

Hi @vasireddy99 and @bryan-aguilar, is there any news on this? We at Lumigo would love more fine-grained info about ECS Tasks to become available, and we'd be willing to do similar contributions for other SDKs.

detectors/node/opentelemetry-resource-detector-aws/src/detectors/AwsEcsDetector.ts

dyladan · 2022-08-04T12:37:54Z

LGTM. waiting on component owner review from @NathanielRN or @willarmiros

codecov · 2022-08-04T13:11:21Z

Codecov Report

Merging #1083 (cfb678d) into main (3522660) will decrease coverage by 0.40%.
The diff coverage is 91.78%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1083      +/-   ##
==========================================
- Coverage   96.08%   95.67%   -0.41%     
==========================================
  Files          14       20       +6     
  Lines         893     1157     +264     
  Branches      191      220      +29     
==========================================
+ Hits          858     1107     +249     
- Misses         35       50      +15

Impacted Files	Coverage Δ
...ource-detector-aws/src/detectors/AwsEcsDetector.ts	`93.33% <91.78%> (ø)`
...ce-detector-aws/src/detectors/AwsLambdaDetector.ts	`100.00% <0.00%> (ø)`
...ource-detector-aws/src/detectors/AwsEc2Detector.ts	`97.91% <0.00%> (ø)`
...detector-aws/src/detectors/AwsBeanstalkDetector.ts	`95.65% <0.00%> (ø)`
...ource-detector-aws/src/detectors/AwsEksDetector.ts	`91.25% <0.00%> (ø)`
...metry-resource-detector-aws/src/detectors/index.ts	`100.00% <0.00%> (ø)`

dyladan · 2022-08-09T18:03:50Z

Ping @NathanielRN or @willarmiros

willarmiros · 2022-08-09T18:08:09Z

Sorry for the delay on this, will take a look within the next week

mmanciop · 2022-08-12T15:31:22Z

There's discussion on open-telemetry/opentelemetry-java#4574 (comment) whether the ARNs like the one this PR builds are actually correct. Until we are use, let's not merge :D

mmanciop · 2022-08-12T16:04:29Z

Update: The uncertainty about ARNs seems sorted out

willarmiros

Tested this locally with the unit test's mocked TMDE response and the resource data looked good! Should be good to go once the CI merges.

detectors/node/opentelemetry-resource-detector-aws/src/detectors/AwsEcsDetector.ts

mmanciop · 2022-08-14T09:56:22Z

@dyladan care to give it another look? Support for aws.logs was added since you last did IIRC.

dyladan · 2022-08-18T20:08:22Z

@willarmiros is the component owner this is OK to merge when he gives the 👍

willarmiros

lgtm, I think just the error needs a type to fix the linter :)

vsakaram · 2022-09-06T19:34:17Z

@dyladan - tagging to request your review/approval of this PR change

dyladan · 2022-09-06T19:50:49Z

Ah sorry I missed will's comment. i'll run the tests and merge this

dyladan · 2022-09-06T20:24:42Z

Please update the PR as an outdated PR cannot be merged. The tests are failing for an unrelated reason that I believe has been fixed since the last push on this PR.

Implement the experimental AWS ECS resource attributes [1] using the Metadata v4 Endpoint. [1] https://opentelemetry.io/docs/reference/specification/resource/semantic_conventions/cloud_provider/aws/ecs/

mmanciop · 2022-09-08T07:48:56Z

@dyladan PR rebased. I also remove the trailing :* in the ARN for the log group as it currently creates issues with the AWS Distro of OpenTelemetry (see open-telemetry/opentelemetry-java#4574).

rauno56 · 2022-09-14T15:05:36Z

Would love to merge this, @mmanciop. We cannot merge out-of-date branch. Please update.

It's best to allow changes from maintainers, so we can keep it up to date ourselves. That permission doesn't work with org-owned forks though.

mmanciop · 2022-09-14T16:12:54Z

@rauno56 another branch update has happened :-)

rauno56 · 2022-09-14T16:16:32Z

Cool... Thanks, I'll keep an eye on this and merge once CI greens.

Please make future PRs from forks under your own account to make the process a little bit easier for us! 🙏

rauno56 · 2022-09-14T17:10:10Z

Do you think you could increase coverage across the diff at all?

mmanciop · 2022-09-14T17:43:34Z

@rauno56 I’ll give it a shot

mmanciop · 2022-09-15T16:04:35Z

@rauno56 I took a page off the AwsEks detector and used nock as HTTP mock server, and that has helped a bunch according to my local test runs :-)

-------------------------|---------|----------|---------|---------|-------------------
File                     | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
-------------------------|---------|----------|---------|---------|-------------------
All files                |   94.82 |    89.06 |   92.85 |   94.75 |                   
 AwsBeanstalkDetector.ts |   95.23 |       50 |     100 |   95.23 | 52                
 AwsEc2Detector.ts       |   97.67 |      100 |     100 |   97.61 | 140               
 AwsEcsDetector.ts       |   94.66 |    90.62 |   85.71 |   94.59 | 188-189,196,223   
 AwsEksDetector.ts       |   91.54 |      100 |    92.3 |   91.42 | 142-151,213       
 AwsLambdaDetector.ts    |     100 |     62.5 |     100 |     100 | 51-58             
 index.ts                |     100 |      100 |     100 |     100 |                   
-------------------------|---------|----------|---------|---------|-------------------

detectors/node/opentelemetry-resource-detector-aws/src/detectors/AwsEcsDetector.ts

rauno56

Thanks for the contribution!

mmanciop · 2022-09-17T05:13:08Z

@NathanielRN would you please have a look?

willarmiros · 2022-09-19T15:37:40Z

@NathanielRN is no longer an approver - he should be removed from the CODEOWNERS on this repo. Either way we should not block on his review, this is good to be merged from my perspective.

NathanielRN

Approving so no one blocks on my review!

blumamir

Sorry for adding this review so late.
Added a few comments, some of them are old issues that I saw during the review and are here mostly for discussion and not to fix as part of this PR.

None is blocker but could be nice to handle if you are up for it

detectors/node/opentelemetry-resource-detector-aws/src/detectors/AwsEcsDetector.ts

blumamir · 2022-09-22T11:23:21Z

detectors/node/opentelemetry-resource-detector-aws/src/detectors/AwsEcsDetector.ts

+    if (hostName || containerId) {
+      return new Resource({
+        [SemanticResourceAttributes.CONTAINER_NAME]: hostName || '',
+        [SemanticResourceAttributes.CONTAINER_ID]: containerId || '',


Also here, I can see that the detector was and is using empty string when the value is unknown.
Wondering if it's cleaner to just not populate this attribute at all in this case 🤔

It is done consistently like this in the various ECS detectors across the SDKs that have them. Not sure if it is an especially good practice tbh.

detectors/node/opentelemetry-resource-detector-aws/src/detectors/AwsEcsDetector.ts

mmanciop · 2022-09-22T15:07:16Z

@blumamir another round?

blumamir · 2022-09-28T15:37:47Z

@blumamir another round?

Sorry for the delay. Thank you :) can you please rebase so I can merge?

mmanciop · 2022-09-28T16:11:17Z

@blumamir done

mmanciop requested a review from a team as a code owner July 6, 2022 16:33

github-actions bot assigned NathanielRN and willarmiros Jul 6, 2022

github-actions bot requested review from NathanielRN and willarmiros July 6, 2022 16:34

mmanciop changed the title ~~Implement experimental AWS ECS resource attributes~~ feat: Implement experimental AWS ECS resource attributes Jul 6, 2022

mmanciop force-pushed the aws_ecs_experimental_attributes branch from e94077f to 84aa245 Compare July 6, 2022 16:40

dyladan reviewed Aug 3, 2022

View reviewed changes

mmanciop requested a review from dyladan August 4, 2022 12:15

mmanciop marked this pull request as draft August 12, 2022 15:31

mmanciop marked this pull request as ready for review August 12, 2022 15:51

willarmiros approved these changes Aug 13, 2022

View reviewed changes

detectors/node/opentelemetry-resource-detector-aws/src/detectors/AwsEcsDetector.ts Show resolved Hide resolved

willarmiros approved these changes Aug 18, 2022

View reviewed changes

dyladan approved these changes Sep 6, 2022

View reviewed changes

feat: implement experimental AWS ECS resource attributes

3934441

Implement the experimental AWS ECS resource attributes [1] using the Metadata v4 Endpoint. [1] https://opentelemetry.io/docs/reference/specification/resource/semantic_conventions/cloud_provider/aws/ecs/

mmanciop force-pushed the aws_ecs_experimental_attributes branch from ea3ea2c to 3934441 Compare September 8, 2022 07:45

Merge branch 'main' into aws_ecs_experimental_attributes

24e2b75

Michele Mancioppi added 2 commits September 15, 2022 18:03

test: increase test coverage using nock to test HTTP fetch

dee239d

Merge branch 'main' into aws_ecs_experimental_attributes

3ea36e1

rauno56 reviewed Sep 15, 2022

View reviewed changes

detectors/node/opentelemetry-resource-detector-aws/src/detectors/AwsEcsDetector.ts Outdated Show resolved Hide resolved

Michele Mancioppi added 4 commits September 15, 2022 19:00

bug: remove leftover code

2f79d48

Merge branch 'main' into aws_ecs_experimental_attributes

5884ee4

fix: lint

15a67d5

Merge branch 'main' into aws_ecs_experimental_attributes

e5117b8

rauno56 approved these changes Sep 16, 2022

View reviewed changes

Merge branch 'main' into aws_ecs_experimental_attributes

988e1d3

NathanielRN approved these changes Sep 20, 2022

View reviewed changes

blumamir reviewed Sep 22, 2022

View reviewed changes

fix: address review comments

ee70af1

blumamir approved these changes Sep 28, 2022

View reviewed changes

Merge branch 'main' into aws_ecs_experimental_attributes

8131bf7

Merge branch 'main' into aws_ecs_experimental_attributes

cfb678d

blumamir merged commit bea8a55 into open-telemetry:main Sep 29, 2022

dyladan mentioned this pull request Sep 29, 2022

chore: release main #1214

Merged

mmanciop mentioned this pull request Oct 29, 2022

feat: implement cloud{provider,platform}, aws.{ecs.*,log.*} in ECS detector open-telemetry/opentelemetry-php-contrib#83

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement experimental AWS ECS resource attributes #1083

feat: Implement experimental AWS ECS resource attributes #1083

mmanciop commented Jul 6, 2022

NathanielRN commented Jul 6, 2022

mmanciop commented Jul 22, 2022

dyladan commented Aug 4, 2022

codecov bot commented Aug 4, 2022 •

edited

dyladan commented Aug 9, 2022

willarmiros commented Aug 9, 2022

mmanciop commented Aug 12, 2022 •

edited

mmanciop commented Aug 12, 2022

willarmiros left a comment

mmanciop commented Aug 14, 2022

dyladan commented Aug 18, 2022

willarmiros left a comment

vsakaram commented Sep 6, 2022

dyladan commented Sep 6, 2022

dyladan commented Sep 6, 2022

mmanciop commented Sep 8, 2022

rauno56 commented Sep 14, 2022

mmanciop commented Sep 14, 2022

rauno56 commented Sep 14, 2022

rauno56 commented Sep 14, 2022

mmanciop commented Sep 14, 2022

mmanciop commented Sep 15, 2022 •

edited

rauno56 left a comment

mmanciop commented Sep 17, 2022

willarmiros commented Sep 19, 2022

NathanielRN left a comment

blumamir left a comment

blumamir Sep 22, 2022

mmanciop Sep 22, 2022

mmanciop commented Sep 22, 2022

blumamir commented Sep 28, 2022

mmanciop commented Sep 28, 2022

feat: Implement experimental AWS ECS resource attributes #1083

feat: Implement experimental AWS ECS resource attributes #1083

Conversation

mmanciop commented Jul 6, 2022

Which problem is this PR solving?

Short description of the changes

Checklist

NathanielRN commented Jul 6, 2022

mmanciop commented Jul 22, 2022

dyladan commented Aug 4, 2022

codecov bot commented Aug 4, 2022 • edited

Codecov Report

dyladan commented Aug 9, 2022

willarmiros commented Aug 9, 2022

mmanciop commented Aug 12, 2022 • edited

mmanciop commented Aug 12, 2022

willarmiros left a comment

Choose a reason for hiding this comment

mmanciop commented Aug 14, 2022

dyladan commented Aug 18, 2022

willarmiros left a comment

Choose a reason for hiding this comment

vsakaram commented Sep 6, 2022

dyladan commented Sep 6, 2022

dyladan commented Sep 6, 2022

mmanciop commented Sep 8, 2022

rauno56 commented Sep 14, 2022

mmanciop commented Sep 14, 2022

rauno56 commented Sep 14, 2022

rauno56 commented Sep 14, 2022

mmanciop commented Sep 14, 2022

mmanciop commented Sep 15, 2022 • edited

rauno56 left a comment

Choose a reason for hiding this comment

mmanciop commented Sep 17, 2022

willarmiros commented Sep 19, 2022

NathanielRN left a comment

Choose a reason for hiding this comment

blumamir left a comment

Choose a reason for hiding this comment

blumamir Sep 22, 2022

Choose a reason for hiding this comment

mmanciop Sep 22, 2022

Choose a reason for hiding this comment

mmanciop commented Sep 22, 2022

blumamir commented Sep 28, 2022

mmanciop commented Sep 28, 2022

codecov bot commented Aug 4, 2022 •

edited

mmanciop commented Aug 12, 2022 •

edited

mmanciop commented Sep 15, 2022 •

edited