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

Adding try catch to fix ImproperlyConfigured exception while allowing #1369

Merged
merged 10 commits into from
Oct 6, 2022

Conversation

jeremydvoss
Copy link
Contributor

@jeremydvoss jeremydvoss commented Oct 3, 2022

custom django settings

Description

See issue: #677
If Django is installed but the DJANGO_SETTINGS_MODULE is not set, auto-instrumentation breaks -- even when auto-instrumenting non-django apps.

  • Since django is installed, sitecustomize attempts to call load_instrumentor but fails when it attempts to pull the MIDDLEWARE setting.
Instrumenting of django failed
Traceback (most recent call last):
  File "c:\users\jeremyvoss\workplace\opentelemetry-python-contrib\opentelemetry-instrumentation\src\opentelemetry\instrumentation\auto_instrumentation\sitecustomize.py", line 89, in _load_instrumentors
    distro.load_instrumentor(entry_point, skip_dep_check=True)
  File "C:\Users\jeremyvoss\workplace\auto_instrumentation\lib\site-packages\opentelemetry\instrumentation\distro.py", line 63, in load_instrumentor
    instrumentor().instrument(**kwargs)
  File "C:\Users\jeremyvoss\workplace\auto_instrumentation\lib\site-packages\opentelemetry\instrumentation\instrumentor.py", line 109, in instrument
    raise exc
  File "c:\users\jeremyvoss\workplace\opentelemetry-python-contrib\opentelemetry-instrumentation\src\opentelemetry\instrumentation\auto_instrumentation\sitecustomize.py", line 89, in _load_instrumentors
    distro.load_instrumentor(entry_point, skip_dep_check=True)
  File "C:\Users\jeremyvoss\workplace\auto_instrumentation\lib\site-packages\opentelemetry\instrumentation\distro.py", line 63, in load_instrumentor
    instrumentor().instrument(**kwargs)
  File "C:\Users\jeremyvoss\workplace\auto_instrumentation\lib\site-packages\opentelemetry\instrumentation\instrumentor.py", line 109, in instrument
    result = self._instrument(  # pylint: disable=assignment-from-no-return
  File "C:\Users\jeremyvoss\workplace\opentelemetry-python-contrib\instrumentation\opentelemetry-instrumentation-django\src\opentelemetry\instrumentation\django\__init__.py", line 280, in _instrument
    settings_middleware = getattr(settings, _middleware_setting, [])
  File "C:\Users\jeremyvoss\workplace\auto_instrumentation\lib\site-packages\django\conf\__init__.py", line 92, in __getattr__
    self._setup(name)
  File "C:\Users\jeremyvoss\workplace\auto_instrumentation\lib\site-packages\django\conf\__init__.py", line 72, in _setup
    raise ImproperlyConfigured(
django.core.exceptions.ImproperlyConfigured: Requested setting MIDDLEWARE, but settings are not configured. You must either define the environment variable DJANGO_SETTINGS_MODULE or call settings.configure() before accessing settings.

Since this would cause issues for non-django apps, and it is unreasonable to ask that users always uninstall django on their machines when running non-django apps or to have a DJANGO_SETTINGS_MODULE env var configured when running non-django apps, I see 2 possible fixes:

  1. If there is no env var, we call settings.configure(). This leaves the settings empty, but configured. Which means we can still add ['opentelemetry.instrumentation.django.middleware.otel_middleware._DjangoMiddleware'] to it. The reason why we would actually set the settings to nothing instead of just the settings_middleware variable is that the remaining code uses getattr again. At first, I expected to make the following change:
if not settings.configured:
    settings.configure()
settings_middleware = getattr(settings, _middleware_setting, [])

However, it turns out that when DJANGO_SETTINGS_MODULE set, settings.configured still returns false. It's only when getattr is called for the first time that django searches for the settings that DJANGO_SETTINGS_MODULE points to. Then, it either sets the settings to configured or throws the ImproperlyConfigured exception. For this reason, this PR checks whether django is configured by calling getattr itself.

  1. The other optione exit the django instrumentation peacefully but allow sitecustomize to continue other instrumentations. This would likely require changes to sitecustomize, since it is currently set up to stop instrumenting ALL instrumentations when an exception occurs in just one.

Fixes # (#677)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Testing auto-instrumentation with DJANGO_SETTINGS_MODULE. Ensured that DJANGO_SETTINGS_MODULE settings are used and not overwritten.
  • Testing auto-instrumentation without DJANGO_SETTINGS_MODULE. Ensured that instrumentation succeeds and spans are produced.

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@jeremydvoss jeremydvoss marked this pull request as ready for review October 4, 2022 16:11
@jeremydvoss jeremydvoss requested a review from a team as a code owner October 4, 2022 16:11
exception,
)
settings.configure()
settings_middleware = getattr(settings, _middleware_setting, [])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though the fix is the same, since these are distinct scenarios, I decided to keep them as separate except clauses

@srikanthccv srikanthccv enabled auto-merge (squash) October 6, 2022 12:53
@srikanthccv srikanthccv merged commit a47c45e into open-telemetry:main Oct 6, 2022
saartochner-lumigo pushed a commit to lumigo-io/opentelemetry-python-contrib that referenced this pull request Nov 13, 2022
saartochner-lumigo pushed a commit to lumigo-io/opentelemetry-python-contrib that referenced this pull request Nov 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants