Codex/feat tracking delivery mvp - #12
Merged
Merged
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements a conversion tracking and event queuing system for the Convert Python SDK. It introduces a TrackingQueue for buffering events in-process, adds a TrackingConfig for batching and data enrichment settings, and extends the HttpxTransport to handle tracking delivery. The Context class now supports queuing conversion events with deduplication logic and provides a release_queues method for explicit flushing. A critical issue was identified in the TrackingQueue.release method where holding a thread lock during network I/O could cause performance bottlenecks and potential infinite loops upon transport failures.
Comment on lines
+106
to
+151
| def release(self, reason: str | None = None) -> TrackingFlushResult: | ||
| """Send queued events through the configured transport in batches.""" | ||
|
|
||
| if reason is not None and not isinstance(reason, str): | ||
| raise TypeError("reason must be a string or None") | ||
|
|
||
| delivered_event_count = 0 | ||
| delivered_batch_count = 0 | ||
| with self._lock: | ||
| if not self._pending: | ||
| return TrackingFlushResult( | ||
| attempted=False, | ||
| delivered_event_count=0, | ||
| delivered_batch_count=0, | ||
| remaining_event_count=0, | ||
| reason=reason, | ||
| ) | ||
|
|
||
| while self._pending: | ||
| batch = tuple(self._pending[: self._tracking_config.batch_size]) | ||
| payload = serialize_tracking_payload( | ||
| batch, | ||
| source=self._tracking_config.source, | ||
| enrich_data=self._tracking_config.enrich_data, | ||
| ) | ||
| self._transport.send_tracking( | ||
| TrackingRequest( | ||
| sdk_key=self._sdk_key, | ||
| sdk_key_secret=self._sdk_key_secret, | ||
| account_id=self._account_id, | ||
| project_id=self._project_id, | ||
| payload=payload, | ||
| transport=self._transport_config, | ||
| ) | ||
| ) | ||
| del self._pending[: len(batch)] | ||
| delivered_batch_count += 1 | ||
| delivered_event_count += len(batch) | ||
|
|
||
| return TrackingFlushResult( | ||
| attempted=True, | ||
| delivered_event_count=delivered_event_count, | ||
| delivered_batch_count=delivered_batch_count, | ||
| remaining_event_count=0, | ||
| reason=reason, | ||
| ) |
There was a problem hiding this comment.
The current implementation of release has two critical issues:
- Lock Contention: The lock is held during network I/O (
send_tracking), which can block other threads from enqueuing events for the duration of all batched network requests. In a highly concurrent application, this can become a significant performance bottleneck. - Infinite Loop on Error: If
send_trackingraises a persistent exception (e.g., a 4xx error), the events are not removed from the queue. Thewhileloop will then attempt to send the same failing batch repeatedly, causing an infinite loop and high CPU usage.
To address this, the method should be refactored to release the lock before performing network I/O and to handle transport errors gracefully to prevent infinite loops and event loss.
def release(self, reason: str | None = None) -> TrackingFlushResult:
"""Send queued events through the configured transport in batches."""
if reason is not None and not isinstance(reason, str):
raise TypeError("reason must be a string or None")
with self._lock:
if not self._pending:
return TrackingFlushResult(
attempted=False,
delivered_event_count=0,
delivered_batch_count=0,
remaining_event_count=0,
reason=reason,
)
events_to_send = self._pending
self._pending = []
delivered_event_count = 0
delivered_batch_count = 0
try:
while events_to_send:
batch = tuple(events_to_send[: self._tracking_config.batch_size])
payload = serialize_tracking_payload(
batch,
source=self._tracking_config.source,
enrich_data=self._tracking_config.enrich_data,
)
self._transport.send_tracking(
TrackingRequest(
sdk_key=self._sdk_key,
sdk_key_secret=self._sdk_key_secret,
account_id=self._account_id,
project_id=self._project_id,
payload=payload,
transport=self._transport_config,
)
)
del events_to_send[: len(batch)]
delivered_batch_count += 1
delivered_event_count += len(batch)
except Exception:
# Re-queue unsent events on failure to prevent event loss
with self._lock:
self._pending = events_to_send + self._pending
raise
with self._lock:
remaining_count = len(self._pending)
return TrackingFlushResult(
attempted=True,
delivered_event_count=delivered_event_count,
delivered_batch_count=delivered_batch_count,
remaining_event_count=remaining_count,
reason=reason,
)
abbaseya
added a commit
that referenced
this pull request
Aug 8, 2026
All eight open Dependabot alerts on this repo sit in `yarn.lock` — the dev-only semantic-release tooling. None of them reach the published wheel/sdist (httpx is still the only runtime dependency). Three of the four highs were already reachable inside the existing semver ranges; the fourth (sigstore GHSA-52v5-jr5w-gjxr, `certificateOIDs` verification constraints silently dropped) needed sigstore >= 4.1.1, which only arrives through a major bump: semantic-release 24.2.9 -> 25.0.9 @semantic-release/npm 12.0.2 -> 13.1.5 npm 10.9.8 -> 11.19.0 libnpmpublish 10 -> 11.2.0 }-> sigstore ^3 -> ^4 (4.1.1) pacote 19 -> 21.5.1 } @sigstore/core 2.0.0 -> 3.2.1 tar 7.5.16 -> 7.5.22 `@semantic-release/github` moves to ^12 to match what semantic-release 25 depends on — leaving it at ^11 would hoist the older copy to the project root and shadow the one core resolves. Resolved (4 high, 4 medium — no criticals were open): #14 high ip-address 10.2.0 -> 10.4.0 (needs >= 10.3.1) #9 high brace-expansion 2.1.1 -> 5.0.9 (needs >= 2.1.2) #7 high js-yaml 4.2.0 -> 4.3.1 (needs >= 4.3.0) #2 high sigstore 3.1.0 -> 4.1.1 (needs >= 4.1.1) #13 medium ip-address (same bump as #14) #12 medium ip-address (same bump as #14) #4 medium tar 7.5.16 -> 7.5.22 (needs >= 7.5.18) #1 medium @sigstore/core 2.0.0 -> 3.2.1 (needs >= 3.2.1) Node: semantic-release 25 requires ^22.14.0 || >= 24.10.0. release.yml installs `lts/*`, currently Node 24.19.0 — satisfied, and every future LTS line stays above the floor. Verified: `yarn install --immutable` (what release.yml runs) passes against the regenerated lockfile with the lockfile format unchanged (__metadata version 10), and `yarn npm audit --all --recursive` reports no suggestions. A `semantic-release --dry-run` against this branch loads all four configured plugins, passes verifyConditions for both exec and github (GitHub authentication + push permission), and analyzes commits to "no release" — correct, since `chore` is a non-releasing type in release.config.mjs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the tracking delivery foundation for the Convert Python SDK.
This PR implements the next tracking slice after context-scoped conversion creation:
BMAD scope covered:
What Changed
Context.track_conversion(...)with optionalconversion_dataTrackingConfigfor queue behaviorContext.release_queues(...)for explicit flush/releaseforce_multiple_transactions=Truefor explicit repeat transaction reporting/track/{sdk_key}/track/{account_id}/{project_id}Validation
Ran in
../python-sdk:uv sync --group devuv run pytest -p no:cacheprovideruv buildResults:
37/37tests passedNotes
This PR intentionally stops before Story 2.4.
Not included yet: