-
Notifications
You must be signed in to change notification settings - Fork 651
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
Add missing references to instrumented packages #1416
Add missing references to instrumented packages #1416
Conversation
b594647
to
19cf4a7
Compare
Looking through it, I agree the code isn't documented enough to tell you what it's for. Conceptually, I believe this is part of a tool that looks at installed packages, and handles installing the appropriate instrumentations for you. This helps for those who prefer to turn on as much instrumentation as possible, and don't want to go through the manual process of combing through the libraries they use to figure out the instrumentations. Would you mind adding a comment to document the purpose of that list? it'll help the next contributor answer that question. |
@@ -25,6 +25,8 @@ | |||
|
|||
# target library to desired instrumentor path/versioned package name | |||
instrumentations = { | |||
"aiohttp-client": "opentelemetry-instrumentation-aiohttp-client>=0.15b0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add the comment I asked above this, maybe right below the imports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet that is super helpful! Added a comment :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM because the change is good, but adding the comment would be great!
opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the comment and the missing instrumentations. If you can remove the system-metrics from the list, we can get this merged right away
opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap.py
Outdated
Show resolved
Hide resolved
fd37bae
to
f912baf
Compare
Description
I'm not sure what this list is for, but it seemed like an incomplete list of the packages for which we have instrumentation for. Added the missing packages (
aiohttp-client
,aiopg
,sklearn
,system-metrics
) according to those packages in the Contrib repo!Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Unit tests cover this change
Checklist:
- [] Unit tests have been added- [ ] Documentation has been updated