Add opentelemetry-instrumentation by ocelotl · Pull Request #1959 · open-telemetry/opentelemetry-python

ocelotl · 2021-07-15T22:08:54Z

Fixes #1958

docs-requirements.txt

codeboten

Looks pretty good overall, one question about dependencies that don't seem necessary.

codeboten

It looks like this change is mixing the release process of updating version numbers with the change to move opentelemetry-instrumentation.

I would recommend reverting the change for version numbers first, then moving this package over, and then creating the release. Otherwise I worry we may miss something.

docs/examples/error_handler/error_handler_0/src/error_handler_0/version.py

Fixes open-telemetry#1958

NathanielRN · 2021-07-16T18:03:09Z

docs-requirements.txt

-./opentelemetry-python-contrib/opentelemetry-instrumentation
+./opentelemetry-instrumentation
 ./opentelemetry-sdk
+./opentelemetry-instrumentation


Probably need to delete this line?

NathanielRN · 2021-07-16T18:03:30Z

CHANGELOG.md

  ([#1946](https://github.com/open-telemetry/opentelemetry-python/pull/1946))

 ### Added
+- Moved `opentelemetry-instrumentation` to core repository.


Suggested change

- Moved `opentelemetry-instrumentation` to core repository.

- Moved `opentelemetry-instrumentation` back to core repository from contrib.

NathanielRN · 2021-07-16T18:05:28Z

eachdist.ini

 sortfirst=
    opentelemetry-api
    opentelemetry-sdk
+    opentelemetry-instrumentation


Shouldn't this go above opentelemetry-sdk since it depends on instrumentation?

NathanielRN · 2021-07-16T18:08:36Z

docs/conf.py

 settings.configure()

+
+source_dirs = [


I know this is what this was called before but we should change the name:

Suggested change

source_dirs = [

instr_source_dir = [

NathanielRN · 2021-07-16T18:08:45Z

docs/conf.py

 ]

-sys.path[:0] = exp_dirs + shim_dirs
+sys.path[:0] = source_dirs + exp_dirs + shim_dirs


Suggested change

sys.path[:0] = source_dirs + exp_dirs + shim_dirs

sys.path[:0] = instr_source_dir + exp_dirs + shim_dirs

NathanielRN · 2021-07-16T18:10:20Z

scripts/build.sh

  rm -rf $DISTDIR/*

- for d in opentelemetry-api/ opentelemetry-sdk/ opentelemetry-proto/ opentelemetry-distro/ opentelemetry-semantic-conventions/ exporter/*/ shim/*/ propagator/*/; do
+ for d in opentelemetry-api/ opentelemetry-sdk/ opentelemetry-instrumentation/ opentelemetry-proto/ opentelemetry-distro/ opentelemetry-semantic-conventions/ exporter/*/ shim/*/ propagator/*/; do


Does instrumentation need to go before SDK?

Suggested change

for d in opentelemetry-api/ opentelemetry-sdk/ opentelemetry-instrumentation/ opentelemetry-proto/ opentelemetry-distro/ opentelemetry-semantic-conventions/ exporter/*/ shim/*/ propagator/*/; do

for d in opentelemetry-api/ opentelemetry-instrumentation/ opentelemetry-sdk/ opentelemetry-proto/ opentelemetry-distro/ opentelemetry-semantic-conventions/ exporter/*/ shim/*/ propagator/*/; do

NathanielRN · 2021-07-16T18:11:33Z

tox.ini

  python -m pip install -e {toxinidir}/opentelemetry-api[test]
  python -m pip install -e {toxinidir}/opentelemetry-semantic-conventions[test]
-  python -m pip install {toxinidir}/opentelemetry-python-contrib/opentelemetry-instrumentation
+  python -m pip install -e {toxinidir}/opentelemetry-instrumentation[test]


It doesn't look like opentelemetry-instrumentation extra test has anything?

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/08daa9d194ee9484fcee506f145901e551aecd10/opentelemetry-instrumentation/setup.cfg#L55-L56

NathanielRN · 2021-07-16T18:11:56Z

tox.ini

-commands_pre =
-  python -m pip install {toxinidir}/opentelemetry-python-contrib/opentelemetry-instrumentation
+commands-pre =
+  python -m pip install {toxinidir}/opentelemetry-instrumentation


Suggested change

python -m pip install {toxinidir}/opentelemetry-instrumentation

pip install {toxinidir}/opentelemetry-instrumentation

We should have pip installed:

opentelemetry-python/tox.ini

Line 118 in dfd2980

py3{6,7,8,9}: python -m pip install -U pip setuptools wheel

ocelotl · 2021-07-16T18:15:34Z

Sorry, @NathanielRN, we got this merged. Please consider opening an issue for your comments 👍

NathanielRN · 2021-07-16T18:20:14Z

@ocelotl I think there are quite a few improvements we can make to this PR. I also think we should address these changes before we go with the release, otherwise these changes will be too far removed in Git History...

ocelotl force-pushed the issue_1958 branch 4 times, most recently from 16a5448 to 7e2bfd7 Compare July 15, 2021 22:25

codeboten reviewed Jul 15, 2021

View reviewed changes

docs-requirements.txt Outdated Show resolved Hide resolved

codeboten reviewed Jul 15, 2021

View reviewed changes

ocelotl force-pushed the issue_1958 branch 2 times, most recently from 220ea1e to cdda41e Compare July 15, 2021 23:02

ocelotl self-assigned this Jul 15, 2021

ocelotl added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label Jul 15, 2021

ocelotl force-pushed the issue_1958 branch 4 times, most recently from 49da963 to bdc9a4f Compare July 15, 2021 23:49

ocelotl requested a review from codeboten July 15, 2021 23:50

codeboten suggested changes Jul 16, 2021

View reviewed changes

docs/examples/error_handler/error_handler_0/src/error_handler_0/version.py Outdated Show resolved Hide resolved

ocelotl mentioned this pull request Jul 16, 2021

Revert version updates #1961

Closed

ocelotl force-pushed the issue_1958 branch from bdc9a4f to baa36b8 Compare July 16, 2021 16:14

lzchen mentioned this pull request Jul 16, 2021

Make it explicit to install distro to get auto-instrumentation working open-telemetry/opentelemetry-python-contrib#588

Merged

ocelotl force-pushed the issue_1958 branch 2 times, most recently from 951de73 to 115a51c Compare July 16, 2021 16:34

Add opentelemetry-instrumentation

1e6c1be

Fixes open-telemetry#1958

ocelotl force-pushed the issue_1958 branch from 115a51c to 1e6c1be Compare July 16, 2021 17:46

ocelotl marked this pull request as ready for review July 16, 2021 17:58

ocelotl requested review from a team, aabmass, codeboten, lzchen and owais and removed request for a team July 16, 2021 17:58

ocelotl requested review from codeboten and removed request for aabmass, codeboten, lzchen and owais July 16, 2021 18:07

codeboten approved these changes Jul 16, 2021

View reviewed changes

lzchen approved these changes Jul 16, 2021

View reviewed changes

lzchen merged commit e78c9c4 into open-telemetry:main Jul 16, 2021

ocelotl deleted the issue_1958 branch July 16, 2021 18:13

NathanielRN suggested changes Jul 16, 2021

View reviewed changes

	- Moved `opentelemetry-instrumentation` to core repository.
	- Moved `opentelemetry-instrumentation` back to core repository from contrib.

	sys.path[:0] = source_dirs + exp_dirs + shim_dirs
	sys.path[:0] = instr_source_dir + exp_dirs + shim_dirs

	for d in opentelemetry-api/ opentelemetry-sdk/ opentelemetry-instrumentation/ opentelemetry-proto/ opentelemetry-distro/ opentelemetry-semantic-conventions/ exporter// shim// propagator/*/; do
	for d in opentelemetry-api/ opentelemetry-instrumentation/ opentelemetry-sdk/ opentelemetry-proto/ opentelemetry-distro/ opentelemetry-semantic-conventions/ exporter// shim// propagator/*/; do

	python -m pip install {toxinidir}/opentelemetry-instrumentation
	pip install {toxinidir}/opentelemetry-instrumentation

Conversation

ocelotl commented Jul 15, 2021

Uh oh!

Uh oh!

codeboten left a comment

Choose a reason for hiding this comment

Uh oh!

codeboten left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ocelotl commented Jul 16, 2021

Uh oh!

NathanielRN commented Jul 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants