Conversation
…etadata out of client
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to improve concurrency by removing per-request mutable state from the REST client and moving request-scoped counters/remote metadata out of the client, while making metadata counters safe for concurrent updates.
Changes:
- Convert
collector.Metadatacounters toatomic.Uint64and update call sites to useAdd/Store/Load. - Make the tools REST client stateless (no cached request/buffer/remote/token/metadata) and thread-safe by passing request metadata explicitly.
- Propagate
Remoteand request metadata through collectors/plugins (including addingPlugin.SetRemote) so plugins can observe updated cluster info and report per-run request metrics.
Reviewed changes
Copilot reviewed 64 out of 64 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/collector/metadata.go | Switch metadata counters to atomic.Uint64 and update Reset() accordingly. |
| pkg/api/ontapi/zapi/client.go | Use atomic increments for ZAPI client metadata counters. |
| integration/test/counter_test.go | Update REST client init to return Remote and pass version explicitly when walking templates. |
| cmd/tools/util.go | Adjust to new REST client Init return value and updated REST fetch signatures. |
| cmd/tools/rest/rest.go | Thread request metadata through fetch helpers and REST client calls. |
| cmd/tools/rest/client.go | Remove client state (request/buffer/remote/token/metadata) and accept optional request metadata per call. |
| cmd/tools/rest/client_test.go | Add concurrency test for GetPlainRest metadata counting. |
| cmd/poller/poller.go | Update REST calls to pass nil metadata with new REST client signatures. |
| cmd/poller/plugin/plugin.go | Add SetRemote to plugin interface; cache Remote + per-run RequestMetadata in AbstractPlugin. |
| cmd/poller/plugin/maxplugin/max.go | Use atomic add for PluginInstances. |
| cmd/poller/plugin/changelog/changelog.go | Use atomic store for PluginInstances. |
| cmd/poller/plugin/aggregator/aggregator.go | Use atomic add for PluginInstances. |
| cmd/poller/collector/collector.go | Call SetRemote before each plugin run; load atomic metadata counters via Load(). |
| cmd/collectors/zapiperf/zapiperf.go | Read ZAPI client metadata using Load(). |
| cmd/collectors/zapi/plugins/snapshotviolation/snapshotviolation.go | Store plugin instance count atomically. |
| cmd/collectors/zapi/plugins/qtree/qtree.go | Store plugin instance count atomically. |
| cmd/collectors/zapi/collector/zapi.go | Read ZAPI client metadata using Load(). |
| cmd/collectors/storagegrid/storagegrid.go | Read StorageGrid client metadata using Load(). |
| cmd/collectors/storagegrid/rest/client.go | Use atomic increments for StorageGrid REST client metadata counters. |
| cmd/collectors/statperf/statperf.go | Replace client-scoped metadata with collector RequestMetadata passed into REST fetches. |
| cmd/collectors/statperf/plugins/flexcache/flexcache.go | Use plugin RequestMetadata instead of client metadata; update Init call. |
| cmd/collectors/statperf/parser_test.go | Minor struct literal formatting adjustment. |
| cmd/collectors/restperf/restperf.go | Replace client-scoped metadata with collector RequestMetadata and thread through fetches. |
| cmd/collectors/restperf/plugins/volumetopmetrics/volumetopmetrics.go | Switch to plugin RequestMetadata and atomic plugin instance storage. |
| cmd/collectors/restperf/plugins/volumetag/volumetag.go | Switch to plugin RequestMetadata and updated Init signature usage. |
| cmd/collectors/restperf/plugins/volume/volume.go | Update test client construction and thread request metadata into FetchAll. |
| cmd/collectors/restperf/plugins/nic/nic.go | Switch to plugin RequestMetadata and updated Init usage. |
| cmd/collectors/restperf/plugins/nic/nic_test.go | Update test client construction (no embedded Metadata). |
| cmd/collectors/restperf/plugins/fcvi/fcvi.go | Switch to plugin RequestMetadata and updated Init usage. |
| cmd/collectors/restperf/plugins/disk/disk.go | Switch to plugin RequestMetadata and thread into all REST calls. |
| cmd/collectors/rest/rest.go | Add collector RequestMetadata; update init/cluster-info refresh and stream fetch calls. |
| cmd/collectors/rest/rest_test.go | Adjust helper construction to use pointer receiver consistently. |
| cmd/collectors/rest/plugins/vscanpool/vscanpool.go | Switch to plugin RequestMetadata and updated Init usage. |
| cmd/collectors/rest/plugins/vscanpool/vscanpool_test.go | Update test client construction (no embedded Metadata). |
| cmd/collectors/rest/plugins/volumeanalytics/volumeanalytics.go | Cache remote via AbstractPlugin.Remote; thread request metadata into analytics fetches. |
| cmd/collectors/rest/plugins/volume/volume.go | Cache remote via AbstractPlugin.Remote; switch to plugin RequestMetadata. |
| cmd/collectors/rest/plugins/tag/tag.go | Cache remote via AbstractPlugin.Remote from client.Init. |
| cmd/collectors/rest/plugins/svm/svm.go | Switch to plugin RequestMetadata and updated Init usage. |
| cmd/collectors/rest/plugins/snapshotviolation/snapshotviolation.go | Switch to plugin RequestMetadata and thread metadata through stream fetch. |
| cmd/collectors/rest/plugins/snapmirror/snapmirror.go | Switch to plugin RequestMetadata and thread into REST calls. |
| cmd/collectors/rest/plugins/securityaccount/securityaccount.go | Switch to plugin RequestMetadata and updated Init usage. |
| cmd/collectors/rest/plugins/ontaps3service/ontaps3service.go | Switch to plugin RequestMetadata and updated Init usage. |
| cmd/collectors/rest/plugins/mav/mav.go | Switch to plugin RequestMetadata and updated Init usage. |
| cmd/collectors/rest/plugins/health/health.go | Cache remote via AbstractPlugin.Remote; switch to plugin RequestMetadata + atomic instance storage. |
| cmd/collectors/rest/plugins/certificate/certificate.go | Switch to plugin RequestMetadata and updated Init usage. |
| cmd/collectors/rest/plugins/auditlog/auditlog.go | Switch to plugin RequestMetadata and thread into REST calls. |
| cmd/collectors/rest/plugins/aggregate/aggregate.go | Switch to plugin RequestMetadata and updated Init usage. |
| cmd/collectors/power.go | Pass cluster name explicitly (no client remote getter) and update REST fetch signature usage. |
| cmd/collectors/keyperf/keyperf.go | Switch to collector RequestMetadata and thread into stream fetch. |
| cmd/collectors/eseriesperf/eseriesperf.go | Read E-series client metadata using Load(). |
| cmd/collectors/eseries/rest/client.go | Use atomic increments for E-series REST client metadata counters. |
| cmd/collectors/eseries/plugins/ssdcachecapacity/ssdcachecapacity.go | Store plugin instance count atomically. |
| cmd/collectors/eseries/plugins/hardware/hardware.go | Store plugin instance count atomically. |
| cmd/collectors/eseries/eseries.go | Read E-series client metadata using Load(). |
| cmd/collectors/ems/ems.go | Use collector Remote instead of REST client Remote() in global labels. |
| cmd/collectors/discovery.go | Update tools REST Init usage to return Remote (others remain error-only). |
| cmd/collectors/commonutils.go | Update REST fetch signature usage (FetchAll, FetchSome) with metadata arg. |
| cmd/collectors/cmperf/cmperf.go | Switch to collector RequestMetadata and read via Load(). |
| cmd/collectors/cisco/plugins/version/version.go | Store Cisco plugin metadata atomically. |
| cmd/collectors/cisco/plugins/optic/optic.go | Store Cisco plugin metadata atomically. |
| cmd/collectors/cisco/plugins/networkinterface/networkinterface.go | Store Cisco plugin metadata atomically. |
| cmd/collectors/cisco/plugins/lldp/lldp.go | Store Cisco plugin metadata atomically. |
| cmd/collectors/cisco/plugins/environment/environment.go | Store Cisco plugin metadata atomically. |
| cmd/collectors/cisco/plugins/cdp/cdp.go | Store Cisco plugin metadata atomically. |
Comments suppressed due to low confidence (1)
cmd/tools/rest/client.go:263
invokeWithAuthRetrycan calldoInvoke()more than once (on auth retry), but the request body is never rewound between calls, anddefer buffer.Reset()clears the buffer after the first attempt. This can cause retries (especially POSTs) to send an empty/consumed body. Before eachc.client.Do(req)call, resetreq.Bodyfromreq.GetBody()(or rebuild the request), and don’t reset/clear the body source until all retry attempts are finished.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…etadata out of client
rahulguptajss
approved these changes
May 14, 2026
Hardikl
approved these changes
May 14, 2026
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.
…etadata out of client