SDK | Structured logging and progress bars#306
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| **kwargs, | ||
| ) | ||
| with LogContext(http_status=response.status_code): | ||
| return response |
There was a problem hiding this comment.
HTTP context dropped before errors
Medium Severity
GatewayClient._request wraps the call in LogContext(url=…) and nests LogContext(http_status=…) only around return response. Callers invoke success_or_raise after _request returns, so failed HTTP responses log under LogCategory.NETWORK without url or http_status in JSONL.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d459397. Configure here.
Co-authored-by: Lucas Parzianello <lucaspar@users.noreply.github.com>
Co-authored-by: Lucas Parzianello <lucaspar@users.noreply.github.com>
Co-authored-by: Lucas Parzianello <lucaspar@users.noreply.github.com>
Co-authored-by: Lucas Parzianello <lucaspar@users.noreply.github.com>
lucaspar
changed the title
|
bugbot review |
Byte-level progress bars and download avg_speed_bps only counted streamed chunks, but totals included files whose contents were skipped or only metadata was transferred. Credit remaining file bytes on successful completion so progress bars and byte counters reach expected totals. Co-authored-by: Lucas Parzianello <lucaspar@users.noreply.github.com>
Multiple asyncio.to_thread workers call _throttled_update on the same shared progress bar. tqdm.update() increments its internal counter without holding its display lock, so concurrent calls can lose bytes or corrupt the display. Add a threading.Lock guarding all mutations of _prog_uploaded_bytes, including throttled byte updates, description changes, clear/close, and total adjustments. Add a regression test for concurrent uploads. Co-authored-by: Lucas Parzianello <lucaspar@users.noreply.github.com>
| bytes_streamed=_file_streamed[0], | ||
| prog_bar=prog_bar, | ||
| bytes_accounted=bytes_downloaded_shared, | ||
| ) |
There was a problem hiding this comment.
Failed download inflates byte metrics
Medium Severity
Progress bars and byte counts for both uploads and downloads can be inaccurate. When a file transfer fails after partial streaming, the bytes already transferred are still counted towards the total, leading to inflated bytes_downloaded/uploaded and avg_speed_bps in completion summaries. This also causes the progress bar to show more progress than actual successful transfers or to stall inconsistently.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9d81aaa. Configure here.
| try: | ||
| with _current_log_path.open("a", encoding="utf-8") as f: | ||
| f.write(json.dumps(entry, default=str) + "\n") | ||
| except OSError: |
There was a problem hiding this comment.
Unlocked concurrent JSONL appends
Low Severity
The structured log sink appends JSON lines with a plain file open/write/close on each message. Concurrent upload workers and the asyncio progress logger can emit log records at the same time, which can interleave writes and produce corrupted or partial JSONL lines under load.
Reviewed by Cursor Bugbot for commit 9d81aaa. Configure here.
- Replace magic-number assertions in credit_unstreamed_file_bytes tests - Type progress_callback as Callable in concurrent upload test - Annotate prog_bar parameter with tqdm[NoReturn] - Apply ruff formatting fixes Co-authored-by: Lucas Parzianello <lucaspar@users.noreply.github.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
There are 5 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1fca4e7. Configure here.
| overwrite=overwrite, | ||
| verbose=verbose, | ||
| prefix=prefix, | ||
| ) |
There was a problem hiding this comment.
Path downloads skip byte progress
Medium Severity
Byte-level download progress runs only when files_to_download is a list. The usual download(from_sds_path=...) path uses a Paginator from list_files, so it always takes _download_files_fallback with file-count tqdm and no byte totals or avg_speed_bps in the completion summary.
Reviewed by Cursor Bugbot for commit 1fca4e7. Configure here.
| f"Downloaded: {file_info.name}", | ||
| file_name=file_info.name, | ||
| file_size=file_info.size, | ||
| ) |
There was a problem hiding this comment.
Skipped downloads log as downloaded
Low Severity
New per-file structured logs emit "Downloaded: {name}" whenever download_single_file returns a successful Result, including skip_contents=True, local-file-exists skips, and BLAKE3-identical skips where no contents were transferred.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1fca4e7. Configure here.
3 participants
Structured logging, test coverage, and progress bars
ts,pid,lvl,cat,msg),LogContext/set_persistent_log_context, boot message (SDK version, OS, Python), category binding, XDG-compliant pathsuccess_or_raiseedge cases (no status code, JSON decode/HTML fallbacks, 3xx), structured logging lifecycledownload_datasetfilter branches,upload_captureerror handling,upload_multichannel_drf_capturewith empty channels,download_fileaccepts File instances +skip_contents, temp file cleanup on failure,extract_error_details_from_html, pagination fixes__init__.pyNote
Medium Risk
Touches all transfer and logging paths (Client init, gateway I/O, concurrent uploads); behavior changes include quieter default stderr and new on-disk logs, but core API remains backward compatible aside from optional
log_fileand progress callbacks.Overview
Adds structured JSONL logging for the SDK: daily files under XDG state (or
log_file/Client(log_file=…)), categories vialog.bind(cat=…), persistent and scoped context, and session summaries for uploads/downloads (counts, bytes, avg speed).Clientturns this on at construction; default Loguru stderr is removed so sinks are explicit (enable_loggingvs structured file).Upload/download progress moves from file-count bars to byte totals with throttled chunk callbacks through
gateway(_ProgressFileReader),sds_files,client, andUploadWorkload(thread-safe bar updates, periodic upload progress logs,credit_unstreamed_file_bytesfor skips/metadata-only).New
progress_log_period_secsconfig,demo-progress-barsscript +justtarget, mkdocs for structured logging, and broad test coverage. Gateway dev-notes/changelog are whitespace-only.Reviewed by Cursor Bugbot for commit 1fca4e7. Bugbot is set up for automated code reviews on this repo. Configure here.