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

Removes Instrumentation and Exporter Code From Core Repo #1258

Merged
merged 4 commits into from
Nov 5, 2020

Conversation

NathanielRN
Copy link
Contributor

@NathanielRN NathanielRN commented Oct 19, 2020

Description

Removes code that will be destined for the Contrib Repository as outlined in this PR.

Fixes #1233

Type of change

Moving files to another repo without breaking API calls

How Has This Been Tested?

The Contrib repo has a CI build to validate changes there use the APIs and SDK here.

Checklist:

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

@codeboten codeboten added the release:required-for-ga To be resolved before GA release label Oct 20, 2020
@NathanielRN
Copy link
Contributor Author

This PR will be closed in favor of several smaller PRs!

@codeboten codeboten added the rc1 label Oct 29, 2020
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
@NathanielRN NathanielRN force-pushed the remove-contrib-files branch 2 times, most recently from 4073ddb to be1ec92 Compare November 2, 2020 21:54
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.

This PR is looking pretty close, just a couple of questions in the tox.ini changes

-e {toxinidir}/instrumentation/opentelemetry-instrumentation-wsgi \
-e {toxinidir}/instrumentation/opentelemetry-instrumentation-flask
-e {toxinidir}/opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-requests \
-e {toxinidir}/opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-wsgi
Copy link
Contributor

Choose a reason for hiding this comment

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

is flask missing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it because I noticed that tests/w3c_tracecontext_validation_server.py didn't use opentelemetry.instrumentation.flask. Not sure if it uses it in auto-instrumentation but I will experiment with it today to find out if it's needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out its not needed!

tox.ini Outdated Show resolved Hide resolved
tox.ini Show resolved Hide resolved
@NathanielRN NathanielRN force-pushed the remove-contrib-files branch 17 times, most recently from ff3802f to 4345c41 Compare November 3, 2020 23:39
docs/conf.py Outdated Show resolved Hide resolved
-e {toxinidir}/instrumentation/opentelemetry-instrumentation-wsgi \
-e {toxinidir}/instrumentation/opentelemetry-instrumentation-flask
-e {toxinidir}/opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-requests \
-e {toxinidir}/opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-wsgi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out its not needed!

tox.ini Show resolved Hide resolved
@NathanielRN NathanielRN force-pushed the remove-contrib-files branch 3 times, most recently from bda99f5 to 0c38760 Compare November 4, 2020 01:49
@NathanielRN NathanielRN marked this pull request as ready for review November 4, 2020 02:20
@NathanielRN NathanielRN requested review from a team, codeboten and hectorhdzg and removed request for a team November 4, 2020 02:20
.github/workflows/test.yml Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
@NathanielRN NathanielRN requested a review from aabmass November 4, 2020 16:20
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

LGTM

@NathanielRN NathanielRN force-pushed the remove-contrib-files branch 6 times, most recently from 3a5aa01 to b0eb8f0 Compare November 4, 2020 22:07
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, thanks for all the work to get this done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:required-for-ga To be resolved before GA release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move non-core instrumentations/exporters/propagators out of main repo
3 participants