refactor: Add common utils to opentelemetry-auto-instrumentation, rename opentelemetry-auto-instrumentation#741
Conversation
cnnradams
force-pushed
the
utils-package
branch
5 times, most recently
from
14ad082 to
29a6d17
Compare
toumorokoshi
left a comment
toumorokoshi
left a comment
There was a problem hiding this comment.
Looks good overall! And thanks for doing this, sorely needed refactor. Couple comments:
-
please refer to my suggested tox optimization.
-
can you rename this package to
opentelemetry-instrumentation-utils? see open-telemetry/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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
good idea, changed it. also renamed the package
| if code <= 299: | ||
| return StatusCanonicalCode.OK | ||
| if code <= 399: | ||
| if allow_redirect: |
There was a problem hiding this comment.
utils seems to be missing this case?
There was a problem hiding this comment.
fixed. didn't realize the functions were slightly different
tox.ini
Outdated
| pypy3-test-ext-sqlite3 | ||
|
|
||
| ; opentelemetry-instrumentation-utils | ||
| py3{5,6,7,8}-test-ext-utils |
There was a problem hiding this comment.
Should we rename to test-instrumentation-utils?
There was a problem hiding this comment.
renamed - there was some reason I kept it as test-ext but it doesn't seem to cause any problems being instrumentation
|
Curious as to whether we should still keep this inside the |
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 |
|
@cnnradams Might be good to bring it up during the SIG meeting. |
cnnradams
force-pushed
the
utils-package
branch
4 times, most recently
from
aa65b03 to
6aa911d
Compare
cnnradams
changed the title
majorgreys
left a comment
majorgreys
left a comment
There was a problem hiding this comment.
Great work! We should also move apply the renaming to the docs.
| OpenTelemetry Python Autoinstrumentation | ||
| ======================================== |
There was a problem hiding this comment.
| OpenTelemetry Python Autoinstrumentation | |
| ======================================== | |
| OpenTelemetry Python Instrumentation | |
| ==================================== |
There was a problem hiding this comment.
Also rename this file and directory.
There was a problem hiding this comment.
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 🤷
…ation,and the console command `opentelemetry-auto-instrumentation` to `opentelemetry-instrument`
cnnradams
force-pushed
the
utils-package
branch
2 times, most recently
from
0847437 to
8b9cfc4
Compare
codeboten
left a comment
codeboten
left a comment
There was a problem hiding this comment.
This is looking great. Thanks for the refactor!
…ame opentelemetry-auto-instrumentation (open-telemetry#741)
5 participants
TL;DR:
Resolves #735, resolves #610, resolves #667, resolves #608