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

Move aiohttp-client instrumentation package to Contrib Repo #1261

Closed

Conversation

NathanielRN
Copy link
Contributor

@NathanielRN NathanielRN commented Oct 21, 2020

Description

Follow up to open-telemetry/opentelemetry-python-contrib#49

How Has This Been Tested?

CI has been updated to remove package tests (since they will live in the Contrib repo instead)

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated (Is this needed?)
    - [ ] Unit tests have been added
    - [ ] Documentation has been updated

@NathanielRN NathanielRN requested review from a team, toumorokoshi and ocelotl and removed request for a team October 21, 2020 06:03
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Please update the title to reflect that this instrumentation is moving to the contrib repo, will make it easier for people looking for it :)

@NathanielRN NathanielRN changed the title Remove aiohttp-client instrumentation package Move aiohttp-client instrumentation package to Contrib Repo Oct 21, 2020
@NathanielRN
Copy link
Contributor Author

@codeboten Sounds good, updated!

@NathanielRN NathanielRN requested a review from codeboten October 21, 2020 17:41
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

I noticed the docs entry for this was removed but not re-introduced in the contrib repo, anby thoughts on the path forward here is?

@NathanielRN
Copy link
Contributor Author

@codeboten I was thinking about that too and was going to bring it up in the SIG meeting. I was going to update this PR to remove the non instrumentation/opentelemtery-instrumentation-aiohttp-client affected files and remove them later. That way we can have just 2 PRs: One in the Contrib to add the references to all Contrib packages outside of their directories and one in the Core to remove the references to Contrib package directories.

Let me know what you think of that! It will make the “remove” PRs in the Core repo here as straightforward as the Contrib ones. And it’s the solution I prefer. Because an extra commit to remove them would have to be reviewed carefully I would think, but with the 2 PRs we can directly compre the differences.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Blocking on merging this until the contrib repo is running CI and we have all the tools over there.

@NathanielRN
Copy link
Contributor Author

Going to close this PR because based on the SIG meeting today we agreed that once we have CI tests passing on Contrib we can just delete all the packages in one PR here.

@NathanielRN NathanielRN deleted the remove-aiohttp-client branch November 2, 2020 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants