Repository: getsentry/sentry-python

Issue:
Clarify proxy fallback behavior
I am currently working on #178 and extended the tests for the proxy selection mechanism, especially when the env variables like `HTTP_PROXY` and `HTTPS_PROXY` are present. One thing that I've observed is the fallback behavior:

- no ENV vars: HTTPS DSNs can fallback to HTTP proxies (see [test](https://github.com/getsentry/sentry-python/blob/master/tests/test_client.py#L47-L48))
- only `HTTP_PROXY` env var: no fallback for HTTPS DSNs, i.e. no proxy is auto-selected

Is that behavior intended or a bug? In either case, my PR for #178 will also include tests (and potential fixes) for that.

Discussion:
untitaker: Thanks for this! Since we try to have this interface aligned across SDKs I'll discuss this with the other SDK maintainers first.
crepererum: @untitaker are there any updates on that?
untitaker: @crepererum I forgot to start the discussion at the last meeting, sorry. We have a meeting every monday (so, today again) where I will bring this up, but please note that this is quite low-priority for us and we can't force a decision "at gunpoint".
untitaker: To be clear, I see two versions:

* ```init(http_proxy=False, https_proxy=False)```

* ```init(no_proxy=True)```
untitaker: Sorry, we have an update now: Could you use empty string to represent `False`? We want to port this behavior to other languages and there we can't replicate this type signature.
crepererum: Done. Build failure seems unrelated (pip doesn't like pyproject.toml and editable installs)
untitaker: I made a PR: #349 

Patch:
diff --git a/sentry_sdk/client.py b/sentry_sdk/client.py
index 8ecea608..15c869a5 100644
--- a/sentry_sdk/client.py
+++ b/sentry_sdk/client.py
@@ -116,13 +116,17 @@ class Client(object):
             and "threads" not in event
         ):
             with capture_internal_exceptions():
-                event["threads"] = [
-                    {
-                        "stacktrace": current_stacktrace(self.options["with_locals"]),
-                        "crashed": False,
-                        "current": True,
-                    }
-                ]
+                event["threads"] = {
+                    "values": [
+                        {
+                            "stacktrace": current_stacktrace(
+                                self.options["with_locals"]
+                            ),
+                            "crashed": False,
+                            "current": True,
+                        }
+                    ]
+                }
 
         for key in "release", "environment", "server_name", "dist":
             if event.get(key) is None and self.options[key] is not None:  # type: ignore
diff --git a/sentry_sdk/integrations/logging.py b/sentry_sdk/integrations/logging.py
index 60fba0dc..bcbe8630 100644
--- a/sentry_sdk/integrations/logging.py
+++ b/sentry_sdk/integrations/logging.py
@@ -170,13 +170,17 @@ class EventHandler(logging.Handler, object):
             event = {}
             hint = None
             with capture_internal_exceptions():
-                event["threads"] = [
-                    {
-                        "stacktrace": current_stacktrace(client_options["with_locals"]),
-                        "crashed": False,
-                        "current": True,
-                    }
-                ]
+                event["threads"] = {
+                    "values": [
+                        {
+                            "stacktrace": current_stacktrace(
+                                client_options["with_locals"]
+                            ),
+                            "crashed": False,
+                            "current": True,
+                        }
+                    ]
+                }
         else:
             event = {}
 
diff --git a/sentry_sdk/transport.py b/sentry_sdk/transport.py
index fe6c41cd..fc071f0d 100644
--- a/sentry_sdk/transport.py
+++ b/sentry_sdk/transport.py
@@ -156,10 +156,15 @@ class HttpTransport(Transport):
         ca_certs,  # type: Optional[Any]
     ):
         # type: (...) -> Union[PoolManager, ProxyManager]
-        # Use http_proxy if scheme is https and https_proxy is not set
-        proxy = parsed_dsn.scheme == "https" and https_proxy or http_proxy
-        if not proxy:
-            proxy = getproxies().get(parsed_dsn.scheme)
+        proxy = None
+
+        # try HTTPS first
+        if parsed_dsn.scheme == "https" and (https_proxy != ""):
+            proxy = https_proxy or getproxies().get("https")
+
+        # maybe fallback to HTTP proxy
+        if not proxy and (http_proxy != ""):
+            proxy = http_proxy or getproxies().get("http")
 
         opts = self._get_pool_options(ca_certs)
 
diff --git a/sentry_sdk/utils.py b/sentry_sdk/utils.py
index 94e14b20..912dbdb4 100644
--- a/sentry_sdk/utils.py
+++ b/sentry_sdk/utils.py
@@ -587,6 +587,10 @@ def iter_event_stacktraces(event):
     # type: (Dict[str, Any]) -> Iterator[Dict[str, Any]]
     if "stacktrace" in event:
         yield event["stacktrace"]
+    if "threads" in event:
+        for thread in event["threads"].get("values") or ():
+            if "stacktrace" in thread:
+                yield thread["stacktrace"]
     if "exception" in event:
         for exception in event["exception"].get("values") or ():
             if "stacktrace" in exception:


Test patch:
diff --git a/tests/integrations/logging/test_logging.py b/tests/integrations/logging/test_logging.py
index 73e0f482..d368b447 100644
--- a/tests/integrations/logging/test_logging.py
+++ b/tests/integrations/logging/test_logging.py
@@ -69,7 +69,7 @@ def test_logging_stack(sentry_init, capture_events):
     event_with, event_without, = events
 
     assert event_with["level"] == "error"
-    assert event_with["threads"][0]["stacktrace"]["frames"]
+    assert event_with["threads"]["values"][0]["stacktrace"]["frames"]
 
     assert event_without["level"] == "error"
     assert "threads" not in event_without
diff --git a/tests/test_client.py b/tests/test_client.py
index ec4b77e8..cd5c84c2 100644
--- a/tests/test_client.py
+++ b/tests/test_client.py
@@ -1,6 +1,7 @@
 # coding: utf-8
 import json
 import logging
+import os
 import pytest
 import subprocess
 import sys
@@ -52,25 +53,106 @@ def test_transport_option(monkeypatch):
     assert str(Client(transport=transport).dsn) == dsn
 
 
-def test_http_proxy(monkeypatch):
-    client = Client("https://foo@sentry.io/123", http_proxy="http://localhost/123")
+def test_proxy_http_use(monkeypatch):
+    client = Client("http://foo@sentry.io/123", http_proxy="http://localhost/123")
     assert client.transport._pool.proxy.scheme == "http"
 
+
+def test_proxy_https_use(monkeypatch):
+    client = Client("https://foo@sentry.io/123", http_proxy="https://localhost/123")
+    assert client.transport._pool.proxy.scheme == "https"
+
+
+def test_proxy_both_select_http(monkeypatch):
     client = Client(
-        "https://foo@sentry.io/123",
+        "http://foo@sentry.io/123",
         https_proxy="https://localhost/123",
         http_proxy="http://localhost/123",
     )
-    assert client.transport._pool.proxy.scheme == "https"
-
-    client = Client("http://foo@sentry.io/123", http_proxy="http://localhost/123")
     assert client.transport._pool.proxy.scheme == "http"
 
+
+def test_proxy_both_select_https(monkeypatch):
     client = Client(
-        "http://foo@sentry.io/123",
+        "https://foo@sentry.io/123",
         https_proxy="https://localhost/123",
         http_proxy="http://localhost/123",
     )
+    assert client.transport._pool.proxy.scheme == "https"
+
+
+def test_proxy_http_fallback_http(monkeypatch):
+    client = Client("https://foo@sentry.io/123", http_proxy="http://localhost/123")
+    assert client.transport._pool.proxy.scheme == "http"
+
+
+def test_proxy_none_noenv(monkeypatch):
+    client = Client("http://foo@sentry.io/123")
+    assert client.transport._pool.proxy is None
+
+
+def test_proxy_none_httpenv_select(monkeypatch):
+    os.environ["HTTP_PROXY"] = "http://localhost/123"
+    client = Client("http://foo@sentry.io/123")
+    assert client.transport._pool.proxy.scheme == "http"
+
+
+def test_proxy_none_httpsenv_select(monkeypatch):
+    os.environ["HTTPS_PROXY"] = "https://localhost/123"
+    client = Client("https://foo@sentry.io/123")
+    assert client.transport._pool.proxy.scheme == "https"
+
+
+def test_proxy_none_httpenv_fallback(monkeypatch):
+    os.environ["HTTP_PROXY"] = "http://localhost/123"
+    client = Client("https://foo@sentry.io/123")
+    assert client.transport._pool.proxy.scheme == "http"
+
+
+def test_proxy_bothselect_bothen(monkeypatch):
+    os.environ["HTTP_PROXY"] = "http://localhost/123"
+    os.environ["HTTPS_PROXY"] = "https://localhost/123"
+    client = Client("https://foo@sentry.io/123", http_proxy="", https_proxy="")
+    assert client.transport._pool.proxy is None
+
+
+def test_proxy_bothavoid_bothenv(monkeypatch):
+    os.environ["HTTP_PROXY"] = "http://localhost/123"
+    os.environ["HTTPS_PROXY"] = "https://localhost/123"
+    client = Client("https://foo@sentry.io/123", http_proxy=None, https_proxy=None)
+    assert client.transport._pool.proxy.scheme == "https"
+
+
+def test_proxy_bothselect_httpenv(monkeypatch):
+    os.environ["HTTP_PROXY"] = "http://localhost/123"
+    client = Client("https://foo@sentry.io/123", http_proxy=None, https_proxy=None)
+    assert client.transport._pool.proxy.scheme == "http"
+
+
+def test_proxy_httpselect_bothenv(monkeypatch):
+    os.environ["HTTP_PROXY"] = "http://localhost/123"
+    os.environ["HTTPS_PROXY"] = "https://localhost/123"
+    client = Client("https://foo@sentry.io/123", http_proxy=None, https_proxy="")
+    assert client.transport._pool.proxy.scheme == "http"
+
+
+def test_proxy_httpsselect_bothenv(monkeypatch):
+    os.environ["HTTP_PROXY"] = "http://localhost/123"
+    os.environ["HTTPS_PROXY"] = "https://localhost/123"
+    client = Client("https://foo@sentry.io/123", http_proxy="", https_proxy=None)
+    assert client.transport._pool.proxy.scheme == "https"
+
+
+def test_proxy_httpselect_httpsenv(monkeypatch):
+    os.environ["HTTPS_PROXY"] = "https://localhost/123"
+    client = Client("https://foo@sentry.io/123", http_proxy=None, https_proxy="")
+    assert client.transport._pool.proxy is None
+
+
+def test_proxy_httpsselect_bothenv_http(monkeypatch):
+    os.environ["HTTP_PROXY"] = "http://localhost/123"
+    os.environ["HTTPS_PROXY"] = "https://localhost/123"
+    client = Client("http://foo@sentry.io/123", http_proxy=None, https_proxy=None)
     assert client.transport._pool.proxy.scheme == "http"
 
 
@@ -147,7 +229,7 @@ def test_attach_stacktrace_enabled():
     foo()
 
     event, = events
-    thread, = event["threads"]
+    thread, = event["threads"]["values"]
     functions = [x["function"] for x in thread["stacktrace"]["frames"]]
     assert functions[-2:] == ["foo", "bar"]
 
@@ -167,11 +249,26 @@ def test_attach_stacktrace_enabled_no_locals():
     foo()
 
     event, = events
-    thread, = event["threads"]
+    thread, = event["threads"]["values"]
     local_vars = [x.get("vars") for x in thread["stacktrace"]["frames"]]
     assert local_vars[-2:] == [None, None]
 
 
+def test_attach_stacktrace_in_app(sentry_init, capture_events):
+    sentry_init(attach_stacktrace=True, in_app_exclude=["_pytest"])
+    events = capture_events()
+
+    capture_message("hi")
+
+    event, = events
+    thread, = event["threads"]["values"]
+    frames = thread["stacktrace"]["frames"]
+    pytest_frames = [f for f in frames if f["module"].startswith("_pytest")]
+    assert pytest_frames
+    assert all(f["in_app"] is False for f in pytest_frames)
+    assert any(f["in_app"] for f in frames)
+
+
 def test_attach_stacktrace_disabled():
     events = []
     hub = Hub(Client(attach_stacktrace=False, transport=events.append))
diff --git a/tests/utils/test_general.py b/tests/utils/test_general.py
index d3ed5c5a..a21a7553 100644
--- a/tests/utils/test_general.py
+++ b/tests/utils/test_general.py
@@ -16,6 +16,7 @@ from sentry_sdk.utils import (
     strip_string,
     filename_for_module,
     handle_in_app_impl,
+    iter_event_stacktraces,
 )
 from sentry_sdk._compat import text_type
 
@@ -167,3 +168,15 @@ def test_in_app(empty):
         in_app_include=empty,
         in_app_exclude=["foo"],
     ) == [{"module": "foo", "in_app": False}, {"module": "bar", "in_app": True}]
+
+
+def test_iter_stacktraces():
+    assert set(
+        iter_event_stacktraces(
+            {
+                "threads": {"values": [{"stacktrace": 1}]},
+                "stacktrace": 2,
+                "exception": {"values": [{"stacktrace": 3}]},
+            }
+        )
+    ) == {1, 2, 3}