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.

Sign up for GitHub

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

Jump to bottom

Implement aws.ecs.* resource attributes #1212

Merged
merged 13 commits into from
Jan 11, 2023
Merged

Implement aws.ecs.* resource attributes #1212

merged 13 commits into from
Jan 11, 2023

Conversation

mmanciop
Copy link
Contributor

@mmanciop mmanciop commented Aug 4, 2022 •
edited
Loading

Description

Implement the experimental AWS ECS resource attributes using the Metadata v4 Endpoint.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Unit tests

General logic on Amazon ECS has been extensively tested by Lumigo in their OpenTelemetry Python Distro, which has shipped this detection logic for some time already.

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated <-- There seems to be no need of that

@mmanciop mmanciop requested a review from a team August 4, 2022 20:16
@github-actions github-actions bot requested a review from NathanielRN August 4, 2022 20:16
@mmanciop mmanciop changed the title [WIP] Implement aws.ecs.* resource attributes Implement aws.ecs.* resource attributes Aug 5, 2022
@mmanciop mmanciop force-pushed the aws_ecs_detector branch 5 times, most recently from 479314e to fd7aa97 Compare August 9, 2022 15:09
@mmanciop
Copy link
Contributor Author

mmanciop commented Aug 12, 2022 •
edited
Loading

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 mmanciop marked this pull request as draft August 12, 2022 15:31
@mmanciop mmanciop marked this pull request as ready for review August 12, 2022 16:02
@mmanciop
Copy link
Contributor Author

Update: The uncertainty about ARNs seems sorted out

@Kausik-A
Copy link
Contributor

As done in the opentelemetry-go-contrib can we add we add test cases that utilize the fargate examples responses?

@Kausik-A
Copy link
Contributor

Kausik-A commented Aug 16, 2022 •
edited
Loading

Running the ECS UNIT tests ( test_ecs.py ) seems to fail for test_ecs.AwsEcsResourceDetectorTest. This is what I get AttributeError: <module 'opentelemetry.sdk.extension.aws.resource.ecs' from '/usr/local/lib/python3.9/site-packages/opentelemetry/sdk/extension/aws/resource/ecs.py'> does not have the attribute '_http_get'
Can you run them again locally and lmk if we have to follow any specified steps for the UNIT tests to pass.

@mmanciop
Copy link
Contributor Author

Running the ECS UNIT tests ( test_ecs.py ) seems to fail for test_ecs.AwsEcsResourceDetectorTest. This is what I get AttributeError: <module 'opentelemetry.sdk.extension.aws.resource.ecs' from '/usr/local/lib/python3.9/site-packages/opentelemetry/sdk/extension/aws/resource/ecs.py'> does not have the attribute '_http_get' Can you run them again locally and lmk if we have to follow any specified steps for the UNIT tests to pass.

@Kausik-A, I am running the tests locally with tox -e py39-test-sdkextension-aws, which is how I believe the CI runs them. When running pytest, there is a whole bunch of entirely-unrelated tests that fail, so I am guessing plain pytest is not really supported.

@mmanciop
Copy link
Contributor Author

As done in the opentelemetry-go-contrib can we add we add test cases that utilize the fargate examples responses?

Done in d945a61

@mmanciop
Copy link
Contributor Author

@Kausik-A I squashed, rebased and run the tests again; care to have a look?

@mmanciop mmanciop force-pushed the aws_ecs_detector branch 2 times, most recently from 51bf4ad to 2de26c8 Compare September 28, 2022 16:17
@mmanciop
Copy link
Contributor Author

mmanciop commented Sep 28, 2022 •
edited
Loading

@Kausik-A fixed the linter warnings and rebased due to changelog conflicts

@srikanthccv
Copy link
Member

@Kausik-A would you be willing to add yourself to CODEOWNERS for this package since @NathanielRN hasn't been very active here?

@Kausik-A
Copy link
Contributor

Kausik-A commented Oct 5, 2022

@Kausik-A fixed the linter warnings and rebased due to changelog conflicts

@mmanciop thank you for the update.Let me have another pass at it

@Kausik-A
Copy link
Contributor

Kausik-A commented Oct 5, 2022

@Kausik-A would you be willing to add yourself to CODEOWNERS for this package since @NathanielRN hasn't been very active here?

@srikanthccv are you referring to being codeowners for opentelemetry-sdk-extension-aws?

@srikanthccv
Copy link
Member

Yes, then your review counts into merge requirements.

@Kausik-A
Copy link
Contributor

Kausik-A commented Oct 5, 2022

Yeah, I can do that

@mmanciop
Copy link
Contributor Author

@Kausik-A have a look please?

@Kausik-A
Copy link
Contributor

LGTM! @srikanthccv can you take a look at this permission-request-PR so that I can approve this?

@Kausik-A
Copy link
Contributor

@srikanthccv can you approve the running workflow for this PR? I still can't seem to add my review.

@srikanthccv
Copy link
Member

You can leave your approval review on the changes.

Michele Mancioppi added 3 commits October 22, 2022 18:49
Implement aws.ecs.* and aws.logs.* resource attributes
693af66 
Add support for the aws.ecs.* and aws.logs.* resource
attributes in the `AwsEcsResourceDetector` detector when
the ECS Metadata v4 is available
lint
f5be8cd
lint some more
fbe2c80
@mmanciop
Copy link
Contributor Author

@Kausik-A I fixed hopefully the last linting issue

@mmanciop mmanciop mentioned this pull request Oct 29, 2022
Merged
@ocelotl
Copy link
Contributor

ocelotl commented Nov 16, 2022

@mmanciop please fix the conflicts ✌️

@mmanciop
Copy link
Contributor Author

@ocelotl done

@mmanciop
Copy link
Contributor Author

@ocelotl @NathanielRN @Aneurysm9

Michele Mancioppi and others added 3 commits January 11, 2023 21:57
@Aneurysm9
Update sdk-extension/opentelemetry-sdk-extension-aws/src/opentelemetr…
909513c 
…y/sdk/extension/aws/resource/ecs.py

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
@Aneurysm9
Update sdk-extension/opentelemetry-sdk-extension-aws/src/opentelemetr…
309245f 
…y/sdk/extension/aws/resource/ecs.py

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
Update CHANGELOG.md
1ae794d
@srikanthccv srikanthccv enabled auto-merge (squash) January 11, 2023 21:00
@srikanthccv srikanthccv merged commit bf9a8e8 into open-telemetry:main Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srikanthccv srikanthccv srikanthccv approved these changes

@Aneurysm9 Aneurysm9 Aneurysm9 approved these changes

@NathanielRN NathanielRN Awaiting requested review from NathanielRN

Assignees

@NathanielRN NathanielRN

@Kausik-A Kausik-A

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mmanciop @Kausik-A @srikanthccv @ocelotl @Aneurysm9 @NathanielRN