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

Explain usage of --noreload #1795

Merged
merged 3 commits into from
Apr 30, 2021
Merged

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Apr 22, 2021

Fixes #1794

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

  • Test A

Does This PR Require a Contrib Repo Change?

Answer the following question based on these examples of changes that would require a Contrib Repo Change:

  • The OTel specification has changed which prompted this PR to update the method interfaces of opentelemetry-api/ or opentelemetry-sdk/

  • The method interfaces of opentelemetry-instrumentation/ have changed

  • The method interfaces of test/util have changed

  • Scripts in scripts/ that were copied over to the Contrib repo have changed

  • Configuration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in

    • pyproject.toml
    • isort.cfg
    • .flake8
  • When a new .github/CODEOWNER is added

  • Major changes to project information, such as in:

    • README.md
    • CONTRIBUTING.md
  • Yes. - Link to PR:

  • No.

Checklist:

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

@ocelotl ocelotl added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Apr 22, 2021
@ocelotl ocelotl self-assigned this Apr 22, 2021
@ocelotl ocelotl requested review from a team, codeboten and aabmass and removed request for a team April 22, 2021 22:28
This same example can be run using auto instrumentation. Comment out the call
to ``DjangoInstrumento().instrument()`` in ``main``, then Run the django app
with ``opentelemetry-instrumentation python manage.py runserver --noreload``.
Repeat the steps with the client, the result should be the same.
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally people don't use runserver in real environment and only use it for dev. I wonder if we should document for local/dev environment like this. If we want it, I'd suggest to change title to Auto Instrumentation for runserver to clarify production envs shouldn't do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Read the rest of the document. This whole doc should be re-structured to contain info on how to instrument in at least three well-known cases which are "runserver" (for local dev), "gunicorn" and "uwsgi".

This PR doesn't need to do that of course but would be nice if we had an issue for 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.

Added clarification 👍

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks for adding these docs. Does it make sense to also add a note in the Flask example? This would address open-telemetry/opentelemetry-python-contrib#477

@ocelotl
Copy link
Contributor Author

ocelotl commented Apr 30, 2021

Thanks for adding these docs. Does it make sense to also add a note in the Flask example? This would address open-telemetry/opentelemetry-python-contrib#477

Added comment 👍

@ocelotl ocelotl requested a review from codeboten April 30, 2021 16:50
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks @ocelotl!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use --noreload for Django example with autoinstrumentation
3 participants