fix(telemetry): stop blocking the caller and leaking the flush thread (#105)

[karlwaldman]

Decision first: the telemetry transport is USED, so it is fixed, not deleted

Issue #105 offers two branches — delete the collector if it has no consumer, or
fix its lifecycle if it does. It has a consumer: the collector is an advertised
public option on both clients, so deleting it is a breaking public API change,
which the issue explicitly forbids ("Do not silently remove an advertised option").

Grep on origin/main (7982b0b), source only:

$ grep -rn "telemetry" --include="*.py" oilpriceapi/ | grep -v "^oilpriceapi/telemetry.py"
oilpriceapi/client.py:116:        enable_telemetry: bool = False,
oilpriceapi/client.py:150:        # Store telemetry settings
oilpriceapi/client.py:224:        # Initialize telemetry (opt-in, disabled by default)
oilpriceapi/client.py:225:        from .telemetry import Telemetry
oilpriceapi/client.py:227:        self._telemetry = Telemetry(enabled=enable_telemetry)
oilpriceapi/client.py:295:                    self._telemetry.track_request(
oilpriceapi/client.py:401:            self._telemetry.track_request(
oilpriceapi/client.py:545:    def _sanitize_path_for_telemetry(method: str, path: str) -> str:
oilpriceapi/client.py:630:        self._telemetry.close()
oilpriceapi/async_client.py:96:        enable_telemetry: bool = False,
oilpriceapi/async_client.py:188:        from .telemetry import Telemetry
oilpriceapi/async_client.py:190:        self._telemetry = Telemetry(enabled=enable_telemetry)
oilpriceapi/async_client.py:257:                    self._telemetry.track_request(
oilpriceapi/async_client.py:355:            self._telemetry.track_request(
oilpriceapi/async_client.py:406:        self._telemetry.close()

OilPriceAPI(..., enable_telemetry=True) and AsyncOilPriceAPI(..., enable_telemetry=True)
are documented constructor parameters, and configure_telemetry / get_telemetry
are module-level API. Both stay. This PR changes no signature and removes no option.

Per-defect verdicts, re-verified against origin/main today

#	Defect	Verdict on current main	Evidence
1	track_request runs a blocking httpx.post(timeout=5) on the caller every 10 events; AsyncOilPriceAPI does it on the event loop	confirmed-live	telemetry.py:167-168 calls self._flush(); telemetry.py:192 posts synchronously under a comment claiming "non-blocking"
2	close() only flushes; never sets enabled=False, never stops the daemon thread	confirmed-live	telemetry.py:213-216
3	configure_telemetry replaces the global instance without stopping the prior thread	confirmed-live	telemetry.py:242
4	A telemetry failure can alter/fail a request result	confirmed-live	client.py:295 and async_client.py:257 call track_request unguarded on the success path, immediately before return response.json()

The change (smallest correct; no new telemetry platform)

oilpriceapi/telemetry.py

• track_request buffers and sets an Event; it never delivers. The buffer is
  bounded at MAX_BUFFERED_EVENTS = 1000 with the oldest dropped, so an
  unreachable collector cannot grow memory without limit.
• The background daemon thread is now the only place _flush runs. It waits on
  the event with flush_interval as the timeout, so a full buffer wakes it
  immediately and close() wakes it at once.
• close() sets _stopping, clears enabled, wakes the thread for a final
  drain, and joins with a bounded close_timeout (default 2.0s). It is
  idempotent, safe on a never-started collector, and never raises.
• configure_telemetry closes the previous global instance before returning.

oilpriceapi/client.py and oilpriceapi/async_client.py

• Both call sites in each client now go through _track_telemetry(**fields),
  a byte-identical guard in both files that swallows any telemetry exception
  into logger.debug. close() in both also guards self._telemetry.close().

Red

New tests run against unmodified origin/main sources
(git checkout origin/main -- oilpriceapi/telemetry.py oilpriceapi/client.py oilpriceapi/async_client.py,
then restored):

E           AssertionError: track_request blocked the caller for 3.00s; delivery must not run on the caller's thread
E           assert 3.0022... < 0.5
tests/unit/test_telemetry_lifecycle.py:109: AssertionError

E           AssertionError: ten client calls took 3.01s with a slow telemetry sink
E           assert 3.0074... < 1.0
tests/unit/test_telemetry_lifecycle.py:128: AssertionError

E           AssertionError: ten async calls took 3.01s with a slow telemetry sink
E           assert 3.0098... < 1.0
tests/unit/test_telemetry_lifecycle.py:155: AssertionError

        telemetry.close()
>       assert telemetry.enabled is False
E       assert True is False
tests/unit/test_telemetry_lifecycle.py:177: AssertionError

        client.close()
        thread.join(timeout=2.0)
>       assert not thread.is_alive()
E       assert not True
E        +    where is_alive = <Thread(Thread-5 (_flush_loop), started daemon 6184710144)>.is_alive
tests/unit/test_telemetry_lifecycle.py:198: AssertionError

>           assert not first_thread.is_alive(), "configure_telemetry leaked the previous flush thread"
E           AssertionError: configure_telemetry leaked the previous flush thread
tests/unit/test_telemetry_lifecycle.py:233: AssertionError

oilpriceapi/client.py:295: in request
    self._telemetry.track_request(
E       RuntimeError: telemetry backend exploded
tests/unit/test_telemetry_lifecycle.py:257: RuntimeError

>       sync_src = inspect.getsource(OilPriceAPI._track_telemetry)
E       AttributeError: type object 'OilPriceAPI' has no attribute '_track_telemetry'
tests/unit/test_telemetry_lifecycle.py:298: AttributeError

FAILED tests/unit/test_telemetry_lifecycle.py::test_ten_tracked_events_do_not_block_the_calling_thread
FAILED tests/unit/test_telemetry_lifecycle.py::test_sync_client_requests_are_not_blocked_by_telemetry_delivery
FAILED tests/unit/test_telemetry_lifecycle.py::test_async_client_event_loop_is_not_blocked_by_telemetry
FAILED tests/unit/test_telemetry_lifecycle.py::test_close_stops_the_background_thread_and_is_idempotent
FAILED tests/unit/test_telemetry_lifecycle.py::test_client_close_stops_telemetry_thread
FAILED tests/unit/test_telemetry_lifecycle.py::test_async_client_close_stops_telemetry_thread
FAILED tests/unit/test_telemetry_lifecycle.py::test_configure_telemetry_stops_the_previous_instance
FAILED tests/unit/test_telemetry_lifecycle.py::test_sync_request_result_survives_a_telemetry_failure
FAILED tests/unit/test_telemetry_lifecycle.py::test_async_request_result_survives_a_telemetry_failure
FAILED tests/unit/test_telemetry_lifecycle.py::test_sync_and_async_clients_share_identical_telemetry_handling
================== 10 failed, 2 passed, 30 warnings in 15.30s ==================

The two that passed pre-fix are the disabled-mode guards (no thread, no network),
which already held and are kept as regression pins.

Green

tests/unit/test_telemetry_lifecycle.py ............                      [100%]
======================= 12 passed, 30 warnings in 0.47s ========================

The 15.30s -> 0.47s drop is the defect itself: the pre-fix runtime is five
rounds of 3s caller-thread blocking on a deliberately slow sink.

Full suite, same venv, before and after:

before (clean origin/main):  3 failed, 864 passed, 63 skipped in 6.38s
after  (this branch):        3 failed, 876 passed, 63 skipped in 5.13s

876 = 864 + the 12 new tests. The 3 failures are unchanged and environmental:
tests/integration/test_demo_contract.py makes live calls and is getting HTTP 429.

ruff check oilpriceapi/ -> All checks passed. mypy oilpriceapi/ --ignore-missing-imports -> Success: no issues found in 47 source files. CI is green on all five matrix Pythons (3.8-3.12) plus the live-API job.

Sync and async stayed identical

test_sync_and_async_clients_share_identical_telemetry_handling pins it:
inspect.getsource(OilPriceAPI._track_telemetry) == inspect.getsource(AsyncOilPriceAPI._track_telemetry),
neither request calls track_request directly any more, both call
_track_telemetry, and both close() bodies call self._telemetry.close().
That test fails if either file is changed without the other.

Test hygiene

An autouse fixture replaces oilpriceapi.telemetry.httpx.post with a function
that raises if called, and each test then installs a local in-process sink. API
calls are mocked at httpx.Client.request / httpx.AsyncClient.request, the
convention the rest of tests/unit already uses -- the first push used respx,
which is not in the [dev] extra and errored collection on every matrix Python;
the second commit removes that dependency with no change to the assertions. No
test in this file can reach the network. Every test stops its collector in a
finally, so no thread outlives a test.

Scope notes

• No version bump. 1.14.0 is already on PyPI and is unchanged here.
• No CHANGELOG entry: 1.14.0 is published, and the only ## [Unreleased]
  heading sits mid-file below 1.12.7 while a release-readiness test pins it to
  exactly one occurrence. Flagging that heading placement for the maintainer
  rather than fixing it in a telemetry PR.
• Not merged, no auto-merge, nothing published.

Closes #105

🤖 Generated with Claude Code

https://claude.ai/code/session_015ao5paex73xXvuM424Libo

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: facd03c7-86be-40c4-afc6-9c26c5638116

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}