Skip to content

Commit e78d0cf

Browse files
authored
chore(test): upgrade the mock collector and drop the seed workaround (#410)
The pinned mock-collector image dated from October 2022, 13 commits behind the tool's master. Move it to c6b91a0e, which carries the fix for the first-insert race in SegmentItems/LogItems (apache/skywalking-agent-test-tool#65): when two processes reported their first segment for the same service name concurrently, the collector silently dropped one while both reporters got HTTP 200. sw_fork_support worked around that race by seeding the service name with a parent-only /ping request and waiting for the collector to register it before triggering the concurrent parent/child reports. The collector no longer needs the help, so the endpoint, the seed step and the extra expected segment are gone and the test is back to asserting exactly the cross-fork trace it is about. The upgrade also picks up the validator changes made since 2022, one of which affected us. LogAssert now sorts both the expected and the actual logs by their body text before comparing them pairwise (apache/skywalking-agent-test-tool#59), and the sort uses the raw expected string, so a matcher such as `text: not null` participates as the literal "not null". sw_loguru matches its two logging-module records that way while the records themselves led with the default layout's timestamp, so they sorted first and their placeholders last, inverting the pairing. Pin a layout that leads with the logger name, which sorts after the placeholders and orders the two records deterministically, and reorder the expected entries to match; SWFormatter is still exercised. Its expected.data.yml is the only one in the tree with a non-empty logItems block, so no other test is affected. Also refresh CLAUDE.md: supported Python and grpcio floor, the current plugin list, the agent's fork/prefork lifecycle, and the plugin-test validation notes.
1 parent 8020a1c commit e78d0cf

7 files changed

Lines changed: 67 additions & 61 deletions

File tree

CLAUDE.md

Lines changed: 46 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,10 @@ tools/
3838

3939
## Python Version Support
4040

41-
- **Current (master):** Python 3.8 - 3.11 (tested in CI), declared >=3.8 <=3.13
42-
- **In-progress (PR #374):** Dropping 3.8, adding 3.12 + 3.13 to CI matrix
43-
- **Upcoming:** Python 3.14 support needed
41+
- Declared `>=3.10, <3.15`; CI runs the plugin/unit matrix on Python 3.10 - 3.14.
42+
- The agent requires `grpcio >= 1.83`. The generated protobuf stubs refuse an older grpcio
43+
at import (`GRPC_GENERATED_VERSION`), so the codegen `grpcio-tools` in the Makefile is
44+
pinned in lockstep — bumping it raises the runtime floor for every user.
4445

4546
## Build & Development
4647

@@ -72,6 +73,32 @@ Plugin-specific settings (all via `SW_` env vars):
7273

7374
Filter functions: `config.ignore_http_method_check(method)`, `config.ignore_grpc_method_check(method)`
7475

76+
## Agent Lifecycle & Fork Support
77+
78+
`SkyWalkingAgent` (skywalking/agent/__init__.py) has two entry points:
79+
80+
- `start()` — full agent: `config.finalize()` (once per lineage; `finalize_name` is NOT
81+
idempotent), `log.install()` + `plugins.install()`, then `__bootstrap()` creates the queues,
82+
the protocol client (the gRPC channel lives here) and the reporter threads.
83+
- `start_prefork_master()` — instrumentation only: everything above EXCEPT queues, protocol
84+
and threads. Used for the Gunicorn master.
85+
86+
**Never create a gRPC channel before `fork()`.** With grpcio >= 1.80 a channel that survives
87+
`fork()` produces `Kick Failure` stderr spam and can silently deadlock the child inside gRPC's
88+
own at-fork handlers (grpc/grpc#43055, apache/skywalking#13958). So:
89+
90+
- Gunicorn (`sw-python run -p gunicorn`): master instruments only, each forked worker runs the
91+
full agent via the `os.register_at_fork(after_in_child=...)` hook. The master is not a
92+
service instance. uWSGI has always worked this way, via its `@postfork` hook.
93+
- `SW_AGENT_ASYNCIO_ENHANCEMENT` has no fork support at all and is rejected under `-p`.
94+
- Explicit `os.fork()` over gRPC is unreliable regardless (reporters enter gRPC independently
95+
of requests) — document HTTP/Kafka for forking apps.
96+
97+
`agent.started()` reports whether reporting is live in this process. It is False in a prefork
98+
master, where `is_segment_queue_full()` returns True so span creation short-circuits to
99+
`NoopSpan` and `archive_*` drop — otherwise instrumented code running at `--preload` import
100+
time would hit uninitialized queues.
101+
75102
## Context & Carrier API Details
76103

77104
### get_context() Signatures
@@ -281,6 +308,8 @@ class TestPlugin(TestPluginBase):
281308
- Services install the plugin lib via `pip install -r /app/requirements.txt`
282309
- Use `sw-python run python3 /app/services/provider.py` to start with agent
283310
- External services (Redis, Kafka, etc.) added as needed with healthchecks
311+
- The mock collector is pinned by image SHA in `docker-compose.base.yml`. Bumping it also
312+
pulls in validator changes — verify the whole plugin matrix on CI, not just one test.
284313

285314
### Expected Data Format (expected.data.yml)
286315

@@ -306,7 +335,17 @@ segmentItems:
306335
skipAnalysis: false
307336
```
308337
309-
Validation operators: `not null`, `gt 0`, exact string match.
338+
Validation operators: `not null`, `gt 0`, `start with`, `end with`, exact string match.
339+
340+
Validation notes:
341+
- Segments and spans are matched by content, and the expected `segmentSize` / span count must
342+
match exactly; only services named in the expected file are checked.
343+
- `logItems` entries are ORDER-SIGNIFICANT: the collector sorts expected and actual logs by
344+
their raw body text before comparing pairwise, so a `text: not null` placeholder sorts as
345+
the literal string `"not null"`. See `sw_loguru`, which pins a log layout to keep the
346+
ordering deterministic — it is the only test asserting `logItems`.
347+
- `prepare()` fixtures should call `.raise_for_status()`; otherwise an HTTP error passes the
348+
fixture silently and surfaces later as a confusing empty-data diff.
310349

311350
### Running Tests
312351

@@ -334,10 +373,10 @@ poetry run pytest -v $(bash tests/gather_test_paths.sh)
334373
7. Run `make doc-gen` to regenerate Plugins.md
335374
8. Verify with `make lint`
336375

337-
## All 35 Plugins
376+
## All 38 Plugins
338377

339-
Web: sw_flask, sw_django, sw_fastapi, sw_sanic, sw_tornado, sw_bottle, sw_pyramid, sw_falcon
340-
HTTP: sw_requests, sw_urllib3, sw_urllib_request, sw_aiohttp, sw_httpx, sw_http_server
378+
Web: sw_flask, sw_django, sw_fastapi, sw_sanic, sw_sanic_v2, sw_tornado, sw_bottle, sw_pyramid, sw_falcon, sw_falcon_v3
379+
HTTP: sw_requests, sw_urllib3, sw_urllib3_v2, sw_urllib_request, sw_aiohttp, sw_httpx, sw_http_server
341380
Database: sw_pymysql, sw_mysqlclient, sw_psycopg, sw_psycopg2, sw_pymongo, sw_elasticsearch, sw_happybase, sw_neo4j, sw_asyncpg
342381
Cache: sw_redis, sw_aioredis
343382
MQ: sw_kafka, sw_rabbitmq, sw_celery, sw_pulsar, sw_confluent_kafka, sw_aiormq, sw_amqp

tests/plugin/data/sw_loguru/docker-compose.yml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,12 @@ services:
4444
SW_AGENT_NAME: provider
4545
SW_AGENT_LOGGING_LEVEL: DEBUG
4646
SW_AGENT_LOG_REPORTER_LEVEL: INFO
47+
# The collector sorts expected and actual logs by their body text before comparing
48+
# them pairwise, so the reported text must order deterministically. The default
49+
# layout leads with the timestamp, which sorts the `logging` records ahead of the
50+
# loguru ones and ahead of the `not null` placeholders they are matched by; leading
51+
# with the logger name keeps them last and stable. See expected.data.yml.
52+
SW_AGENT_LOG_REPORTER_LAYOUT: '%(name)s [%(threadName)s] %(levelname)s - %(message)s'
4753

4854
consumer:
4955
extends:

tests/plugin/data/sw_loguru/expected.data.yml

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,11 @@
1515
# limitations under the License.
1616
#
1717

18+
# The collector sorts expected and actual logs by their body text and then compares them
19+
# pairwise, so the order here is significant. The two loguru records sort by their literal
20+
# text; the two `logging` records are matched by `text: not null`, which sorts after them,
21+
# and among themselves they follow the order the collector sees - CRITICAL before ERROR,
22+
# given the log layout pinned in docker-compose.yml.
1823
logItems:
1924
- serviceName: provider
2025
logSize: 4
@@ -46,17 +51,17 @@ logItems:
4651
body:
4752
type: 'text'
4853
content:
49-
text: not null
54+
text: 'Loguru provider warning reported.'
5055
traceContext:
5156
traceId: not null
5257
traceSegmentId: not null
5358
spanId: 0
5459
tags:
5560
data:
5661
- key: level
57-
value: ERROR
62+
value: WARNING
5863
- key: logger
59-
value: not null
64+
value: loguru
6065
- key: thread
6166
value: not null
6267
layer: ''
@@ -66,17 +71,17 @@ logItems:
6671
body:
6772
type: 'text'
6873
content:
69-
text: 'Loguru provider warning reported.'
74+
text: not null
7075
traceContext:
7176
traceId: not null
7277
traceSegmentId: not null
7378
spanId: 0
7479
tags:
7580
data:
7681
- key: level
77-
value: WARNING
82+
value: CRITICAL
7883
- key: logger
79-
value: loguru
84+
value: not null
8085
- key: thread
8186
value: not null
8287
layer: ''
@@ -94,7 +99,7 @@ logItems:
9499
tags:
95100
data:
96101
- key: level
97-
value: CRITICAL
102+
value: ERROR
98103
- key: logger
99104
value: not null
100105
- key: thread

tests/plugin/docker-compose.base.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ version: '2.1'
2020

2121
services:
2222
collector:
23-
image: ghcr.io/apache/skywalking-agent-test-tool/mock-collector:7f20775e0631356c4823d9372b09d653db0e6540
23+
image: ghcr.io/apache/skywalking-agent-test-tool/mock-collector:c6b91a0eaef16d427268e9806d5df65949e2a9bf
2424
ports:
2525
- 19876:19876
2626
- 12800:12800

tests/plugin/web/sw_fork_support/expected.data.yml

Lines changed: 1 addition & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -17,27 +17,8 @@
1717

1818
segmentItems:
1919
- serviceName: provider
20-
segmentSize: 3
20+
segmentSize: 2
2121
segments:
22-
- segmentId: not null
23-
spans:
24-
- operationName: /ping
25-
parentSpanId: -1
26-
spanId: 0
27-
spanLayer: Http
28-
tags:
29-
- key: http.method
30-
value: GET
31-
- key: http.url
32-
value: http://0.0.0.0:9090/ping
33-
- key: http.status_code
34-
value: '200'
35-
startTime: gt 0
36-
endTime: gt 0
37-
componentId: 7001
38-
spanType: Entry
39-
peer: not null
40-
skipAnalysis: false
4122
- segmentId: not null
4223
spans:
4324
- operationName: /users

tests/plugin/web/sw_fork_support/services/app.py

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -50,14 +50,6 @@ def users():
5050

5151
frontend = Flask('frontend')
5252

53-
# readiness/warm-up endpoint: its segment arrives alone and seeds the service name
54-
# in the mock collector, whose first-insert for a service is not concurrency-safe
55-
# (SegmentItems.addSegmentItem check-then-act) — the parent and child later report
56-
# their /users segments concurrently under the same service name
57-
@frontend.route('/ping', methods=['GET'])
58-
def ping():
59-
return jsonify({'song': 'Despacito'})
60-
6153
@frontend.route('/users', methods=['GET'])
6254
def call_backend():
6355
res = requests.get('http://127.0.0.1:9091/users', timeout=5)

tests/plugin/web/sw_fork_support/test_fork_support.py

Lines changed: 1 addition & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
# See the License for the specific language governing permissions and
1515
# limitations under the License.
1616
#
17-
import time
1817
from typing import Callable
1918

2019
import pytest
@@ -26,10 +25,7 @@
2625
@pytest.fixture
2726
def prepare():
2827
# type: () -> Callable
29-
# /ping (parent-only) also seeds the service name in the mock collector before the
30-
# parent and child post their /users segments concurrently: the collector's very first
31-
# insert for a service name is not concurrency-safe and can silently drop one segment
32-
return lambda *_: requests.get('http://0.0.0.0:9090/ping', timeout=5).raise_for_status()
28+
return lambda *_: requests.get('http://0.0.0.0:9090/users', timeout=5).raise_for_status()
3329

3430

3531
class TestPlugin(TestPluginBase):
@@ -44,19 +40,6 @@ class TestPlugin(TestPluginBase):
4440

4541
@pytest.mark.parametrize('version', ['grpcio>=1.83'])
4642
def test_plugin(self, docker_compose, version):
47-
# the /ping seed segment must be REGISTERED by the collector before /users makes
48-
# the parent and child report concurrently, otherwise all three segments can be
49-
# in flight together and the collector's first-insert race still drops one
50-
for _ in range(30):
51-
if '/ping' in requests.get('http://localhost:12800/receiveData', timeout=5).text:
52-
break
53-
time.sleep(1)
54-
else:
55-
raise Exception('the /ping seed segment never reached the collector')
56-
57-
response = requests.get('http://0.0.0.0:9090/users', timeout=5)
58-
assert response.status_code == 200
59-
6043
self.validate()
6144

6245
stdout, stderr = docker_compose.get_logs()

0 commit comments

Comments
 (0)