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

Resolve issue with Pylint not supporting namespace packages properly. #290

Closed
owais opened this issue Jan 21, 2021 · 1 comment
Closed

Comments

@owais
Copy link
Contributor

owais commented Jan 21, 2021

We name our instrumentation packages the same as the libraries they instrument. For example, requests instrumentation is hosted in a module called requests with full import path being opentelemetry.instrumentation.requests. Newer versions of Pylint cannot deal with this properly. When importing the actual requests inside an instrumentation, pylint thinks we are importing the relative module (opentelemetry.instrumentation.requests) instead. As a result pylint complains that the module does not have fields or members we might access.

Current workaround

Currently, we silence such errors by disabling them on the specific lines where the modules are imported or their members are accessed.

Other options

Wait for Pylint to fix the bug***

The issue has been reported at least 2 years ago and remains open. It doesn't look like this will be fixed any time soon.
pylint-dev/pylint#2648
pylint-dev/pylint#2862

Revert to an older version of pylint

We can downgrade pylint to an older version that did not have this issue but unfortunately due to dependency issues we'll have to downgrade isort and/or black as well. This means we'll have to use a version of isort that does not support black compatible formatting resulting in developers having to fight formatting issues every time they run isort or black as they'd step on each other's toes.

Rename instrumentation modules

We could rename instrumentation modules so that the instrumentation module is never the same as the module being instrumented. For example, we could postfix every instrumentation module _otel or _inst. Import paths would look like opentelemetry.instrumentation.requests_otel. The downside is that this would be a breaking change and users instrumenting their code manually would have to update the import paths. We could possibly still keep the old import paths as aliases that log warnings if we really wanted to.

Pylint config or pythonpath manipulation

Perhaps there is another way to tweak how and where pylint looks for packages. May be re-ordering the pythonpath before running pylint can fix this as well. I haven't tried or come across any such solution so far but worth a look.

@owais owais added bug Something isn't working build & infra instrumentation and removed bug Something isn't working labels Jan 21, 2021
@ocelotl
Copy link
Contributor

ocelotl commented Jan 21, 2021

This is a Pylint issue, it should be fixed (or worked around) by using some Pylint-related mechanism. It may be Pylint configuration or comments that tell Pylint to ignore the errors. Whichever way it is, it should not require any change in the actual code itself of any instrumentation or OpenTelemetry component, linting tools should never require a code change to pass a wrongfully failing test.

@owais owais closed this as completed Apr 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants