Skip to content

Commit d9105a4

Browse files
committed
fix: address multi-backend review (collector exclude, fork env, shutdown, RPC margin)
Skip sw_grpc instrumentation for agent collector channels via thread-local scope; move throttled reporter logs off grpc_channel so GRPC_ENABLE_FORK_SUPPORT is set before import grpc; cancel only background tasks on async shutdown; widen sync RPC deadline vs queue batch window.
1 parent d6d11de commit d9105a4

9 files changed

Lines changed: 460 additions & 137 deletions

File tree

skywalking/agent/__init__.py

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@
3333
from skywalking.profile.snapshot import TracingThreadSnapshot
3434
from skywalking.protocol.language_agent.Meter_pb2 import MeterData
3535
from skywalking.protocol.logging.Logging_pb2 import LogData
36-
from skywalking.utils.grpc_channel import log_dropped_throttled, log_reporter_exception_throttled
36+
from skywalking.utils.reporter_log import log_dropped_throttled, log_reporter_exception_throttled
3737
from skywalking.utils.singleton import Singleton
3838

3939
if TYPE_CHECKING:
@@ -147,13 +147,19 @@ async def _shutdown_async_queue(q: asyncio.Queue, label: str) -> None:
147147
)
148148

149149

150-
async def _cancel_pending_tasks(loop) -> None:
150+
async def _cancel_pending_tasks(tasks) -> None:
151151
"""
152-
Cancel every task on the agent loop except the caller. The current task must be
153-
excluded from the gather too, otherwise it would await itself and never return.
152+
Cancel agent-owned reporter / connectivity-watch tasks only.
153+
154+
Must not cancel the asyncio.run root (__start_event_loop_async): that tears down
155+
the Runner and cancels this fini coroutine before protocol aclose() runs.
156+
The current task (fini) is also excluded so we never await ourselves.
154157
"""
155-
current = asyncio.current_task(loop)
156-
pending = [task for task in asyncio.all_tasks(loop) if task is not current]
158+
current = asyncio.current_task()
159+
pending = [
160+
task for task in tasks
161+
if task is not None and task is not current and not task.done()
162+
]
157163
if not pending:
158164
return
159165
for task in pending:
@@ -814,8 +820,11 @@ async def __start_event_loop_async(self) -> None:
814820

815821
await self.__bootstrap() # gather all coroutines
816822

823+
# Track Tasks explicitly so shutdown cancels only reporters/watchers, not the
824+
# asyncio.run root task that is awaiting this gather.
825+
self.background_tasks = {asyncio.create_task(coro) for coro in self.background_coroutines}
817826
logger.debug('All background coroutines started')
818-
await asyncio.gather(*self.background_coroutines)
827+
await asyncio.gather(*self.background_tasks)
819828

820829
def __start_event_loop(self) -> None:
821830
try:
@@ -885,7 +894,7 @@ async def __fini_async(self):
885894
if config.agent_meter_reporter_active:
886895
await _shutdown_async_queue(self.__meter_queue, 'meter')
887896

888-
await _cancel_pending_tasks(self.loop)
897+
await _cancel_pending_tasks(getattr(self, 'background_tasks', set()) or set())
889898

890899
aclose = getattr(self.__protocol, 'aclose', None)
891900
if callable(aclose):

skywalking/agent/protocol/grpc.py

Lines changed: 41 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
import logging
1919
import traceback
2020
from queue import Queue, Empty
21-
from time import time
21+
from time import monotonic
2222

2323
import grpc
2424

@@ -33,8 +33,8 @@
3333
create_sync_channel,
3434
handle_rpc_error,
3535
is_channel_ready,
36-
log_dropped_throttled,
3736
)
37+
from skywalking.utils.reporter_log import log_dropped_throttled
3838
from skywalking.profile.profile_task import ProfileTask
3939
from skywalking.profile.snapshot import TracingThreadSnapshot
4040
from skywalking.protocol.common.Common_pb2 import KeyStringValuePair
@@ -45,6 +45,25 @@
4545
from skywalking.trace.segment import Segment
4646

4747

48+
def _queue_get_within_batch(queue: Queue, block: bool, batch_deadline: float):
49+
"""
50+
Get one item within an absolute batch window (monotonic deadline).
51+
52+
Avoids int(elapsed) truncation that could let queue waits approach
53+
agent_queue_timeout + 1s and collide with a tight RPC deadline.
54+
Returns None when the window is exhausted or the queue is empty.
55+
"""
56+
remaining = batch_deadline - monotonic()
57+
if remaining <= 0:
58+
return None
59+
try:
60+
if block:
61+
return queue.get(block=True, timeout=remaining)
62+
return queue.get(block=False)
63+
except Empty:
64+
return None
65+
66+
4867
class GrpcProtocol(Protocol):
4968
def __init__(self):
5069
self.properties_sent = False
@@ -154,16 +173,11 @@ def generator():
154173
nonlocal start, sent
155174

156175
while True:
157-
try:
158-
timeout = config.agent_queue_timeout # type: int
159-
if not start: # make sure first time through queue is always checked
160-
start = time()
161-
else:
162-
timeout -= int(time() - start)
163-
if timeout <= 0: # this is to make sure we exit eventually instead of being fed continuously
164-
return
165-
segment = queue.get(block=block, timeout=timeout) # type: Segment
166-
except Empty:
176+
if start is None:
177+
start = monotonic()
178+
batch_deadline = start + float(config.agent_queue_timeout)
179+
segment = _queue_get_within_batch(queue, block, batch_deadline) # type: Segment
180+
if segment is None:
167181
return
168182

169183
queue.task_done()
@@ -230,16 +244,11 @@ def generator():
230244
nonlocal start, sent
231245

232246
while True:
233-
try:
234-
timeout = config.agent_queue_timeout # type: int
235-
if not start: # make sure first time through queue is always checked
236-
start = time()
237-
else:
238-
timeout -= int(time() - start)
239-
if timeout <= 0: # this is to make sure we exit eventually instead of being fed continuously
240-
return
241-
log_data = queue.get(block=block, timeout=timeout) # type: LogData
242-
except Empty:
247+
if start is None:
248+
start = monotonic()
249+
batch_deadline = start + float(config.agent_queue_timeout)
250+
log_data = _queue_get_within_batch(queue, block, batch_deadline) # type: LogData
251+
if log_data is None:
243252
return
244253

245254
queue.task_done()
@@ -268,16 +277,11 @@ def generator():
268277
nonlocal start, sent
269278

270279
while True:
271-
try:
272-
timeout = config.agent_queue_timeout # type: int
273-
if not start: # make sure first time through queue is always checked
274-
start = time()
275-
else:
276-
timeout -= int(time() - start)
277-
if timeout <= 0: # this is to make sure we exit eventually instead of being fed continuously
278-
return
279-
meter_data = queue.get(block=block, timeout=timeout) # type: MeterData
280-
except Empty:
280+
if start is None:
281+
start = monotonic()
282+
batch_deadline = start + float(config.agent_queue_timeout)
283+
meter_data = _queue_get_within_batch(queue, block, batch_deadline) # type: MeterData
284+
if meter_data is None:
281285
return
282286

283287
queue.task_done()
@@ -305,16 +309,11 @@ def generator():
305309
nonlocal start, sent
306310

307311
while True:
308-
try:
309-
timeout = config.agent_queue_timeout # type: int
310-
if not start: # make sure first time through queue is always checked
311-
start = time()
312-
else:
313-
timeout -= int(time() - start)
314-
if timeout <= 0: # this is to make sure we exit eventually instead of being fed continuously
315-
return
316-
snapshot = queue.get(block=block, timeout=timeout) # type: TracingThreadSnapshot
317-
except Empty:
312+
if start is None:
313+
start = monotonic()
314+
batch_deadline = start + float(config.agent_queue_timeout)
315+
snapshot = _queue_get_within_batch(queue, block, batch_deadline) # type: TracingThreadSnapshot
316+
if snapshot is None:
318317
return
319318

320319
queue.task_done()

skywalking/agent/protocol/grpc_aio.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,12 @@
2828
from skywalking.client.grpc_aio import GrpcServiceManagementClientAsync, GrpcTraceSegmentReportServiceAsync, \
2929
GrpcProfileTaskChannelServiceAsync, GrpcLogReportServiceAsync, GrpcMeterReportServiceAsync
3030
from skywalking.loggings import logger, logger_debug_enabled
31+
from skywalking.utils.reporter_log import log_dropped_throttled
3132
from skywalking.utils.grpc_channel import (
3233
apply_connectivity_transition,
3334
create_aio_channel,
3435
handle_rpc_error,
3536
is_channel_ready,
36-
log_dropped_throttled,
3737
)
3838
from skywalking.profile.profile_task import ProfileTask
3939
from skywalking.profile.snapshot import TracingThreadSnapshot

skywalking/plugins/sw_grpc.py

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -236,8 +236,19 @@ def intercept_stream_stream(self, continuation, client_call_details, request_ite
236236
return self._intercept(continuation, client_call_details, request_iterator)
237237

238238
def _sw_grpc_channel_factory(target: str, *args: Any, **kwargs: Any):
239+
from skywalking.utils.grpc_channel import (
240+
is_agent_collector_channel,
241+
is_building_agent_collector_channel,
242+
)
243+
239244
c = _grpc_channel(target, *args, **kwargs)
240-
if target == config.agent_collector_backend_services:
245+
# Prefer explicit agent→OAP marker/scope: multi-address targets are rewritten
246+
# (ipv4:/ipv6:) and no longer equal agent_collector_backend_services.
247+
if (
248+
is_building_agent_collector_channel()
249+
or is_agent_collector_channel(c)
250+
or target == config.agent_collector_backend_services
251+
):
241252
return c
242253
return grpc.intercept_channel(c, _ClientInterceptor(target))
243254

@@ -463,7 +474,14 @@ def __init__(
463474
compression: Optional[grpc.Compression],
464475
interceptors: Optional[Sequence[grpc.aio.ClientInterceptor]],
465476
):
466-
if target != config.agent_collector_backend_services:
477+
from skywalking.utils.grpc_channel import is_building_agent_collector_channel
478+
479+
# Multi-address collector targets are rewritten; do not rely on string equality alone.
480+
skip_sw = (
481+
is_building_agent_collector_channel()
482+
or target == config.agent_collector_backend_services
483+
)
484+
if not skip_sw:
467485
_sw_interceptors: List[grpc.aio.ClientInterceptor] = [
468486
_AioClientUnaryUnaryInterceptor(target),
469487
_AioClientUnaryStreamInterceptor(target),

0 commit comments

Comments
 (0)