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

Using get_tracer removes all warnings filters #3102

Closed
rafikdraoui opened this issue Dec 21, 2022 · 4 comments
Closed

Using get_tracer removes all warnings filters #3102

rafikdraoui opened this issue Dec 21, 2022 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@rafikdraoui
Copy link

rafikdraoui commented Dec 21, 2022

In #3041, a DeprecationWarning is ignored by adding a temporary warning filter and then later using warnings.resetfilters to remove it. However, warnings.resetfilters seems to clear every filters, including the default ones. This means that instrumenting code can enable noisy warning logging that is usually suppressed by default.

There is a context manager that can be used for temporarily changing the warning filters (https://docs.python.org/3/library/warnings.html#warnings.catch_warnings), but its documentation mentions that it is not thread-safe, so it maybe it wouldn't be appropriate for this library...


Describe your environment
Python 3.10, macOS and Linux (via Docker), otel-python v1.15.0 & 0.36b0

Steps to reproduce
Using the example Django app in this repo

  1. Modify the view to emit a (normally ignored) warning and to log the current filters
diff --git a/docs/examples/django/pages/views.py b/docs/examples/django/pages/views.py
index e805f4318..8f9e3bdd8 100644
--- a/docs/examples/django/pages/views.py
+++ b/docs/examples/django/pages/views.py
@@ -28,4 +28,8 @@ trace.get_tracer_provider().add_span_processor(


 def home_page_view(request):
+    import warnings
+    warnings.warn("hello", ResourceWarning)
+    print("--- warnings.filters ---")
+    print(warnings.filters)
+    print("------------------------")
     return HttpResponse("Hello, world")
  1. Start the server with python manage.py runserver --noreload

  2. Open http://127.0.0.1:8000/,

  3. Stop the server with ctrl-c

We see the following in the console:

With instrumentation:

...
~/.../opentelemetry-python/docs/examples/django/pages/views.py:32: ResourceWarning: hello
  warnings.warn("hello", ResourceWarning)
--- warnings.filters ---
[]
------------------------
[21/Dec/2022 12:34:56] "GET / HTTP/1.1" 200 12
{
  ... // otel trace
}
^Csys:1: ResourceWarning: unclosed <socket.socket fd=4, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('127.0.0.1', 8000)>
ResourceWarning: Enable tracemalloc to get the object allocation traceback

Without instrumentation (i.e. commenting out DjangoInstrumentor().instrument() in manage.py)

...
--- warnings.filters ---
[('default', None, <class 'DeprecationWarning'>, '__main__', 0), ('ignore', None, <class 'DeprecationWarning'>, None, 0), ('ignore', None, <class 'PendingDeprecationWarning'>, None, 0), ('ignore', None, <class 'ImportWarning'>, None, 0), ('ignore', None, <class 'ResourceWarning'>, None, 0), ('ignore', None, <class 'pkg_resources.PEP440Warning'>, None, 0)]
------------------------
[21/Dec/2022 12:34:56] "GET / HTTP/1.1" 200 12
^C 

Additional context

The default warnings filters are preserved when using versions 1.14.0 and 0.35b0.

@rafikdraoui rafikdraoui added the bug Something isn't working label Dec 21, 2022
@ccorbacho
Copy link

Just to add on this - this is actually quite a major regression.

For those of us where we are using third party services to process our logs, and at scale, activating all the deprecation warnings over a huge number of containers is causing a massive increase in the number of log lines being emitted from applications, which in turn pushes up our costs considerably (as these services will charge per lines of logs ingested), so we had to rollback to 1.14.0 and cannot upgrade until this is fixed.

(I suspect this may impact others in a similar situation - but even without a cloud vendor, it's now a huge amount of extra lines of logs having to be processed with all the deprecation warnings being emitted, so all the associated storage and CPU costs).

If this is going to take a long time to fix in a safe and proper way, can you revert the original change until then, as calling resetfilters() and turning on all the deprecation warnings is far more problematic than the original bug you were trying to fix?

@jeremydvoss
Copy link
Contributor

jeremydvoss commented Feb 10, 2023

I'm also getting this "ResourceWarning: unclosed" warning. Regardless of warning filtering, the memory allocation issue seems like something we should fix in general. The flask instrumentation seems to be the source of the issue for me.

Do we think this warning always existed but was simply filtered out before #3041 ?

EDIT: This issue of mine seems to be resolved with the revert of #3041, #3157

@samsamoa
Copy link

workaround:

def dont_reset_warnings():
    pass

warnings.resetwarnings = dont_reset_warnings

@srikanthccv
Copy link
Member

This is fixed in 1.16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants