Skip to content

Commit

Permalink
fix(wsgi): don't report stopiteration errors [backport 2.9] (#9828)
Browse files Browse the repository at this point in the history
Backport 99a9dca from #9788 to 2.9.

There are several integrations which send StopIteration errors as part
of normal operation (say, when we hit the last element of an interable).
We are currently flagging such spans as errors, when its expected
behavior.

After this change, we still generate the spans, but they are no longer
reported as errors.

Some salient points:

* "Errors" on spans are directly tied to UI/Datadog app behaviors and
are desired to be actionable by, or at least significant to, the
customer.
* The wsgi method in this issue is expected to raise StopIteration.
Whether or not it is caught, raising an exception is normal behavior for
this piece of code.
* The StopIteration raised by wsgi may be caught (customer doesn't care
and it should not be an error) or not caught (customer does care and it
should be an error). We don't know at the time of this span close if the
issue is caught or not, so we can't make a 100% informed decision.
* Given those points, the decision is to only ignore StopIteration in
wsgi explicitly for the method which is expected to raise StopIteration.
While the exception may not be caught elsewhere, we are relying on other
mechanisms (such as crash detection) to report the issue.

Related issue:
miguelgrinberg/flask-sock#64 (comment)


## Checklist

- [x] The PR description includes an overview of the change
- [x] The PR description articulates the motivation for the change
- [x] The change includes tests OR the PR description describes a
testing strategy
- [x] The PR description notes risks associated with the change, if any
- [x] Newly-added code is easy to change
- [x] The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- [x] The change includes or references documentation updates if
necessary
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate
- [x] All changes are related to the pull request's stated goal
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- [x] Testing strategy adequately addresses listed risks
- [x] Newly-added code is easy to change
- [x] Release note makes sense to a user of the library
- [x] If necessary, author has acknowledged and discussed the
performance implications of this PR as reported in the benchmarks PR
comment
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

Co-authored-by: Teague Bick <[email protected]>
  • Loading branch information
github-actions[bot] and tabgok authored Jul 17, 2024
1 parent 01291fc commit ffa42c6
Show file tree
Hide file tree
Showing 4 changed files with 126 additions and 0 deletions.
19 changes: 19 additions & 0 deletions ddtrace/contrib/wsgi/wsgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ def blocked_view():
not_blocked = False

core.dispatch("wsgi.block_decided", (blocked_view,))
stop_iteration_exception = None

if not_blocked:
core.dispatch("wsgi.request.prepare", (ctx, start_response))
Expand All @@ -137,6 +138,17 @@ def blocked_view():
start_response(str(status), headers)
closing_iterable = [content]
core.dispatch("wsgi.app.exception", (ctx,))
except StopIteration as e:
"""
WSGI frameworks emit `StopIteration` when closing connections
with Gunicorn as part of their standard workflow. We don't want to mark
these as errors for the UI since they are standard and expected
to be caught later.
Here we catch the exception and close out the request/app spans through the non-error
handling pathways.
"""
stop_iteration_exception = e
except BaseException:
core.dispatch("wsgi.app.exception", (ctx,))
raise
Expand All @@ -151,6 +163,13 @@ def blocked_view():
result = core.dispatch_with_results(
"wsgi.request.complete", (ctx, closing_iterable, self.app_is_iterator)
).traced_iterable

if stop_iteration_exception:
if result.value:
# Close the request and app spans
result.value._finish_spans()
core.dispatch("wsgi.app.success", (ctx, closing_iterable))
raise stop_iteration_exception
return result.value if result else []

def _traced_start_response(self, start_response, request_span, app_span, status, environ, exc_info=None):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
fixes:
- |
wsgi: Ensures the status of wsgi Spans are not set to error when a ``StopIteration`` exception is raised
marked the span as an error. With this change, ``StopIteration`` exceptions in this context will be ignored.
20 changes: 20 additions & 0 deletions tests/contrib/wsgi/test_wsgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,23 @@ def chunked_response_generator_error(start_response):
raise GeneratorExit()


def raise_stop_iteration_exception(start_response):
status = "200 OK"
headers = [("Content-type", "text/plain")]
start_response(status, headers)

# We are simulating a StopIteration here that is usually generated by
# WSGI implementations to indicate a socket has closed
raise StopIteration()


def application(environ, start_response):
if environ["PATH_INFO"] == "/error":
raise Exception("Oops!")
elif environ["PATH_INFO"] == "/baseException":
raise BaseException("base exception raised in wsgi app")
elif environ["PATH_INFO"] == "/stopIteration":
raise_stop_iteration_exception(start_response)
elif environ["PATH_INFO"] == "/chunked":
return chunked_response(start_response)
elif environ["PATH_INFO"] == "/generatorError":
Expand Down Expand Up @@ -277,6 +289,14 @@ def test_base_exception_in_wsgi_app_py3():
app.get("/baseException")


@snapshot(wait_for_num_traces=1)
def test_stop_iteration_in_wsgi_app_py3():
# StopIteration should not mark span as an error: https://github.com/miguelgrinberg/flask-sock/issues/64
with pytest.raises(StopIteration):
app = TestApp(wsgi.DDWSGIMiddleware(application))
app.get("/stopIteration")


@pytest.mark.snapshot(
token="tests.contrib.wsgi.test_wsgi.test_wsgi_base_middleware",
wait_for_num_traces=1,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
[[
{
"name": "wsgi.request",
"service": "wsgi",
"resource": "GET /stopIteration",
"trace_id": 0,
"span_id": 1,
"parent_id": 0,
"type": "web",
"error": 0,
"meta": {
"_dd.base_service": "",
"_dd.p.dm": "-0",
"_dd.p.tid": "669187f500000000",
"component": "wsgi",
"http.method": "GET",
"http.status_code": "200",
"http.status_msg": "OK",
"http.url": "http://localhost:80/stopIteration",
"language": "python",
"runtime-id": "66ae49fcbdcd41f2806b895660ff137f",
"span.kind": "server"
},
"metrics": {
"_dd.top_level": 1,
"_dd.tracer_kr": 1.0,
"_sampling_priority_v1": 1,
"process_id": 213
},
"duration": 3528433877,
"start": 1720813557991691963
},
{
"name": "wsgi.application",
"service": "wsgi",
"resource": "wsgi.application",
"trace_id": 0,
"span_id": 2,
"parent_id": 1,
"type": "",
"error": 0,
"meta": {
"_dd.base_service": "",
"component": "wsgi"
},
"duration": 2339998959,
"start": 1720813557991988422
},
{
"name": "wsgi.start_response",
"service": "wsgi",
"resource": "wsgi.start_response",
"trace_id": 0,
"span_id": 4,
"parent_id": 2,
"type": "web",
"error": 0,
"meta": {
"_dd.base_service": "",
"component": "wsgi",
"span.kind": "server"
},
"duration": 39750,
"start": 1720813557992143630
},
{
"name": "wsgi.response",
"service": "wsgi",
"resource": "wsgi.response",
"trace_id": 0,
"span_id": 3,
"parent_id": 1,
"type": "",
"error": 0,
"meta": {
"_dd.base_service": "",
"component": "wsgi",
"result_class": "tuple"
},
"duration": 1188001334,
"start": 1720813560332044089
}]]

0 comments on commit ffa42c6

Please sign in to comment.