feat: parallel OID walks with per-module walk_concurrency config

[amuntean-godaddy]

Summary

Sequential walking of N OID subtrees in a module takes sum(N) seconds. On devices with large interface tables (e.g. a 48-port switch with hundreds of sub-interfaces, 19-OID if-mib at 45ms RTT), the walk can exceed the configured scrape timeout.

This PR adds parallel OID walks within a single module, controlled by a new walk_concurrency field in the module config. The default is 1 — identical to the current sequential behaviour — so existing configs are completely unaffected.

Changes

New config field: walk_concurrency

modules:
  if_mib:
    walk_concurrency: 4   # walk 4 OID subtrees in parallel; default 1 (sequential)
    max_repetitions: 10
    timeout: 5s
    walk:
      - ifAdminStatus
      - ifOperStatus
      # ...

Propagates through generator.yml → snmp.yml like max_repetitions and timeout.

Implementation

Fixed worker pool (collector/collector.go): ScrapeTarget now accepts ctx context.Context as its first parameter. When walk_concurrency > 1, a fixed pool of min(walk_concurrency, len(walk)) workers is created, each with its own cloned SNMP connection. Workers pull OID subtrees from a pre-filled buffered channel. errgroup.WithContext(ctx) cancels all workers on the first error and g.Wait() guarantees ScrapeTarget never returns with live goroutines.

Clone() on SNMPScraper (scraper/): Each worker gets its own connection via Clone(). The implementation does a field-by-field copy of GoSNMP and explicitly:

• Zeroes callback fields (OnSent, OnRecv, OnRetry, OnFinish, PreSend) — they capture per-scrape counters by reference
• Strips the port from LocalAddr via hostOnly() — prevents bind: address already in use with --snmp.source-address=host:port
• Deep-copies SecurityParameters via .Copy() — avoids shared AES/DES IV state across connections
• Zeroes Conn — connections are not shared

Metrics: packets/retries counters in collect() use sync/atomic types for correctness. Walk-phase packets are not counted when walk_concurrency > 1 (clone callbacks are intentionally nil); the metric help strings document this.

Testing

go test ./... -race

All packages pass. New tests:

• TestScrapeTargetParallelWalk — walk_concurrency=3, 3 subtrees × 2 PDUs, verifies all 6 PDUs present
• TestScrapeTargetParallelWalkConnectError — clone connect failure propagates cleanly
• TestHostOnly, TestCloneStripsLocalAddrPort, TestMockCloneCopiesConnectError — Clone() correctness

@SuperQ @bastischubert @RichiH

[lukeod]

Not a maintainer, but took a look at the PR as it's a feature i've considered doing a PoC for but never got around to it.

I think there's some significant issues with the PR as it stands:

• Nothing in the test suite ever sets walk_concurrency > 1, so the new code path is never executed by any test. So the go test ./... -race result doesn't mean much.
• testMetrics() was added but has no callers. The mock's Clone() also doesn't copy ConnectError, so the clone-connect-failure branch can't be tested as-is.
• There's a race: ScrapeTarget needs to wait for all its walk goroutines before returning, but on the first walk error it returns immediately (wg.Wait() only happens in the channel-closer goroutine) - the leftover goroutines then Clone() the shared client while the worker reconfigures it via SetOptions for the next module.
• Clone() copies LocalAddr verbatim, so with a fixed source port (e.g. --snmp.source-address=10.0.0.5:16100) every clone's Connect() fails with bind: address already in use
• Clones drop OnSent/OnRecv/OnRetry, which are the only source of snmp_packets_total, snmp_retries_total, snmp_duration_seconds and snmp_scrape_packets_sent/_retried. With concurrency on, walk traffic (i.e. nearly all of it) goes uncounted.
• Connections are made per-subtree, not per-worker: N walk entries = N connect/close cycles per scrape regardless of the concurrency setting, and nothing cancels in-flight clones after an early error.

Suggestion: a fixed pool of concurrency workers, each doing Clone()+Connect() once and pulling subtrees off a channel (same pattern as the existing worker pool in Collect()), wrapped in errgroup.WithContext so the first error cancels the siblings and the call waits for them before returning. That bounds connections to concurrency and fixes the race and orphan-walk issues in one go; golang.org/x/sync is already an indirect dep.

[amuntean-godaddy]

Thank you @lukeod @SuperQ
I am going to re-work on the issues you have pointed out and run the real test in my environment.
I am not very proficient in go, however I really need the concurrency support and using AI here :) .

[amuntean-godaddy]

@SuperQ could you please review the current changes ?

[SuperQ]

Looks like some CI issues.

[amuntean-godaddy]

Hi @SuperQ @bastischubert @RichiH — addressed all the lint issues raised by CI:

• gci: fixed import grouping in collector/collector.go, collector/collector_test.go, and scraper/gosnmp.go
• errorlint: replaced err != wantErr with errors.Is(err, wantErr) in scraper/gosnmp_test.go
• staticcheck: removed the deprecated NonRepeaters field from Clone() in scraper/gosnmp.go

The Build generator failure is an infrastructure flake (503 fetching Dell MIBs from an external URL — unrelated to this PR).

The branch is ready for re-review. Thanks for your time!

[SuperQ]

@lukeod Want to give this another pass?