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

Allow django to be instrumented automatically #1239

Merged

Conversation

owais
Copy link
Contributor

@owais owais commented Oct 14, 2020

Description

Django instrumentation required that OTEL_PYTHON_DJANGO_INSTRUMENT
environment variable be set to "True" by users. All other
instrumentations are enabled out of the box and there is no apparent
reason to disable Django by default. This commit changes the logic so
that Django instrumentation is applied by default. Users can set
OTEL_PYTHON_DJANGO_INSTRUMENT env var to "False" to disable it.

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?

  • Updated test suite
  • Manual testing

Checklist:

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

@owais owais requested review from a team, ocelotl and lzchen and removed request for a team October 14, 2020 13:34
Django instrumentation required that OTEL_PYTHON_DJANGO_INSTRUMENT
environment variable be set to "True" by users. All other
instrumentations are enabled out of the box and there is no apparent
reason to disable Django by default. This commit changes the logic so
that Django instrumentation is applied by default. Users can set
`OTEL_PYTHON_DJANGO_INSTRUMENT` env var to `"False"` to disable it.
@owais owais force-pushed the enable-django-instrumentation-by-default branch from d92c457 to 164b89c Compare October 14, 2020 13:34
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Nice!

docs/examples/django/README.rst Show resolved Hide resolved

Django's instrumentation can be disabled by setting the following environment variable.

#. ``export OTEL_PYTHON_DJANGO_INSTRUMENT=False``
Copy link
Member

Choose a reason for hiding this comment

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

I like .. envvar:: directive to keep track of all the envvars we have. Then they show up in the doc index nicely. Maybe in the module's docblock and then reference it here with :envvar:`OTEL_PYTHON_DJANGO_INSTRUMENT`?

Related #1147

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree but probably better if handled in a separate PR. One PR per package would provide a nice incremental improvement.

Disabling Django Instrumentation
--------------------------------

Django's instrumentation can be disabled by setting the following environment variable.
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious as to why we would expose a way to disable instrumentation where the user can simply not instrument manually/autoinstrument? Just asking because I don't think we have this mechanism for any other instrumentation.

Copy link
Contributor

@ocelotl ocelotl Oct 14, 2020

Choose a reason for hiding this comment

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

This was inspired by the DataDog implementation. If you have the Xinstrumentation plugin installed in your virtual environment it will instrument the Xlibrary when you run auto instrumentation, regardless if the user wants it or not. I see that this has some value when testing stuff, since it is easier than uninstalling the Xinstrumentation plugin, but I don't find this to be a critical feature. Maybe we should follow the same pattern in the rest of the instrumentations or remove it from this one to be consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is helpful when auto-instrumentation comes into play especially with the bootstrap command. The flow looks something like this:

pip install opentelemetry-instrumentation
opentelemetry-instrumentation-bootstrap -a=install
opentelemetry-instrument python main.py

Since the bootstrap command does not have any way to exclude certain instrumentations, an env var like this can help selectively disable some in case users find issues. I think instead of having this for every instrumentation, we can have a single var for the instrument command that accept a comma separated values to disable instrumentations. Something like,

export OTEL_PYTHON_DISABLED_INSTRUMENTATIONS=django,psycopg2
opentelemetry-instrument python main.py

This will allow people to temporarily disable instrumentation if they want and the feature will be consolidated in a single place (instrument command).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah consistency is what I recommend. @owais I don't have a strong opinion about this, please create an issue for the other instrumentations if you do decide to keep it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lzchen lzchen merged commit c782f14 into open-telemetry:master Oct 14, 2020
@owais owais deleted the enable-django-instrumentation-by-default branch October 14, 2020 16:34
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
* chore: update metrics example with UpDownCounter

* chore: update getting-started

* chore: update README

* fix: lint

* Update packages/opentelemetry-metrics/README.md

Co-authored-by: Daniel Dyla <[email protected]>

* Update packages/opentelemetry-metrics/README.md

Co-authored-by: Daniel Dyla <[email protected]>

Co-authored-by: Daniel Dyla <[email protected]>
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.

4 participants