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.

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

feat: Implement experimental AWS ECS resource attributes #1083

Merged

Conversation

mmanciop
Copy link
Contributor

@mmanciop mmanciop commented Jul 6, 2022

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/

@mmanciop mmanciop requested a review from a team July 6, 2022 16:33
@mmanciop mmanciop changed the title Implement experimental AWS ECS resource attributes feat: Implement experimental AWS ECS resource attributes Jul 6, 2022
@mmanciop mmanciop force-pushed the aws_ecs_experimental_attributes branch from e94077f to 84aa245 Compare July 6, 2022 16:40
@NathanielRN
Copy link
Contributor

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

@mmanciop
Copy link
Contributor Author

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.

@mmanciop mmanciop requested a review from dyladan August 4, 2022 12:15
@dyladan
Copy link
Member

dyladan commented Aug 4, 2022

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

@codecov
Copy link

codecov bot commented Aug 4, 2022

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
Copy link
Member

dyladan commented Aug 9, 2022

Ping @NathanielRN or @willarmiros

@willarmiros
Copy link
Contributor

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

@mmanciop
Copy link
Contributor Author

mmanciop commented Aug 12, 2022

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 mmanciop marked this pull request as draft August 12, 2022 15:31
@mmanciop mmanciop marked this pull request as ready for review August 12, 2022 15:51
@mmanciop
Copy link
Contributor Author

Update: The uncertainty about ARNs seems sorted out

Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

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

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.

@mmanciop
Copy link
Contributor Author

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

@dyladan
Copy link
Member

dyladan commented Aug 18, 2022

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

Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

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

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

@vsakaram
Copy link

vsakaram commented Sep 6, 2022

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

@dyladan
Copy link
Member

dyladan commented Sep 6, 2022

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

@dyladan
Copy link
Member

dyladan commented Sep 6, 2022

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.

@mmanciop mmanciop force-pushed the aws_ecs_experimental_attributes branch from ea3ea2c to 3934441 Compare September 8, 2022 07:45
@mmanciop
Copy link
Contributor Author

mmanciop commented Sep 8, 2022

@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
Copy link
Member

rauno56 commented Sep 14, 2022

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
Copy link
Contributor Author

@rauno56 another branch update has happened :-)

@rauno56
Copy link
Member

rauno56 commented Sep 14, 2022

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
Copy link
Member

rauno56 commented Sep 14, 2022

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

@mmanciop
Copy link
Contributor Author

@rauno56 I’ll give it a shot

@mmanciop
Copy link
Contributor Author

mmanciop commented Sep 15, 2022

@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 |                   
-------------------------|---------|----------|---------|---------|-------------------

Copy link
Member

@rauno56 rauno56 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@mmanciop
Copy link
Contributor Author

@NathanielRN would you please have a look?

@willarmiros
Copy link
Contributor

@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.

Copy link
Contributor

@NathanielRN NathanielRN left a comment

Choose a reason for hiding this comment

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

Approving so no one blocks on my review!

Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@mmanciop
Copy link
Contributor Author

@blumamir another round?

@blumamir
Copy link
Member

@blumamir another round?

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

@mmanciop
Copy link
Contributor Author

@blumamir done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants