Skip to content

Conversation

seongpil0948
Copy link

Which problem is this PR solving?

AWS ECS Fargate changed their cgroup naming convention to /ecs/<taskId>/<taskId>-<containerId> format. The existing container ID extraction logic only takes the last 64 characters, causing truncation in the middle of the taskId and resulting in incomplete container IDs like 438aa1824ca4058bdcab/c23e5f76c09d438aa1824ca4058bdcab-1234678 instead of the expected full c23e5f76c09d438aa1824ca4058bdcab-1234678.

This breaks compatibility with observability vendors like DataDog that require the complete taskId-containerId format for proper container identification.

Short description of the changes

  • Enhanced container ID extraction: Extract the last path segment from cgroup paths instead of truncating to 64 characters
  • New _extractContainerIdFromLine() method: Handles various container runtime formats (Docker, containerd, CRI-O) with proper prefix/suffix removal
  • Flexible validation: Uses permissive regex pattern [a-zA-Z0-9\-_]+ to support real-world container ID formats
  • Backward compatibility: Maintains fallback to original 64-char logic for legacy formats
  • Comprehensive tests: Added test cases covering DataDog compatibility scenarios and edge cases

Supported formats:

  • New ECS Fargate: /ecs/<taskId>/<taskId>-<containerId>taskId-containerId
  • Docker: /docker/containerid.scopecontainerid
  • Containerd: system.slice:cri-containerd:containeridcontainerid
  • UUID format: /uuid/34dc0b5e-626f-2c5c-4c51-70e34b10e76534dc0b5e-626f-2c5c-4c51-70e34b10e765

Fixes #2455

@pichlermarc
Copy link
Member

@seongpil0948 looks like this PR is in draft, is this intentional? 🤔

@seongpil0948 seongpil0948 marked this pull request as ready for review July 2, 2025 22:26
@seongpil0948 seongpil0948 requested a review from a team as a code owner July 2, 2025 22:26
@seongpil0948
Copy link
Author

@seongpil0948 looks like this PR is in draft, is this intentional? 🤔

i'm sorry to late .. 🫡

@david-luna
Copy link
Contributor

@seongpil0948

There has been a re-organization of the packages in this repository, see #2928

Would you mind to sync your PR with main branch?

@dyladan
Copy link
Member

dyladan commented Jul 9, 2025

Looks like this needs reviews. The conflicts are just from package naming. @jj22ee can you please take a look?

@seongpil0948
Copy link
Author

seongpil0948 commented Jul 12, 2025

i worked on a new branch based on new structured packages
thanks all

@seongpil0948 seongpil0948 force-pushed the fix/2455_ECS-consistent-container-id branch from a84117b to f3db796 Compare July 12, 2025 09:41
Copy link

codecov bot commented Jul 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.58%. Comparing base (41d272d) to head (098ef50).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2855      +/-   ##
==========================================
+ Coverage   89.55%   89.58%   +0.03%     
==========================================
  Files         193      193              
  Lines        9698     9729      +31     
  Branches     2011     2025      +14     
==========================================
+ Hits         8685     8716      +31     
  Misses       1013     1013              
Files with missing lines Coverage Δ
...ource-detector-aws/src/detectors/AwsEcsDetector.ts 97.90% <100.00%> (+0.58%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Refactored test suite to align with existing code style:
- Added helper functions to reduce code duplication
- Improved test organization and descriptions
- Extracted common constants
- Maintained all test functionality while improving readability

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@jj22ee
Copy link
Contributor

jj22ee commented Jul 23, 2025

Hi @seongpil0948, wondering why not opt to use a more automated regex based approach (as mentioned in the issue, something like in DataDog/dd-trace-js#1176, which is now here)?

IMO that would help alleviate some of the burden in the current PR which adds logic to "track and remove known prefixes", execute step-by-step extraction+sanitizing of the lastSegment, and perform two sanity validations based on the length of the extracted segment based on what we think is reasonable. Regex would be more "straight to the point" at a glance, easier to maintain.

seongpil0948 and others added 2 commits July 26, 2025 08:57
…paths

Fixes container ID extraction for the new AWS ECS Fargate cgroup format
(/ecs/<taskId>/<taskId>-<containerId>) by implementing regex-based pattern
matching instead of manual string parsing.

Changes:
- Add _extractContainerIdFromLine() method with ECS-specific regex pattern
- Maintain backward compatibility with legacy 64-character container IDs
- Add comprehensive tests for new format and backward compatibility

Fixes open-telemetry#2455

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@dyladan
Copy link
Member

dyladan commented Aug 6, 2025

@jj22ee looks like there's been changes since your last comment. I think this is ready for another review.

Copy link
Contributor

@jj22ee jj22ee left a comment

Choose a reason for hiding this comment

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

Ty for the fixes & enhancements! I have one remaining nit, and then I can approve.

@jj22ee
Copy link
Contributor

jj22ee commented Aug 22, 2025

Need help from @open-telemetry/javascript-approvers to run the remaining PR workflows.

@jj22ee jj22ee added the has:owner-approval Approved by Component Owner label Aug 26, 2025
@jj22ee
Copy link
Contributor

jj22ee commented Aug 27, 2025

@seongpil0948 There is a lint issue in the PR check. Should be able to be fixed by running:

cd packages/resource-detector-aws
npm run lint:fix

@Copilot Copilot AI review requested due to automatic review settings August 29, 2025 08:18
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes container ID extraction in AWS ECS Fargate environments by updating the logic to handle the new cgroup naming convention /ecs/<taskId>/<taskId>-<containerId>. The existing 64-character truncation was breaking compatibility with observability vendors that require the complete taskId-containerId format.

Key changes:

  • Enhanced container ID extraction to prefer full path segments over character truncation
  • Added new extraction methods with fallback hierarchy for different container runtime formats
  • Maintained backward compatibility with legacy 64-character truncation logic

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
packages/resource-detector-aws/src/detectors/AwsEcsDetector.ts Refactored container ID extraction logic with new methods and fallback hierarchy
packages/resource-detector-aws/test/detectors/AwsEcsDetector.test.ts Added comprehensive test cases for various cgroup formats and edge cases

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@seongpil0948
Copy link
Author

Thank you @pichlermarc !

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

Successfully merging this pull request may close these issues.

[detectors-aws] incomplete containerId detected for AWS ECS Fargate
5 participants