[core] add Deprecation system with documentation #450

palazzem · 2018-04-13T16:44:12Z

Overview

This patch introduces a deprecation system via warnings module. It offers a way to add deprecation around our code, when we need to alter/remove a functionality in newer releases. The deprecation log is silent by default, though it could be activated via Python flag / environment variable. This approach should be considered "safe" especially because it's used by many web frameworks (i.e. Django) and well documented in the Python official docs.

ddtrace.util -> ddtrace.utils.* (split in many modules)
ddtrace.contrib.util -> ddtrace.utils.importlib

User impact

No impact for the user since the DeprecationWarning is silent by default, so unless the production application is run via -Wall, nothing will be printed in users' logs.

What is missing

It's more part of a process, but we should document somewhere (even with a simple list) every time we add a deprecation warning. In that way, users can find easily what's going to be changed in the next major release. A good example is Django: https://docs.djangoproject.com/en/dev/internals/deprecation/

palazzem · 2018-04-13T16:45:12Z

ddtrace/contrib/cassandra/session.py

we can safely remove everything after going 1.0. No reason to remove these things now, even if it's reasonable safe.

palazzem · 2018-04-13T16:46:16Z

ddtrace/contrib/util.py

It's more a matter of style, but I definitely prefer having ddtrace.utils with all our utils, rather than 2 different places with bunch of functions without a logical connection.

If that's the API we agree on, then you can remove this comment as it makes the reader think that this is a legacy interface only for BC. Instead of understanding that this is the main interface.

Also, should this move to contrib/utils/__init__.py instead? (same for the below utils/__init__.py.

Maybe I misunderstood your comment. The main interface is ddtrace.utils for internal utilities, so all others (contrib/util.py and util.py) will be removed in 1.0. I would prefer to keep everything inside ddtrace.utils rather than splitting it in ddtrace.utils and ddtrace.contrib.utils.

In Django, for instance, you have everything under this package and then they're separated in multiple modules by functionality.

Alright my bad! Got it after a second read :)

palazzem · 2018-04-13T16:47:09Z

ddtrace/util.py

If deprecation warnings are activated, this action will trigger a deprecation message (and will not crash with an import fail):

from ddtrace.util import asbool

palazzem · 2018-04-13T16:47:37Z

ddtrace/utils/deprecation.py

It's to ensure consistency for all our deprecation calls.

palazzem · 2018-04-13T16:49:14Z

ddtrace/utils/wrappers.py

This function is never used in our repo and it should not be used by anyone. But since there is no reason to remove it in this minor release, I would prefer to keep it till 1.0.0 where we can definitely clean a lot our code.

palazzem · 2018-04-13T16:50:05Z

docs/index.rst

Section that is in line with Django Web Framework. Credits to this great community!

Ref: https://docs.djangoproject.com/en/2.0/howto/upgrade-version/#resolving-deprecation-warnings

palazzem · 2018-04-13T16:50:42Z

tests/test_utils.py

tested as suggested in the stdlib: https://docs.python.org/3/library/warnings.html#testing-warnings

…tibility

…fe_patch`; they will note be used anymore * `ddtrace.util` is replaced with `ddtrace.utils` package * `ddtrace.contrib.util` is replaced with `ddtrace.utils.importlib` * `safe_patch` will be removed because not used anymore

… going to be removed

palazzem · 2018-04-13T21:51:23Z

ddtrace/utils/deprecation.py

+from functools import wraps
+
+
+class RemovedInDDTrace10Warning(DeprecationWarning):


Used to create messages such as:

script.py:10: RemovedInDDTrace10Warning: [...]

LotharSee · 2018-04-16T12:17:47Z

ddtrace/utils/formats.py

+    env = 'DD_{}'.format(key)
+
+    # [Backward compatibility]: `DATADOG_` variables should be supported;
+    # add a deprecation warning later if it's used, so that we can drop the key


Is it time to do it? or we might forget.

oh good call! I totally forgot about it

LotharSee · 2018-04-16T12:21:07Z

docs/index.rst

+    # or
+
+    $ PYTHONWARNINGS=all python app.py
+


Is there a programatic way we could add and document too?
Dev might not have access to the runtime options but they could change something like in tracer.configure.

Yes-ish?

logging.captureWarnings and warnings.simplefilter offer some options, but the developer will need to be careful about where those calls are made.

Specifically, they will need to make those calls before any ddtrace imports happen (since most of the warnings of interest happen at import time). Additionally, they may have some other import sequencing considerations depending on what their logging infra/setup looks like.

What could be the plan here? I may add something programmatic here, but as said by @SeanOC it may not work 100% of the time. I totally agree that devs may not have access to the runtime, but these warnings can still be raised in a local development environment (i.e. running application tests).

If we don't want to provide a programmatic way, I can reword a bit better this part of the documentation explicitly saying what I wrote in the previous paragraph.

SeanOC

Overall looks good.

Gave some hopefully helpful context re: options for enabling warnings without control over the runtime command/environment. For me, this falls into the category of nice to have, especially given Django doesn't specifically mention it.

SeanOC · 2018-04-16T17:30:18Z

docs/index.rst

+    # or
+
+    $ PYTHONWARNINGS=all python app.py
+


Yes-ish?

logging.captureWarnings and warnings.simplefilter offer some options, but the developer will need to be careful about where those calls are made.

Specifically, they will need to make those calls before any ddtrace imports happen (since most of the warnings of interest happen at import time). Additionally, they may have some other import sequencing considerations depending on what their logging infra/setup looks like.

palazzem · 2018-04-16T22:15:31Z

@SeanOC @LotharSee I would like to agree in the deprecation warning (programmatically or better documentation?) before proceeding with the merge.

SeanOC · 2018-04-16T22:27:16Z

Chewing on it a little more, I'm actually inclined to not provide documentation on setting this programmatically.

By design, these warnings really should only get turned on in a dev environment where a developer should have access to either the run command or environment variables.

If we include instructions on how to enable them programmatically, I suspect we'll mostly just confuse people and leave people making a noisy mess in their production logs (e.g. people just skim the docs and blindly apply the programmatic approach). Should questions come up about it, we can always either one-off point people towards options or update the docs later.

Knowing the Django folks, if this was a problem for a non-trivial portion of folks, they'd have included it in their docs.

palazzem · 2018-04-16T22:30:13Z

Totally agreed, especially because the Django community is quite big and had lot of use cases in the past. Let's wait for @LotharSee if there is anything else we can improve here.

LotharSee · 2018-04-17T08:32:37Z

Sounds fair, good to me!

palazzem added enhancement core labels Apr 13, 2018

palazzem added this to the 0.12.0 milestone Apr 13, 2018

palazzem requested review from LotharSee and SeanOC April 13, 2018 16:44

palazzem commented Apr 13, 2018

View reviewed changes

Emanuele Palazzetti added 6 commits April 13, 2018 14:00

[core] add deprecation module

7af26f1

[core] split util module in utils package; keeping backward compa…

1ed86e7

…tibility

[core] library migration to ddtrace.utils package

0632287

[core] add ddtrace.utils.deprecation tests

af474fb

[docs] add Resolving deprecation warnings section

6e4712d

palazzem force-pushed the palazzem/deprecation branch from 21e2c86 to 6e4712d Compare April 13, 2018 18:01

[core] add RemovedInDDTrace10Warning to be explicit when something is…

f364291

… going to be removed

palazzem commented Apr 13, 2018

View reviewed changes

LotharSee reviewed Apr 16, 2018

View reviewed changes

SeanOC approved these changes Apr 16, 2018

View reviewed changes

palazzem merged commit 6d378c4 into 0.12-dev Apr 17, 2018

palazzem deleted the palazzem/deprecation branch April 17, 2018 18:40

		from functools import wraps


		class RemovedInDDTrace10Warning(DeprecationWarning):

[core] add Deprecation system with documentation #450

[core] add Deprecation system with documentation #450

Uh oh!

Conversation

palazzem commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

User impact

What is missing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LotharSee Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SeanOC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palazzem commented Apr 16, 2018

Uh oh!

SeanOC commented Apr 16, 2018

Uh oh!

palazzem commented Apr 16, 2018

Uh oh!

LotharSee commented Apr 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

palazzem commented Apr 13, 2018 •

edited

Loading

LotharSee Apr 16, 2018 •

edited

Loading