[Resources.Aws] Get ECS container ID on Windows#4028
Conversation
Detect container ID for Windows ECS containers. Fixes open-telemetry#750.
github-actions
Bot
added
the
comp:resources.aws
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4028 +/- ##
==========================================
+ Coverage 71.94% 72.08% +0.14%
==========================================
Files 458 448 -10
Lines 17872 17825 -47
==========================================
- Hits 12858 12850 -8
+ Misses 5014 4975 -39
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| // On Linux the container ID is obtained from a file which does not exist on Windows. | ||
| // The ECS Metadata V4 container endpoint always carries the same ID in the "DockerId" field. | ||
| // See https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-metadata-endpoint-v4-response.html. | ||
| if (OperatingSystem.IsWindows() && |
There was a problem hiding this comment.
I was intentionally conservative here to not change the existing behaviour for Linux.
Add PR number.
There was a problem hiding this comment.
Pull request overview
This PR improves AWS ECS resource detection on Windows by extracting the ECS container ID from the ECS Metadata V4 container response, addressing issue #750 where Windows ECS containers lacked container identification.
Changes:
- Extract
container.idfrom the ECS Metadata V4DockerIdfield when running on Windows. - Update ECS detector unit tests to assert
container.idon Windows for V4 metadata scenarios. - Add an entry to the Resources.AWS changelog documenting the behavior change.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/OpenTelemetry.Resources.AWS.Tests/AWSECSDetectorTests.cs | Adds Windows-only assertions for container.id in Metadata V4 tests and minor test refactor. |
| src/OpenTelemetry.Resources.AWS/AWSECSDetector.cs | Adds Windows path to populate container.id from Metadata V4 DockerId. |
| src/OpenTelemetry.Resources.AWS/CHANGELOG.md | Documents the new Windows ECS container.id extraction behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // On Linux the container ID is obtained from a file which does not exist on Windows. | ||
| // The ECS Metadata V4 container endpoint always carries the same ID in the "DockerId" field. | ||
| // See https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-metadata-endpoint-v4-response.html. | ||
| if (OperatingSystem.IsWindows() && | ||
| containerResponse.RootElement.TryGetProperty("DockerId", out var dockerIdElement) && | ||
| dockerIdElement.GetString() is string { Length: > 0 } dockerId) | ||
| { | ||
| resourceAttributes.AddAttributeContainerId(dockerId); | ||
| } |
There was a problem hiding this comment.
Detect() still attempts to read /proc/self/cgroup on all OSes (via GetECSContainerId(AWSECSMetadataPath)), which will throw on Windows and gets logged as an extraction exception. Now that Windows container ID is obtained from Metadata V4, consider skipping the cgroup file read when OperatingSystem.IsWindows() (or otherwise avoiding the exception path) to prevent noisy logs and exception overhead on every detection run.
There was a problem hiding this comment.
Open to changing this if wanted. I wanted to minimise the diff.
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
martincostello
added
keep-open
|
@normj, could you please check this PR? |
martincostello
removed
the
keep-open
Fixes #750
Changes
Detect container ID for Windows ECS containers.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changesChanges in public API reviewed (if applicable)