feat: Add twisted instrumentation and unit/integration tests by CagriYonca · Pull Request #888 · instana/python-sensor

CagriYonca · 2026-07-13T09:27:21Z

Added twisted webserver instrumentation and related tests.

Signed-off-by: Cagri Yonca <cagri@ibm.com>

sonarqubecloud · 2026-07-13T12:26:46Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
99.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

GSVarsha · 2026-07-20T09:36:34Z

+    def test_get(self) -> None:
+        with self.tracer.start_as_current_span("test"):
+            response = requests.get(testenv["twisted_server"] + "/")
+
+        time.sleep(0.5)
+        spans = self.recorder.queued_spans()
+        assert len(spans) == 3
+
+        server_span = get_first_span_by_name(spans, "twisted-server")
+        client_span = get_first_span_by_name(spans, "urllib3")
+        test_span = get_first_span_by_name(spans, "sdk")
+
+        assert server_span
+        assert client_span
+        assert test_span
+
+        assert not get_current_span().is_recording()
+
+        # Same traceId
+        traceId = test_span.t
+        assert traceId == client_span.t
+        assert traceId == server_span.t
+
+        # Parent relationships
+        assert client_span.p == test_span.s
+        assert server_span.p == client_span.s
+
+        # Error logging
+        assert not test_span.ec
+        assert not client_span.ec
+        assert not server_span.ec
+
+        assert server_span.data["http"]["status"] == 200
+        assert testenv["twisted_server"] + "/" == server_span.data["http"]["url"]
+        assert not server_span.data["http"].get("params")
+        assert server_span.data["http"]["method"] == "GET"
+
+        assert "X-INSTANA-T" in response.headers
+        assert response.headers["X-INSTANA-T"] == hex_id(traceId)
+        assert "X-INSTANA-S" in response.headers
+        assert response.headers["X-INSTANA-S"] == hex_id(server_span.s)
+        assert "X-INSTANA-L" in response.headers
+        assert response.headers["X-INSTANA-L"] == "1"
+        assert "Server-Timing" in response.headers
+        assert response.headers["Server-Timing"] == f"intid;desc={hex_id(traceId)}"
+
+    def test_post(self) -> None:
+        with self.tracer.start_as_current_span("test"):
+            _ = requests.post(testenv["twisted_server"] + "/", data={"hello": "post"})
+
+        time.sleep(0.5)
+        spans = self.recorder.queued_spans()
+        assert len(spans) == 3
+
+        server_span = get_first_span_by_name(spans, "twisted-server")
+        client_span = get_first_span_by_name(spans, "urllib3")
+        test_span = get_first_span_by_name(spans, "sdk")
+
+        assert server_span
+        assert client_span
+        assert test_span
+
+        assert not get_current_span().is_recording()
+
+        # Same traceId
+        traceId = test_span.t
+        assert traceId == client_span.t
+        assert traceId == server_span.t
+
+        # Parent relationships
+        assert client_span.p == test_span.s
+        assert server_span.p == client_span.s
+
+        # Error logging
+        assert not test_span.ec
+        assert not client_span.ec
+        assert not server_span.ec
+
+        assert server_span.data["http"]["status"] == 200
+        assert testenv["twisted_server"] + "/" == server_span.data["http"]["url"]
+        assert not server_span.data["http"].get("params")
+        assert server_span.data["http"]["method"] == "POST"
+
+    def test_get_301(self) -> None:
+        with self.tracer.start_as_current_span("test"):
+            _ = requests.get(testenv["twisted_server"] + "/301", allow_redirects=False)
+
+        time.sleep(0.5)
+        spans = self.recorder.queued_spans()
+        assert len(spans) == 3
+
+        server_span = get_first_span_by_name(spans, "twisted-server")
+        client_span = get_first_span_by_name(spans, "urllib3")
+        test_span = get_first_span_by_name(spans, "sdk")
+
+        assert server_span
+        assert client_span
+        assert test_span
+
+        assert not get_current_span().is_recording()
+
+        # Same traceId
+        traceId = test_span.t
+        assert traceId == client_span.t
+        assert traceId == server_span.t
+
+        # Parent relationships
+        assert client_span.p == test_span.s
+        assert server_span.p == client_span.s
+
+        assert server_span.data["http"]["status"] == 301
+        assert testenv["twisted_server"] + "/301" == server_span.data["http"]["url"]
+        assert server_span.data["http"]["method"] == "GET"
+
+    def test_get_404(self) -> None:
+        with self.tracer.start_as_current_span("test"):
+            _ = requests.get(testenv["twisted_server"] + "/404")
+
+        time.sleep(0.5)
+        spans = self.recorder.queued_spans()
+        assert len(spans) == 3
+
+        server_span = get_first_span_by_name(spans, "twisted-server")
+        client_span = get_first_span_by_name(spans, "urllib3")
+        test_span = get_first_span_by_name(spans, "sdk")
+
+        assert server_span
+        assert client_span
+        assert test_span
+
+        assert server_span.data["http"]["status"] == 404
+        assert testenv["twisted_server"] + "/404" == server_span.data["http"]["url"]
+
+    def test_get_500(self) -> None:
+        with self.tracer.start_as_current_span("test"):
+            _ = requests.get(testenv["twisted_server"] + "/500")
+
+        time.sleep(0.5)
+        spans = self.recorder.queued_spans()
+        assert len(spans) == 3
+
+        server_span = get_first_span_by_name(spans, "twisted-server")
+        client_span = get_first_span_by_name(spans, "urllib3")
+        test_span = get_first_span_by_name(spans, "sdk")
+
+        assert server_span
+        assert client_span
+        assert test_span
+
+        assert not get_current_span().is_recording()
+
+        # Same traceId
+        traceId = test_span.t
+        assert traceId == client_span.t
+        assert traceId == server_span.t
+
+        assert server_span.data["http"]["status"] == 500
+        assert testenv["twisted_server"] + "/500" == server_span.data["http"]["url"]
+
+        # Error logging
+        assert not test_span.ec
+        assert client_span.ec == 1
+        assert server_span.ec == 1
+
+    def test_get_with_params_to_scrub(self) -> None:
+        with self.tracer.start_as_current_span("test"):
+            _ = requests.get(testenv["twisted_server"] + "/", params={"secret": "yeah"})
+
+        time.sleep(0.5)
+        spans = self.recorder.queued_spans()
+        assert len(spans) == 3
+
+        server_span = get_first_span_by_name(spans, "twisted-server")
+        client_span = get_first_span_by_name(spans, "urllib3")
+        test_span = get_first_span_by_name(spans, "sdk")
+
+        assert server_span
+        assert client_span
+        assert test_span
+
+        assert not get_current_span().is_recording()
+
+        # Same traceId
+        traceId = test_span.t
+        assert traceId == client_span.t
+        assert traceId == server_span.t
+
+        assert server_span.data["http"]["status"] == 200
+        assert testenv["twisted_server"] + "/" == server_span.data["http"]["url"]
+        assert server_span.data["http"]["params"] == "secret=<redacted>"
+
+    def test_request_header_capture(self) -> None:
+        original_extra_http_headers = agent.options.extra_http_headers
+        agent.options.extra_http_headers = ["X-Capture-This", "X-Capture-That"]
+
+        with self.tracer.start_as_current_span("test"):
+            _ = requests.get(
+                testenv["twisted_server"] + "/",
+                headers={"X-Capture-This": "this", "X-Capture-That": "that"},
+            )
+
+        agent.options.extra_http_headers = original_extra_http_headers
+
+        time.sleep(0.5)
+        spans = self.recorder.queued_spans()
+        assert len(spans) == 3
+
+        server_span = get_first_span_by_name(spans, "twisted-server")
+        client_span = get_first_span_by_name(spans, "urllib3")
+        test_span = get_first_span_by_name(spans, "sdk")
+
+        assert server_span
+        assert client_span
+        assert test_span
+
+        assert "X-Capture-This" in server_span.data["http"]["header"]
+        assert server_span.data["http"]["header"]["X-Capture-This"] == "this"
+        assert "X-Capture-That" in server_span.data["http"]["header"]
+        assert server_span.data["http"]["header"]["X-Capture-That"] == "that"
+
+    def test_response_header_capture(self) -> None:
+        original_extra_http_headers = agent.options.extra_http_headers
+        agent.options.extra_http_headers = ["X-Capture-This-Too", "X-Capture-That-Too"]
+
+        with self.tracer.start_as_current_span("test"):
+            _ = requests.get(testenv["twisted_server"] + "/response_headers")
+
+        agent.options.extra_http_headers = original_extra_http_headers
+
+        time.sleep(0.5)
+        spans = self.recorder.queued_spans()
+        assert len(spans) == 3
+
+        server_span = get_first_span_by_name(spans, "twisted-server")
+        client_span = get_first_span_by_name(spans, "urllib3")
+        test_span = get_first_span_by_name(spans, "sdk")
+
+        assert server_span
+        assert client_span
+        assert test_span
+
+        assert "X-Capture-This-Too" in server_span.data["http"]["header"]
+        assert server_span.data["http"]["header"]["X-Capture-This-Too"] == "this too"
+        assert "X-Capture-That-Too" in server_span.data["http"]["header"]
+        assert server_span.data["http"]["header"]["X-Capture-That-Too"] == "that too"


The first 8 test methods in TestTwistedClient are functionally identical to the 8 tests of the same name in TestTwistedServer. Every one of them:

Looks up get_first_span_by_name(spans, "urllib3") — the variable is even named client_span, but it holds the urllib3 span, not a twisted-client span

Never checks for a span named "twisted-client" at all

GSVarsha · 2026-07-21T05:49:14Z

+            span.set_attribute(SpanAttributes.HTTP_METHOD, method_str)
+
+            # Build / augment headers with trace correlation
+            from twisted.web.http_headers import Headers as TwistedHeaders


Please move this to the top, in the outermost try block

GSVarsha · 2026-07-21T05:49:37Z

+    def _finish_tracing(result: Any, span: "InstanaSpan") -> Any:
+        """Callback/errback attached to the Agent.request Deferred."""
+        try:
+            from twisted.python.failure import Failure


Same as here

GSVarsha · 2026-07-21T06:01:42Z

+
+            # Capture outgoing request headers
+            headers_dict = {
+                k.decode("latin-1"): v[0].decode("latin-1")


As mentioned in the official twisted doc here, header names are encoded using ISO-8859-1 (latin-1) and header values are encoded using UTF-8.

getAllRawHeaders() returns raw bytes in both cases, so the caller must decode each correctly:

Suggested change

k.decode("latin-1"): v[0].decode("latin-1")

k.decode("latin-1"): v[0].decode("utf-8")

GSVarsha · 2026-07-21T06:02:01Z

+
+            # Capture response headers
+            headers_dict = {
+                k.decode("latin-1"): v[0].decode("latin-1")


Same as here

GSVarsha · 2026-07-21T06:02:10Z

+            parent_context = None
+            if request.requestHeaders:
+                headers_dict: dict[str, str] = {
+                    k.decode("latin-1"): v[0].decode("latin-1")


Same as here

GSVarsha · 2026-07-21T06:02:19Z

+
+                    # Capture response headers
+                    response_hdrs = {
+                        k.decode("latin-1"): v[0].decode("latin-1")


Same as here

GSVarsha

Few requests

pvital · 2026-07-13T14:47:05Z

@@ -0,0 +1 @@
+# (c) Copyright IBM Corp. 2026


If you don't load or start anything in the __init__.py file, it must be empty.

Ack, I don't think we need to copyright an empty file.

pvital · 2026-07-22T12:19:03Z

+            logger.debug("twisted request_with_instana", exc_info=True)
+            return wrapped(*argv, **kwargs)
+
+    def _finish_tracing(result: Any, span: "InstanaSpan") -> Any:


If you expect result to be twisted.web.iweb.IResponse as commented on line 108 below, and the return of this method is the result itself, why set the types as Any?

Suggested change

def _finish_tracing(result: Any, span: "InstanaSpan") -> Any:

def _finish_tracing(result: twisted.web.iweb.IResponse, span: "InstanaSpan") -> twisted.web.iweb.IResponse:

Since result can be either a valid response or a failure, I think a union of them would be more precise.

Suggested change

def _finish_tracing(result: Any, span: "InstanaSpan") -> Any:

def _finish_tracing(result: Union[IResponse, Failure], span: "InstanaSpan") -> Union[IResponse, Failure]:

pvital · 2026-07-22T12:43:10Z

+            if isinstance(result, Failure):
+                span.record_exception(result.value)
+                if span.is_recording():
+                    span.end()
+                return result


When result is a Failure, the code calls span.end() and then returns result — bypassing the finally block entirely. That is correct for the happy path. However, if span.record_exception() or span.end() raises an unexpected exception, control falls through to the except Exception block at line 123, and then the finally block at line 125 unconditionally calls span.end() again (because span.is_recording() might still return True if end() failed before completing). This causes span.end() to be called twice.

Suggested change

if isinstance(result, Failure):

span.record_exception(result.value)

if span.is_recording():

span.end()

return result

if isinstance(result, Failure):

span.record_exception(result.value)

return result

Woah that's cool, I just learned that the finally block gets executed even with a return in the try block!
Ack on this one too 👍🏼

P.S. I found this blog interesting: https://www.raravind.com/blog/data-science/do-you-really-understand-try-finally-in-python

pvital · 2026-07-23T13:58:31Z

+            raw_host = request.getHeader(b"host") or b""
+            host = (
+                raw_host.decode("latin-1") if isinstance(raw_host, bytes) else raw_host
+            )


request.getHeader() expects an AnyStr, returning the same or None. As an AnyStr can be bytes or str, I think it would be best if we call it using str to already receive it as a decoded string.

Suggested change

raw_host = request.getHeader(b"host") or b""

host = (

raw_host.decode("latin-1") if isinstance(raw_host, bytes) else raw_host

)

host = request.getHeader("host") or ""

pvital · 2026-07-23T13:59:49Z

+            raw_path = request.path
+            path = (
+                raw_path.decode("latin-1") if isinstance(raw_path, bytes) else raw_path
+            )


Doesn't the following work?

Suggested change

raw_path = request.path

path = (

raw_path.decode("latin-1") if isinstance(raw_path, bytes) else raw_path

)

path = request.getHeader("path") or ""

CagriYonca self-assigned this Jul 13, 2026

CagriYonca force-pushed the twisted branch 4 times, most recently from 1585ae8 to b116c82 Compare July 13, 2026 11:12

feat: Add twisted instrumentation and unit/integration tests

e558a6f

Signed-off-by: Cagri Yonca <cagri@ibm.com>

CagriYonca force-pushed the twisted branch from 4624e6d to e558a6f Compare July 13, 2026 12:10

CagriYonca marked this pull request as ready for review July 13, 2026 12:10

CagriYonca requested a review from a team as a code owner July 13, 2026 12:10

pvital added the feature label Jul 13, 2026

GSVarsha reviewed Jul 20, 2026

View reviewed changes

GSVarsha reviewed Jul 21, 2026

View reviewed changes

GSVarsha requested changes Jul 21, 2026

View reviewed changes

pvital requested changes Jul 23, 2026

View reviewed changes

	k.decode("latin-1"): v[0].decode("latin-1")
	k.decode("latin-1"): v[0].decode("utf-8")

	def _finish_tracing(result: Any, span: "InstanaSpan") -> Any:
	def _finish_tracing(result: twisted.web.iweb.IResponse, span: "InstanaSpan") -> twisted.web.iweb.IResponse:

	def _finish_tracing(result: Any, span: "InstanaSpan") -> Any:
	def _finish_tracing(result: Union[IResponse, Failure], span: "InstanaSpan") -> Union[IResponse, Failure]:

Uh oh!

Conversation

CagriYonca commented Jul 13, 2026

Uh oh!

sonarqubecloud Bot commented Jul 13, 2026

Quality Gate passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GSVarsha Jul 21, 2026 • 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!

GSVarsha left a comment

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!

GSVarsha Jul 23, 2026 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GSVarsha Jul 21, 2026 •

edited

Loading

GSVarsha Jul 23, 2026 •

edited

Loading