Sitelet https://github.com/Stephenson-Software/trace-client-python/pull/4
Skip to content

Keep report() from raising on a non-string name and the sender alive on a scheme-less URL - #4

Merged
dmccoystephenson merged 1 commit into
mainfrom
trace/never-raise-on-bad-input
Sep 27, 2026
Merged

Keep report() from raising on a non-string name and the sender alive on a scheme-less URL#4
dmccoystephenson merged 1 commit into
mainfrom
trace/never-raise-on-bad-input

Conversation

@dmccoystephenson

@dmccoystephenson dmccoystephenson commented Sep 24, 2026

Copy link
Copy Markdown
Member

Summary

  • report(): the blank-name check was moved inside the existing try. A non-string name such as report(123) used to raise AttributeError into the host program. Now it is dropped and logged at DEBUG (could not queue 123: ...), which is what the README's "Never raises" promise says happens.
  • _send(): the urllib.request.Request is now built inside the existing try. A base_url with no scheme ("trace.example.org") used to make Request.__init__ raise ValueError on the sender thread, which killed the thread and printed a traceback to the host's stderr. Now each report is logged at DEBUG as could not deliver and the thread keeps running.
  • The constructor is unchanged. It does not start rejecting URLs it used to accept, so no host program that constructs a client today can start failing at construction.

Closes #2
Closes #3

Test plan

  • python3 -m unittest -v on Python 3.8.10: 23 tests ran, OK
  • Regression check: with the trace_client.py change stashed, test_report_does_not_raise_for_a_name_that_is_not_a_string errors (AttributeError) and test_a_base_url_without_a_scheme_is_logged_not_a_dead_thread fails (threading.excepthook received the ValueError). Both pass once the change is restored.
  • CI Build: test (3.8), test (3.10), test (3.12)

Java parity

No Java-visible behavior changed: the wire format, the opt-out values and the disabled_reason strings are untouched. Both fixes cover inputs specific to Python (untyped name, and URL parsing that happens lazily on the sender thread). Nothing has to be done in trace-client-java.

Notes

  • Only ubuntu-latest is covered by CI. Nothing in this diff depends on the platform or on Pyodide.
  • No other open issues existed at triage time, so nothing was deferred.

This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

🤖 Generated with Claude Code

drafted by Claude on behalf of Daniel Stephenson

@dmccoystephenson @claude
Keep report() from raising on a non-string name and the sender alive …
0fdccde 
…on a scheme-less URL

report() checked the name before its try, so report(123) raised
AttributeError into the host program. _send() built the Request before
its try, so a base_url with no scheme killed the sender thread with a
traceback on stderr. Both now fall inside the existing try and are
logged at DEBUG like every other dropped report.

Closes #2
Closes #3

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@dmccoystephenson

dmccoystephenson commented Sep 24, 2026

Copy link
Copy Markdown
Member Author

Self-review rubric (checked against CI run 35967784767 on the PR head):

  • Scope: PASS. Only trace_client/trace_client.py (the two fixes) and tests/test_trace_client.py (two regression tests) were modified: +34/−8.
  • Tests-new: PASS. No new public names were added.
  • Tests-fix: PASS. Confirmed by stash-and-run on Python 3.8.10. With the source change stashed, test_report_does_not_raise_for_a_name_that_is_not_a_string errored (AttributeError) and test_a_base_url_without_a_scheme_is_logged_not_a_dead_thread failed (threading.excepthook received ValueError: unknown url type). With the change restored, all 23 tests pass.
  • Sibling structure / renames: PASS. No new files and no renames.
  • Docs: PASS. The README's "Never raises" row and the class docstring already describe the behavior this PR delivers. No wording needed to change.
  • Issue resolution: PASS. report() raises AttributeError for a non-string name, breaking the never-raises promise #2 (report() name check) and A base_url without a scheme kills the sender thread and prints a traceback to stderr #3 (Request built outside the try) are both changed at the named lines.
  • CI: PASS. test (3.8), test (3.10) and test (3.12) are green.
  • Stdlib-only: PASS. The diff adds no import lines, and pyproject.toml is untouched.
  • Py3.8 floor: PASS. test (3.8) is green. threading.excepthook, which the new test patches, has existed since 3.8.
  • Never-raises / never-blocks: PASS. No new blocking call was added. Both moved statements now sit inside existing try blocks that log at DEBUG.
  • Log level: PASS. The diff adds no _LOG. calls, and the existing [trace] DEBUG messages are reused.
  • Version agreement: PASS. The version is untouched, and 0.2.0 still appears consistently.
  • Exports: PASS. No public names changed.
  • Java parity noted: PASS. The PR body states that no Java-visible behavior changed.

Observations outside the diff:

  • trace_client/trace_client.py:176 (_drain) still has no catch-all around self._send(body). After this PR, every known raising statement in _send is inside its try, so nothing currently reaches it. An outer guard would be defense in depth for future edits to _send. It is a candidate for a separate issue and was left out here to keep this PR scoped.
  • Do-not-auto-merge list: no protected path is touched (no pyproject.toml, no version string, no wire-format or opt-out change, no workflow). The merge still needs a human, because this dispatch is not authorized to merge.

This review comment was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

drafted by Claude on behalf of Daniel Stephenson

@dmccoystephenson dmccoystephenson mentioned this pull request Sep 27, 2026
Closed
@dmccoystephenson
dmccoystephenson merged commit 244b1b9 into main Sep 27, 2026
3 checks passed
dmccoystephenson added a commit that referenced this pull request Sep 30, 2026
@dmccoystephenson @claude
Merge main (#4: never raise on bad input) into program-version-on-eve…
022efc7 
…ry-event

Keeps #4's name check inside report()'s try and applies _with_version
there; the two tests #4 added now pass a program version.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AuTszT2gqv7nLVYrtKD4ji
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

1 participant

@dmccoystephenson