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

chore!: deprecate DD_CALL_BASIC_CONFIG #3172

Closed
wants to merge 3 commits into from

Conversation

mabdinur
Copy link
Contributor

@mabdinur mabdinur commented Jan 24, 2022

Deprecate DD_CALL_BASIC_CONFIG configuration.

This change removes all invocations of logging.basicConfig from tracing instrumentation: #3168.

Checklist

  • Added to the correct milestone.
  • Tests provided or description of manual testing performed is included in the code or PR.
  • Library documentation is updated.
  • Corp site documentation is updated (link to the PR).

@mabdinur mabdinur requested a review from a team as a code owner January 24, 2022 23:06
@mabdinur mabdinur added the v1.0 label Jan 25, 2022
@mabdinur mabdinur changed the title chore!: deprecate DD_CALL_BASIC_CONFIG [ddtrace-1.0] chore!: deprecate DD_CALL_BASIC_CONFIG Jan 25, 2022
@@ -92,11 +92,6 @@ below:
- False
- Enables :ref:`Logs Injection`.

.. _dd-call-basic-config:
* - ``DD_CALL_BASIC_CONFIG``
Copy link
Member

Choose a reason for hiding this comment

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

we shouldn't remove from here yet

---
upgrade:
- |
ddtrace will no longer handle basic configuration of a logging system. `logging.basicConfig()` should be called in a client application.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ddtrace will no longer handle basic configuration of a logging system. `logging.basicConfig()` should be called in a client application.
ddtrace will no longer handle basic configuration of a logging system. ``logging.basicConfig()`` should be called in a client application.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can state more simply:

ddtrace will no longer call logging.baseConfig() on startup.

Comment on lines +76 to +81
deprecation(
name="ddtrace.tracer.logging.basicConfig",
message="ddtrace will no longer handle basic configuration for a logging system.\
`logging.basicConfig()` should be called in a user's application",
version="1.0.0",
)
Copy link
Member

Choose a reason for hiding this comment

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

🤔

I am not sure we can handle this with a deprecation actually. Since we do this by default we are basically going to add a warning log on every startup even when the user does nothing.

We might be better off just making the change on the 1.0-dev branch and have it as an upgrade note and we can document in any migration guide.

We could maybe deprecate the DD_CALL_BASIC_CONFIG env variable? But not sure if it is fully necessary since most people use it to disable calling basic config, so should not have any impact on upgrade.

cc @Kyle-Verhoog what do you think? is there another deprecation approach I am not thinking of?

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'll close this PR

@mabdinur mabdinur closed this Jan 26, 2022
@mabdinur mabdinur deleted the munir/disable-logging-master branch February 18, 2022 21:44
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.

2 participants