Improved ECS attribute and origin translation in awsxrayexporter #1428
Conversation
willarmiros
requested review from
anuraaga,
james-bebbington and
jrcamp
as code owners
| @@ -61,6 +71,8 @@ func makeAws(attributes map[string]string, resource pdata.Resource) (map[string] | |||
| switch key { | |||
| case semconventions.AttributeCloudProvider: | |||
| cloud = value.StringVal() | |||
| case "cloud.infrastructure_service": | |||
There was a problem hiding this comment.
As these are destined to be replaced by constants anyways, I'd go ahead and have private constants like AttributeCloudInfrastructureService defined in this file
There was a problem hiding this comment.
That's fair, will replace
| @@ -163,6 +196,14 @@ func makeAws(attributes map[string]string, resource pdata.Resource) (map[string] | |||
| } | |||
| } | |||
|
|
|||
| // Since we must couple log group ARNs and Log Group Names in the same CWLogs object, we first try to derive the | |||
| // names from the ARN, then fall back to just recording the names | |||
| if logGroupArns != (pdata.AnyValueArray{}) && logGroupArns.Len() > 0 { | |||
There was a problem hiding this comment.
Do we need this != even though we have a len check?
There was a problem hiding this comment.
Yeah, I initially didn't have it and it turns out the Len() method relies on an underlying pointer orig in the AnyValueArray struct, so if we don't initialize logGroupArns we get a nil pointer exception. I could check for nilness of orig, but it seemed like an implementation detail that I shouldn't be referencing directly.
|
|
||
| // TODO(willarmiros): Only use infrastructure_service for origin resolution once detectors for all AWS environments are | ||
| // implemented for robustness | ||
| is, present := resource.Attributes().Get("cloud.infrastructure_service") |
There was a problem hiding this comment.
I think you can inline, e.g., if is, ok := resource...; ok {
| @@ -92,10 +91,10 @@ func (d *Detector) Detect(context.Context) (pdata.Resource, error) { | |||
| // The launch type and log data attributes are only available in TMDE v4 | |||
| switch lt := strings.ToLower(tmdeResp.LaunchType); lt { | |||
| case "ec2": | |||
| attr.InsertString("aws.ecs.launchtype", "EC2") | |||
| attr.InsertString("aws.ecs.launchtype", "ec2") | |||
There was a problem hiding this comment.
If it's possible, it'd generally be better to have separate PRs for this processor and the exporter
There was a problem hiding this comment.
Sorry, definitely makes sense. In this case I realized some issues with the detector as I was making the exporter changes, and made them side-by-side. In the future I will definitely take care to separate PRs by module.
Codecov Report
@@ Coverage Diff @@
## master #1428 +/- ##
==========================================
- Coverage 88.72% 88.69% -0.03%
==========================================
Files 344 344
Lines 16844 16918 +74
==========================================
+ Hits 14944 15005 +61
- Misses 1434 1446 +12
- Partials 466 467 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@willarmiros please resolve the conflicts. |
|
@tigrannajaryan done, sorry for the delay |
|
Looks like open-telemetry/opentelemetry-collector#2039 removed some APIs that broke this PR and #1360 which is already merged. I'll get them fixed so the tests pass. |
…1428) * adding codeql workfklow * removing PR and commit triggers * updating changelog * removing push trigger Co-authored-by: Azfaar Qureshi <[email protected]>
Description
Follows up #1360 by translating the relevant ECS attributes from their OTel versions to X-Ray Segment attributes within the
aws.ecsblock. Also added the two new ECS origins that are derived from the ECS Launch Type. I updated the origin resolution logic to prefer thecloud.infrastructure_serviceattribute when available instead of guessing the cloud service type based on other predefined attributes. This PR also introduces thecloudwatch_logsfield to the AWS translation model, since they are recorded in the ECS resource detector with other services planned.I will replace all the strings for resource attribute names with more officially defined constants once open-telemetry/opentelemetry-specification#1112 and open-telemetry/opentelemetry-specification#1099 are merged.
Minor changes
cloud.infrastructure_serviceattribute in the EC2 resource detector. I'll avoid adding it to non-AWS detectors til it's an official attributeTesting: Added unit tests/updated existing ones. Also tested end-to-end with a sample ECS app publishing to X-Ray.
Documentation: Updated resource detector Readme for its minor changes.