feat: opt-in tracing for periodic log uploads - #3373
Conversation
This commit adds optional tracing instrumentation to the SaveLogsPeriodicallySidecar to help diagnose issues where logs stop updating while the sidecar is still running. Changes: - Import traced context manager from metaflow.tracing - Wrap periodic upload invocations in traced() spans - Capture key observability attributes: elapsed_seconds, total_bytes, files_changed, returncode, success status, and exception type (if any) - Add unit tests verifying tracing is opt-in and doesn't interfere with normal operation The tracing is completely opt-in via existing Metaflow tracing configuration (OTEL_ENDPOINT, ZIPKIN_ENDPOINT, or CONSOLE_TRACE_ENABLED). When tracing is disabled (the default), the traced() context manager is a no-op. Fixes Netflix#3307
Greptile SummaryAdds opt-in tracing and upload-result attributes to periodic log uploads, with tests covering disabled and enabled tracing. However, the upload still completes before the empty traced context begins, so the previous finding remains unresolved.
Confidence Score: 4/5The PR is not yet safe to merge because the upload remains outside the traced context, leaving the existing blocking tracing defect unresolved. The previous finding remains outstanding: Files Needing Attention: metaflow/mflog/save_logs_periodically.py
|
| Filename | Overview |
|---|---|
| metaflow/mflog/save_logs_periodically.py | Adds upload tracing attributes, but the traced context still does not enclose the upload operation. |
| test/unit/test_save_logs_periodically.py | Adds tracing tests, though they verify invocation and attributes rather than that the upload executes inside the span. |
Reviews (2): Last reviewed commit: "Add opt-in tracing for periodic log uplo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| with traced("save_logs_periodically.upload", attrs=attrs): | ||
| pass |
There was a problem hiding this comment.
When tracing is enabled, _call_save_logs() finishes before this context manager is entered, and the context body is empty. The resulting span has near-zero duration and cannot provide active trace context for the upload or capture its nested instrumentation and failures. Move the upload into the traced context and attach its outcome attributes to the active span.
Knowledge Base Used: Task logging and sidecars
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
895e11a to
9aad30c
Compare
1 participant
Fixes
Fixes #3307
Summary
Wraps periodic log upload work in Metaflow's existing
traced()helper so uploads emit spans only when tracing is already enabled. Default behavior stays a no-op.Test plan