Implement aws.ecs.* resource attributes

[mmanciop]

Implemented retrieving the aws.ecs.* resource attributes in go.opentelemetry.io/detectors/aws/ecs based on the ECS Metadata v4 endpoint.

[codecov]

Codecov Report

Merging #2626 (f3c4f53) into main (15ceaa2) will decrease coverage by 0.7%.
The diff coverage is 0.0%.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #2626     +/-   ##
=======================================
- Coverage   69.7%   68.9%   -0.8%     
=======================================
  Files        147     147             
  Lines       6806    6875     +69     
=======================================
- Hits        4746    4743      -3     
- Misses      1944    2014     +70     
- Partials     116     118      +2     

Impacted Files	Coverage Δ
detectors/aws/ecs/ecs.go	18.0% <0.0%> (-36.8%)	⬇️

[mmanciop]

There's discussion on open-telemetry/opentelemetry-java#4574 (comment) whether the ARNs like the one this PR builds are actually correct. Until we are use, let's not merge :D

[mmanciop]

Update: The uncertainty about ARNs seems sorted out

[bryan-aguilar]

Can we add test cases that utilize the fargate examples responses also?
Edit: If these are the same we can disregard. I am still digging into the differences if any, and whether or not these additional test cases would help.

[bryan-aguilar]

Looks like there may be slight differences between fargate and ec2 metadata response. As a sanity check can we add those example responses linked in the comment above to the unit tests?

[mmanciop]

Looks like there may be slight differences between fargate and ec2 metadata response. As a sanity check can we add those example responses linked in the comment above to the unit tests?

Done in 2ef2026. There are no factual differences that this patch cares about.

[bryan-aguilar]

From the ADOT team perspective this looks good to go.
Any long term concerns with using AWS ECS Metadata Go as it describes itself being built for fargate services?

[Aneurysm9]

Suggested change

	semconv.AWSLogGroupNamesKey.String(logsOptions.AwsLogsGroup),
	semconv.AWSLogGroupARNsKey.String(fmt.Sprintf("arn:aws:logs:%s:%s:log-group:%s:*", logsRegion, awsAccount, logsOptions.AwsLogsGroup)),
	semconv.AWSLogStreamNamesKey.String(logsOptions.AwsLogsStream),
	semconv.AWSLogStreamARNsKey.String(fmt.Sprintf("arn:aws:logs:%s:%s:log-group:%s:log-stream:%s", logsRegion, awsAccount, logsOptions.AwsLogsGroup, logsOptions.AwsLogsStream)),
	semconv.AWSLogGroupARNsKey.StringSlice([]string{fmt.Sprintf("arn:aws:logs:%s:%s:log-group:%s:*", logsRegion, awsAccount, logsOptions.AwsLogsGroup)}),
	semconv.AWSLogStreamARNsKey.StringSlice([]string{fmt.Sprintf("arn:aws:logs:%s:%s:log-group:%s:log-stream:%s", logsRegion, awsAccount, logsOptions.AwsLogsGroup, logsOptions.AwsLogsStream)}),

These are duplicative, so I'd suggest only including the ARN as the name can be extracted from it.

The values are expected to be string[].

[mmanciop]

String slices introduced in 4e857dd. I am not sold on skipping attributes though, see #2626 (comment).

[mmanciop]

@Aneurysm9 about the duplication of Log Group|Stream Name|ARN attributes, I implemented the spec: https://opentelemetry.io/docs/reference/specification/resource/semantic_conventions/cloud_provider/aws/logs/ . They may be duplicative (provided that the backend behind does processing, which is not the case with some OSS backends that have only limited query capabilities), but at the end of the day, if we wanna drop them, I'd rather first they get dropped from the spec.

[Aneurysm9]

All of the log group/stream fields are optional. There is no requirement to include all of them if any of them are included.

[mmanciop]

All of the log group/stream fields are optional. There is no requirement to include all of them if any of them are included.

IMO, this would wreak havoc with less sophisticated tracing backends that do not have processing capabilities to normalize the attributes, as the same Log stream or group could be represented with different values.

[mmanciop]

Rebased on main, solved a bunch of merge conflicts; @Aneurysm9 care to have a look?

[Aneurysm9]

@mmanciop This requires an additional approval from @open-telemetry/go-approvers before it can be merged.

[dmathieu]

This repository has 3 stars and 14 commits. I wonder how reliable it can be in the future.
It's also fairly simple. So we can also extract its content and bring it back into this package later on if necessary.

[mmanciop]

I was committing fixed to that repo. The maintainer was nice and responsive. If worse comes to worse, can always be incorporated here.

[mmanciop]

@Aneurysm9 does this ring a bell?

panic: httptest: failed to listen on a port: listen tcp6 [::1]:0: socket: The requested service provider could not be loaded or initialized. [recovered]
panic: httptest: failed to listen on a port: listen tcp6 [::1]:0: socket: The requested service provider could not be loaded or initialized.

I copied the test code from the Ec2Detector and the tests pass on my Mac; am I perhaps missing something in the CI configs?

(The fact that the CI does not trigger every time I push a commit makes it is even more daunting to troubleshoot...)