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

refactor: Add common utils to opentelemetry-auto-instrumentation, rename opentelemetry-auto-instrumentation #741

Merged
merged 11 commits into from
Jun 8, 2020
Merged

refactor: Add common utils to opentelemetry-auto-instrumentation, rename opentelemetry-auto-instrumentation #741

merged 11 commits into from
Jun 8, 2020

Conversation

cnnradams
Copy link
Member

@cnnradams cnnradams commented May 27, 2020 •
edited
Loading

TL;DR:

  • opentelemetry-auto-instrumentation package was renamed to opentelemetry-instrumentation
  • opentelemetry-auto-instrumentation executable was renamed to opentelemetry-instrument (probably controversial)
  • Added utils file to opentelemetry-instrumentation, use it in instrumentations

Resolves #735, resolves #610, resolves #667, resolves #608

@cnnradams cnnradams requested a review from a team May 27, 2020 18:31
@cnnradams cnnradams force-pushed the utils-package branch 5 times, most recently from 14ad082 to 29a6d17 Compare May 27, 2020 20:24
toumorokoshi
toumorokoshi suggested changes May 30, 2020
View reviewed changes
Copy link
Member

@toumorokoshi toumorokoshi 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 overall! And thanks for doing this, sorely needed refactor. Couple comments:

  1. please refer to my suggested tox optimization.

  2. can you rename this package to opentelemetry-instrumentation-utils? see normalize term for instrumentations opentelemetry-specification#539

tox.ini Outdated

grpc: pip install {toxinidir}/ext/opentelemetry-ext-grpc[test]

wsgi,flask,django,asgi: pip install {toxinidir}/ext/opentelemetry-ext-utils
Copy link
Member

Choose a reason for hiding this comment

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

since this is used in pretty much every extension: maybe we can do a broader match for tox? IIRC tox is substring match, so you could may just use the string "ext":

ext: ...opentelemetry-ext-utils

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea, changed it. also renamed the package

lzchen
lzchen reviewed Jun 1, 2020
View reviewed changes
ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py
if code <= 299:
return StatusCanonicalCode.OK
if code <= 399:
if allow_redirect:
Copy link
Contributor

Choose a reason for hiding this comment

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

utils seems to be missing this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed. didn't realize the functions were slightly different

lzchen
lzchen reviewed Jun 1, 2020
View reviewed changes
tox.ini Outdated
@@ -87,6 +87,10 @@ envlist =
py3{4,5,6,7,8}-test-ext-sqlite3
pypy3-test-ext-sqlite3

; opentelemetry-instrumentation-utils
py3{5,6,7,8}-test-ext-utils
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rename to test-instrumentation-utils?

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed - there was some reason I kept it as test-ext but it doesn't seem to cause any problems being instrumentation

@lzchen
Copy link
Contributor

lzchen commented Jun 1, 2020

Curious as to whether we should still keep this inside the ext package as it is neither an integration nor an exporter.

@cnnradams
Copy link
Member Author

Curious as to whether we should still keep this inside the ext package as it is neither an integration nor an exporter.

Wasn't sure about that one either, on one hand it would only be used by integrations/exporters and never outside so its scope is ext/. However of course it isn't actually an integration or exporter as you said. So I'm up for suggestions, if you think it should belong in its own root folder or somewhere else I'm happy to move it

@lzchen
Copy link
Contributor

lzchen commented Jun 2, 2020

@cnnradams Might be good to bring it up during the SIG meeting.

@cnnradams cnnradams force-pushed the utils-package branch 4 times, most recently from aa65b03 to 6aa911d Compare June 4, 2020 15:50
@cnnradams cnnradams mentioned this pull request Jun 4, 2020
Closed
6 tasks
@cnnradams cnnradams changed the title refactor: Create utils package for common extension code refactor: Add common utils to opentelemetry-auto-instrumentation, rename opentelemetry-auto-instrumentation Jun 5, 2020
lzchen
lzchen approved these changes Jun 5, 2020
View reviewed changes
Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

LGTM

majorgreys
majorgreys suggested changes Jun 5, 2020
View reviewed changes
Copy link
Contributor

@majorgreys majorgreys left a comment

Choose a reason for hiding this comment

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

Great work! We should also move apply the renaming to the docs.

docs/auto_instrumentation/auto_instrumentation.rst Outdated
Comment on lines 1 to 2
OpenTelemetry Python Autoinstrumentation
========================================
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
OpenTelemetry Python Autoinstrumentation
========================================
OpenTelemetry Python Instrumentation
====================================

Copy link
Contributor

Choose a reason for hiding this comment

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

Also rename this file and directory.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated. let me know if I have missed any other doc locations, I have ctrl+f'd auto_instrumentation and others and haven't found anything but 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 5, 2020 •
edited
Loading

CLA Check
The committers are authorized under a signed CLA.

majorgreys
majorgreys approved these changes Jun 5, 2020
View reviewed changes
Copy link
Contributor

@majorgreys majorgreys left a comment

Choose a reason for hiding this comment

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

👍

toumorokoshi
toumorokoshi approved these changes Jun 6, 2020
View reviewed changes
Copy link
Member

@toumorokoshi toumorokoshi 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.

@cnnradams cnnradams force-pushed the utils-package branch 2 times, most recently from 0847437 to 8b9cfc4 Compare June 8, 2020 14:05
codeboten
codeboten approved these changes Jun 8, 2020
View reviewed changes
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 is looking great. Thanks for the refactor!

opentelemetry-instrumentation/tests/test_utils.py Outdated Show resolved Hide resolved
@codeboten codeboten merged commit 1041e11 into open-telemetry:master Jun 8, 2020
codeboten pushed a commit to codeboten/opentelemetry-python that referenced this pull request Jun 11, 2020
@cnnradams
refactor: Add common utils to opentelemetry-auto-instrumentation, ren…
02f3c18 
…ame opentelemetry-auto-instrumentation (open-telemetry#741)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@majorgreys majorgreys majorgreys approved these changes

@codeboten codeboten codeboten approved these changes

@toumorokoshi toumorokoshi toumorokoshi approved these changes

@lzchen lzchen lzchen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@cnnradams @lzchen @majorgreys @codeboten @toumorokoshi