Repository navigation
refactor(observability): run every periodic flush on one background loop with one shutdown bound [FND-4648] - #4819
Merged
Merged
Conversation
…oop with one shutdown bound [FND-4648] Logs, metrics, traces and sizing each started their periodic flush their own way (a task on the running loop, an asyncio.run thread, a new_event_loop + run_forever thread, a hand-built loop on a thread), and the Segment client ran a fourth thread with a private loop and a 5 s init wait. Each had its own shutdown, or none. Add observability/background_loop.py: one daemon thread (atlan-observability-loop) with one event loop, started on first use. Every sink starts its flush through AtlanObservability._start_periodic_flush() on that loop, and the Segment sender runs there as a task. One atexit shutdown, bounded by ATLAN_OBSERVABILITY_SHUTDOWN_TIMEOUT_SECONDS (default 10 s), cancels each started task once so it makes its final flush, awaits other tasks on the loop without cancelling them, and stops the loop. A cancel that lands before a task's first step is deferred so the final flush still runs. The console writer stays a plain thread: it does blocking stdout writes that must not stall the flushes. Claude-Session: https://claude.ai/code/session_01AbKqGbSfZqfoW5hkNx3D61
cmgrote
requested review from
Aryamanz29,
OnkarVO7,
atlan-ci and
vaibhavatlan
as code owners
November 9, 2026 21:38
Contributor
📜 Docstring Coverage ReportRESULT: PASSED (minimum: 75.0%, actual: 80.0%) Detailed Coverage Report |
Collaborator
Author
|
/lens |
Contributor
lens summary · updated after round 4 (re-review · only commits since cda08d0)✅ Ready to merge — every finding is resolved Open findings: 🔴 0 critical · 🟠 0 high · 🟡 0 medium · ⚪ 0 low (nit) lens reads this PR as — Observability periodic flushes previously ran on several unrelated asyncio loops and threads, while Segment had its own worker loop, leaving no single lifecycle to coordinate their final drains. The PR aims to make scheduling consistent and place a bound on shutdown flushing. How: It adds a lazy process-wide background event loop, routes each sink's periodic flush and Segment's queue worker through it, and performs a staged, time-bounded shutdown: periodic tasks flush first, the Segment worker then drains, and other tasks on the loop are awaited rather than cancelled. Approach check — worth a second look (advisory, does not block):
✔️ Resolved (8)
🕘 Round history (5)
round 4 · $0.313 of $1.00 · 147 model calls · 0 failed requests · 84% prompt cache hits · gpt-6-luna |
…start flushes after fork [FND-4648] - Stop the Segment worker only after the periodic flushes: the metrics final flush drains its queue through the HTTP client the worker closes. - Schedule a started task under the loop lock, so a racing shutdown's drain always finds it. - After fork, the child replaces the loop's lock, drops each Segment client's worker and restarts every sink's periodic flush on its own loop. - Traces and metrics keep their flush handle and stop it on reset. - Log the shutdown timeout with exc_info (conformance L004); bound the hang test with a pytest timeout instead of a wall-clock assert. - Tests: staged stop, start/shutdown race, Segment drain on an open client, child flush after fork, traces reset; drop redundant asyncio markers. Claude-Session: https://claude.ai/code/session_01AbKqGbSfZqfoW5hkNx3D61
Collaborator
Author
|
/lens |
Contributor
|
🟡 lens · round 2 · re-review · only commits since 5f0628c — not ready: 5 medium/low open (F-68ccaa, F-74ce6d, F-bc1d48…) Open findings: 🔴 0 critical · 🟠 0 high · 🟡 2 medium · ⚪ 3 low (nit)
Approach check —
$0.162 of $1.00 · 71 model calls · 0 failed requests |
Collaborator
Author
|
/lens force verify-budget |
Contributor
☂️ Code Coverage
Overall Coverage
New Files
Modified Files
|
Contributor
|
🟡 lens · round 3 · re-review · full, because requested with force — not ready: 1 medium/low open (F-e6ddbc) Open findings: 🔴 0 critical · 🟠 0 high · 🟡 0 medium · ⚪ 1 low (nit) Approach check —
$0.313 of $1.00 · 146 model calls · 0 failed requests |
Collaborator
Author
|
/lens recheck F-e6ddbc Fixed in cda08d0: tests/unit/observability/test_traces_adaptor.py:244 is now |
Contributor
|
🟡 lens · re-check after round 3 · F-e6ddbc by @cmgrote — not ready: 1 medium/low open (F-e6ddbc) Open findings: 🔴 0 critical · 🟠 0 high · 🟡 0 medium · ⚪ 1 low (nit) Approach check —
$0.313 of $1.00 · 147 model calls · 0 failed requests
|
…ptor tests [FND-4648] asyncio_mode is auto, so pytest-asyncio collects these without the marker. Claude-Session: https://claude.ai/code/session_01AbKqGbSfZqfoW5hkNx3D61
Collaborator
Author
|
/lens |
Contributor
|
✅ lens · round 4 · re-review · only commits since cda08d0 — ready to merge — every finding resolved · $0.31 Open findings: 🔴 0 critical · 🟠 0 high · 🟡 0 medium · ⚪ 0 low (nit) ℹ️ Nothing was re-checked this round — none of the PR's own files changed since the last review (a merge from the base branch alone changes none), so A1 were not re-judged. They show as open because nothing checked them. Comment Approach check —
$0.313 of $1.00 · 147 model calls · 0 failed requests |
atlan-ci
approved these changes
Nov 9, 2026
atlan-ci
left a comment
atlan-ci
left a comment
Collaborator
There was a problem hiding this comment.
lens: ready to merge — every finding at every level is resolved (round 4).
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
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.
Changelog
application_sdk/observability/background_loop.py: one daemon thread (atlan-observability-loop) running one event loop, started on first use, fork-aware.AtlanObservability._start_periodic_flush(), always on that loop. This replaces three start styles: a task on whatever loop was running, anasyncio.runthread, and anew_event_loop+run_foreverthread (plus sizing's hand-built loop on a thread).SegmentClientruns its queue worker on the shared loop instead of its own thread and loop. Its queue and HTTP client are built at start, so the up-to-5 s init wait and the per-send 1 s init wait are gone. When the metrics flush, which now runs on the same loop, callsflush(), it awaits_flush_queuedirectly instead of bridging to itself.atexitshutdown, bounded by the newATLAN_OBSERVABILITY_SHUTDOWN_TIMEOUT_SECONDS(default 10 s), stops everything: each started task is cancelled once and makes its final flush or sends its last batch. Other tasks on the loop (batch flushesadd_recordstarted there) are awaited, not cancelled, because cancelling one deletes its half-written file. Then the loop stops.Behaviour changes for reviewers
atexit(segment_client.close). The loop shutdown sends the Segment worker's last batch. The old close waited up to 10 s for the Segment thread; the new single bound is also 10 s, so worst-case exit time does not grow.tryand its final flush never runs. Found while writing the tests; both race tests went red before the fix.write()to stdout and on the shared loop a stalled pipe would stall every flush.dapr_log_forwarderis a separate process._sync_flush's per-error temporary loop is not a periodic flush. The OTLP and consoleatexitdrains keep their own bounds, so the shutdown-budget work can unify them later.AtlanLoggerAdapter._spawn_flush_thread/_start_asyncio_flush,AtlanMetricsAdapter._spawn_flush_thread/_start_asyncio_flush,AtlanTracesAdapter._start_asyncio_flush,SizingObservabilitySink._flush_thread/_flush_forever,SegmentClient._start_worker_thread*/_worker_thread/_initialized_event/_worker_task. All are underscore-private.Additional context
tests/unit/observability/test_background_loop.py: one thread for all tasks, cancel-once final flush, await-don't-cancel, bounded shutdown, nothing starts after shutdown, cancel racing the start, and two realAtlanObservabilitysinks whose final flushes both land on one shutdown. Mutation check: making the drain cancel every task turnstest_awaits_other_tasks_instead_of_cancelling_themred.Checklist
Copyleft License Compliance
🤖 Generated with Claude Code
https://claude.ai/code/session_01AbKqGbSfZqfoW5hkNx3D61