Skip to content

Commit 904966d

Browse files
fix(lib-injection): don't close stdout/stderr (#13926)
# Overview This commit resolves an issue where telemetry forwarding never completes when DD_APM_INSTRUMENTATION_DEBUG is enabled. We are closing the pipe from the Python side, which means the telemetry forwarder will fill the buffer and be unable to complete. # Test I updated the `telemetry-forwarder.sh` to look like the following: ```bash #!/usr/bin/env bash # Generate 64KB of output on stderr (writes to fd 2) # This creates 1024 lines of 64 bytes each = 64KB for i in {1..1024}; do echo "This is a line of telemetry data to fill the pipe buffer ---- $i" >&2 done # Write stdin + $1 to a file as before echo "$1 $(</dev/stdin)" >>mock-telemetry.out ``` I then ran this on main and this branch to confirm `mock-telemetry.out` is written on this branch but never gets written on main: ```bash DD_APM_INSTRUMENTATION_DEBUG="true" DD_INJECTION_ENABLED=true DD_TRACE_DEBUG=true PYTHONPATH=/Users/mark.spicer/go/src/github.com/DataDog/dd-trace-py/lib-injection/sources:$PYTHONPATH DD_TELEMETRY_FORWARDER_PATH=/Users/mark.spicer/go/src/github.com/DataDog/dd-trace-py/lib-injection/sources/telemetry-forwarder.sh python3 /Users/mark.spicer/go/src/github.com/DataDog/injector-dev/apps/python/app.py ``` ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - 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: Brett Langdon <brett.langdon@datadoghq.com>
1 parent 7ed22e2 commit 904966d

File tree

2 files changed

+6
-6
lines changed

2 files changed

+6
-6
lines changed

lib-injection/sources/sitecustomize.py

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -182,8 +182,8 @@ def _send_telemetry(event):
182182
p = subprocess.Popen(
183183
[FORWARDER_EXECUTABLE, "library_entrypoint"],
184184
stdin=subprocess.PIPE,
185-
stdout=subprocess.PIPE,
186-
stderr=subprocess.PIPE,
185+
stdout=subprocess.DEVNULL,
186+
stderr=subprocess.DEVNULL,
187187
universal_newlines=True,
188188
)
189189
# Mimic Popen.__exit__ which was added in Python 3.3
@@ -199,10 +199,6 @@ def _send_telemetry(event):
199199
finally:
200200
if p.stdin:
201201
p.stdin.close()
202-
if p.stderr:
203-
p.stderr.close()
204-
if p.stdout:
205-
p.stdout.close()
206202

207203
# backwards compatible `p.wait(1)`
208204
start = time.time()
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
fixes:
3+
- |
4+
lib-injection: Fix a bug preventing the Single Step Instrumentation (SSI) telemetry forwarder from completing when debug logging was enabled.

0 commit comments

Comments
 (0)