feat(aws): add EC2, ECS and EKS resource detectors - #721
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #721 +/- ##
=======================================
- Coverage 79.7% 71.4% -8.3%
=======================================
Files 133 136 +3
Lines 28369 28621 +252
=======================================
- Hits 22623 20452 -2171
- Misses 5746 8169 +2423
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
It's well-built code at this scale. Thanks for that! Thorough docs, unit tests and live testing |
My leaning is to downgrade to debug, what do you think? But if we are on the platform and a call that should've worked fails, that still feels worth a warn
We can keep IMDS in the ECS/EKS detectors, seems safe without having to add another detector, but I'll need to analyze this more
v2-only should be fine. AWS is pushing v2 as the default. @cijothomas could answer more if there are any OTel standards. Also, I'd keep /latest/. Is there a specific field you're worried about? |
I think an empty data can look like real data and break things downstream, unlike null so since these are utils and a few small re-used helpers (with tests) - we should be good to keep
I looked at how the other SDKs do it and Go uses the full client-go library, but Python and JS both just hit the k8s API directly over plain HTTPS with the service-account toke. So, curious to know how quantitative are the benefits of avoiding the kubernetes client dependency? I'm good with your lighter-weight approach, just wanna understand more. Also, right now the platform tie is AWS_REGION (eks.rs:170, eks.rs:185) and AWS_REGION is set by pretty much any app which will use a AWS SDK, so what if a GKE/AKS pod reads from S3 and gets labelled as cloud.platform=aws_eks? Let's think about both happy cases and edge cases
Let's keep it separate, what do you think? This PR is already big |
|
As you work on the re-factor, the only red check is codecov currently - can we add more mockable scenarios? |
Fixes #705
Fixes #706
Part of #707
Part of #94
Note
Marked as a draft on purpose. The code works (see "Testing" below), but several design points listed under "Discussion / open questions" need maintainer input before this is ready for final review.
I'll be AFK for the next 2 weeks and will address any and all remarks upon returning.
Changes
Adds three AWS resource detectors to the
opentelemetry-awscrate, each behind its own feature flag:Ec2ResourceDetector(detector-aws-ec2) — queries IMDSv2 forcloud.*andhost.*attributes.EcsResourceDetector(detector-aws-ecs) — reads the ECS container metadata endpoint v4 (ECS_CONTAINER_METADATA_URI_V4) and IMDSv2 on EC2 workers forcloud.*,aws.ecs.*,container.*andaws.log.*attributes.EksResourceDetector(detector-aws-eks) — detects a pod via theservice-account namespace file and ties it to AWS via IMDSv2, reporting
cloud.*,k8s.*,aws.eks.cluster.arn,container.idand nodehost.*.Supporting changes:
_detector/_detector-httpto share deps.ureq(blocking HTTP client —ResourceDetector::detectis synchronous). Empty attribute values are filtered out; errors are best-effort
logged (gated by
internal-logs) and the attribute is skipped.LambdaResourceDetectoris intentionally untouched.Usage
Discussion / open questions
I'd like maintainer input on the following before finalizing:
single warning log line (disableable by dropping the internal-logs
feature). Pro: surfaces likely misconfiguration cheaply. Con: an app that
legitimately runs on multiple platforms (e.g. EC2 and ECS) and enables
several detectors will always warn for the ones that don't match. Keep,
downgrade to debug, or drop?
IMDSv2 to enrich with node info, which does not work on Fargate. Should these
detectors instead focus only on ECS/EKS-specific sources and leave node
detection to the EC2 detector (user opts in by also enabling it)? If so, how
should cloud.platform be resolved when several detectors are chained —
relying on detector order is poor API design; is there a better contract?
wanted? Relatedly, endpoints use /latest/; should a pinned, dated IMDS
version be used instead?
avoid populating keys with empty strings. Is the added code worth it, or is
it acceptable to emit empty values?
(read aws-auth / cluster-info configmaps, container.id from cgroup).
This PR instead uses the service-account namespace file + IMDSv2
aws:eks:cluster-name instance tag + downward-API env vars, which avoids a
Kubernetes client dependency but requires instance-tags-in-metadata to be
enabled (or AWS_CLUSTER_NAME to be set) for k8s.cluster.name. Is this
trade-off acceptable, or should it follow [Feature]: AWS EKS resource detector #707's configmap approach?
attributes with unwrap_or_default(), producing empty values on error —
inconsistent with the empty-filtering the new detectors do. Should a small
refactor be included in this PR, or kept separate?
Testing
Unit tests cover the pure logic (arch mapping, value filtering, ARN parsing, etc.).
End-to-end behavior was verified by running a probe program on real AWS (EC2, ECS, EKS). See: https://github.com/RustyServerless/opentelemetry-rust-contrib/tree/wip/aws-resource-detectors/otel-aws-probe-deploy
Merge requirement checklist