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(resource-detector-aws): add support for cloud.resource_id to AWS ECS detector #1936

Merged
merged 2 commits into from
May 3, 2024

Conversation

mmanciop
Copy link
Contributor

@mmanciop mmanciop commented Feb 14, 2024

Which problem is this PR solving?

Add support for cloud.resource_id in the AWS ECS detector.

Short description of the changes

Implement setting the ContainerARN as value to cloud.resource_id in the AWS ECS detector as discussed in this issue in semantic-conventions.

Since the OpenTelemetry SDK missing the enum entry for cloud.resource_id (to be fixed in this PR), a temporary enum is added to the package.

Copy link

codecov bot commented Feb 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.47%. Comparing base (dfb2dff) to head (9ec3c9f).
Report is 105 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1936      +/-   ##
==========================================
- Coverage   90.97%   90.47%   -0.51%     
==========================================
  Files         146      149       +3     
  Lines        7492     7592     +100     
  Branches     1502     1591      +89     
==========================================
+ Hits         6816     6869      +53     
- Misses        676      723      +47     
Files Coverage Δ
...ource-detector-aws/src/detectors/AwsEcsDetector.ts 96.90% <100.00%> (+0.03%) ⬆️
...or-aws/src/detectors/SemanticResourceAttributes.ts 100.00% <100.00%> (ø)

... and 35 files with indirect coverage changes

@mmanciop
Copy link
Contributor Author

mmanciop commented Apr 15, 2024

@JamieDanielson may I interest you in a review? This logic has been already implemented in most other SDKs that support ECS detectors (.NET, Go, Java, PHP), and has been in review purgatory for... well... forever :-)

(Pinging you because I saw you reviewing another PR in this package, namely #2047)

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

I still don't see the changes @pichlermarc commented here, so until then this PR looks like a good solution

@trentm trentm changed the title feat: add support for cloud.resource_id to AWS ECS detector feat(resource-detector-aws): add support for cloud.resource_id to AWS ECS detector May 2, 2024
@pichlermarc pichlermarc merged commit cc71492 into open-telemetry:main May 3, 2024
17 checks passed
@dyladan dyladan mentioned this pull request May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants