Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ResourceDetectors.AWS] Implement support for some cloud.* attributes in AWS ECS detector #1552

Merged
merged 4 commits into from
Feb 1, 2024

Conversation

mmanciop
Copy link
Contributor

Changes

Implement support for the following resource attributes in the AWS ECS detector when using the metadata endpoint v4:

  • cloud.account.id
  • cloud.availability_zone
  • cloud.region

Also, fix mispelling of the AWSSemanticConventions.AttributeCloudAvailableZone to AWSSemanticConventions.AttributeCloudAvailabilityZone

  • Appropriate CHANGELOG.md updated for non-trivial changes

@mmanciop mmanciop requested a review from a team January 29, 2024 15:41
Copy link

codecov bot commented Jan 29, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (71655ce) 73.91% compared to head (91c1941) 80.66%.
Report is 132 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1552      +/-   ##
==========================================
+ Coverage   73.91%   80.66%   +6.75%     
==========================================
  Files         267      113     -154     
  Lines        9615     3083    -6532     
==========================================
- Hits         7107     2487    -4620     
+ Misses       2508      596    -1912     
Flag Coverage Δ
unittests-Solution 80.66% <88.57%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...Telemetry.Exporter.InfluxDB/InfluxDBEventSource.cs 60.00% <ø> (ø)
...ry.Exporter.InfluxDB/InfluxDBExporterExtensions.cs 100.00% <100.00%> (ø)
...metry.Exporter.InfluxDB/InfluxDBMetricsExporter.cs 84.61% <ø> (-3.85%) ⬇️
...Telemetry.Exporter.InfluxDB/PointDataExtensions.cs 87.50% <100.00%> (ø)
...ry.Exporter.InfluxDB/TelegrafPrometheusWriterV1.cs 100.00% <ø> (ø)
...ry.Exporter.InfluxDB/TelegrafPrometheusWriterV2.cs 100.00% <ø> (ø)
...stana/Implementation/InstanaExporterEventSource.cs 0.00% <ø> (ø)
...try.Exporter.Instana/Implementation/InstanaSpan.cs 100.00% <ø> (ø)
...orter.Instana/Implementation/InstanaSpanFactory.cs 100.00% <ø> (ø)
...er.Instana/Implementation/InstanaSpanSerializer.cs 93.79% <ø> (ø)
... and 31 more

... and 225 files with indirect coverage changes

@mmanciop mmanciop force-pushed the aws-ecs-attrs branch 2 times, most recently from f66b4bb to 60fb095 Compare January 29, 2024 16:17
…ability_zone,region} in AWS ECS detector

Implement support for the following resource attributes in
the AWS ECS detector when using the metadata endpoint v4:

* cloud.account.id
* cloud.availability_zone
* cloud.region

Also, fix mispelling of the AWSSemanticConventions.AttributeCloudAvailableZone
to AWSSemanticConventions.AttributeCloudAvailabilityZone
@mmanciop
Copy link
Contributor Author

The build error looks like something that is on main:

Run markdownlint .
src/OpenTelemetry.ResourceDetectors.Azure/README.md:44:231 MD056/table-column-count Table column count [Expected: 2; Actual: 3; Too many cells, extra data will be missing]
Error: Process completed with exit code 1.

@Kielek Kielek added the comp:resources.aws Things related to OpenTelemetry.Resources.AWS label Jan 30, 2024
@Kielek
Copy link
Contributor

Kielek commented Jan 30, 2024

@ppittle, @normj, @birojnayak, please review

@martincostello martincostello mentioned this pull request Jan 30, 2024
3 tasks
Copy link
Contributor

@birojnayak birojnayak left a comment

Choose a reason for hiding this comment

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

looks good to me.. thank you for adding additional details.

Copy link
Contributor

@normj normj left a comment

Choose a reason for hiding this comment

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

Looks good

@Kielek
Copy link
Contributor

Kielek commented Jan 31, 2024

@mmanciop, please update also README.md. There is a list of detected resource.

@mmanciop
Copy link
Contributor Author

@mmanciop, please update also README.md. There is a list of detected resource.

Done in 4ead9fd, and I retrofitted the update I forgot back when with #875. Although I gotta say, this format that does not neither mentions the actual keys, nor links to the specs of the semconvs, does not strike me as particularly useful.

@Kielek
Copy link
Contributor

Kielek commented Feb 1, 2024

@mmanciop, please update also README.md. There is a list of detected resource.

Done in 4ead9fd, and I retrofitted the update I forgot back when with #875. Although I gotta say, this format that does not neither mentions the actual keys, nor links to the specs of the semconvs, does not strike me as particularly useful.

Other documentation relays on exact keys: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/main/src/OpenTelemetry.ResourceDetectors.ProcessRuntime/README.md

It can be fixed as separate PR. Merging.

@Kielek Kielek merged commit a066abb into open-telemetry:main Feb 1, 2024
32 checks passed
@mmanciop mmanciop deleted the aws-ecs-attrs branch February 1, 2024 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:resources.aws Things related to OpenTelemetry.Resources.AWS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants