feat(opensearchtransport): collect per-request metrics by default (#896)

Per-request counters (requests, failures, responses-by-status) are now
collected by default, independent of EnableMetrics. The metrics struct
is always allocated and wired into the connection pools; EnableMetrics
now sets only a `detailed` flag that gates the expensive
detailed-metrics path (per-connection enumeration and the
per-connection/per-policy/per-snapshot callbacks).

Previously these counters were gated behind EnableMetrics alongside the
expensive detailed path, so callers who wanted basic request/response
counts had to opt into work they did not need, and internal consumers
could not rely on the counters being populated.

- The responses-by-status counter moves from a mutex-guarded
  map[int]int to a lock-free atomic.Int64 array indexed by status code,
  plus a single overflow bucket for out-of-range codes. (requests and
  failures were already atomic as of #776.)
- Metrics() returns the per-request counters unconditionally and only
  builds the detailed, callback-augmented fields when detailed is
  enabled. It no longer returns an error when metrics are disabled.
- Detailed-callback registration at all 7 policy sites is guarded by a
  nil-safe metrics.detailedEnabled() helper, so the counters-only path
  does zero per-snapshot work.
- A custom ConnectionPoolFunc returning a single-URL pool no longer
  errors during construction: metrics wiring is a no-op on a pool type
  mismatch, mirroring the multi-node branch.
- The public Metrics.Responses map keys out-of-range status codes under
  -1 (statusOverflow); documented on the field.

Closes #891

Signed-off-by: Ryan Yuan <ryan.yuan@crowdstrike.com>
Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com>
Co-authored-by: Ryan Yuan <ryan.yuan@crowdstrike.com>
Co-authored-by: Sean Chittenden <sean.chittenden@crowdstrike.com>

Author: ryanyuan

CHANGELOG.md

@@ -165,6 +165,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 
 ### Changed
 
+- **BREAKING**: Per-request transport metrics (`requests`, `failures`, responses-by-status) are now always collected via lock-free atomics, independent of `EnableMetrics`. `EnableMetrics` now gates only the detailed-metrics snapshot (per-connection, per-policy, and router state returned by `Metrics()`). The responses-by-status counter moved from a mutex-guarded map to a lock-free atomic array. `Metrics()` no longer returns an error when metrics are disabled -- it always returns the per-request counters (callers that branched on `if err != nil` for the disabled case should drop that check). See [`UPGRADING_V5.md`](UPGRADING_V5.md#metrics-error-on-disabled-removed) for migration. ([#891](https://github.com/opensearch-project/opensearch-go/issues/891))
 - Reorganize the documentation. Split `UPGRADING.md` into a version-history index plus per-major-version guides (`UPGRADING_V5.md` through `UPGRADING_V2.md`) and rename `opensearchapi/MIGRATING.md` to `opensearchapi/UPGRADING_V4_TO_V5.md`. Group the `guides/` and `_samples/` files by subsystem (`transport-`, `indexing-`, `usage-`, `config-`) and add a `guides/README.md` index. Make `guides/usage-error_handling.md` the single source for partial-error handling and `guides/transport-retry_backoff.md` the single source for resurrection-timeout config, replacing the duplicated copies in `opensearchapi/README.md` and `guides/transport-routing.md` with links. Add package documentation (`doc.go`) for `opensearchapi`, `plugins`, `signer`, and `signer/awsv2`.
 - Trim the CI compatibility matrix to the currently-patched OpenSearch set (2.19.x and 3.x) per the 12-month support policy; older lines (1.3.x - 2.18.x) are no longer part of the tested matrix and the 4.x client remains their supported path. No client code change ([#856](https://github.com/opensearch-project/opensearch-go/issues/856))
 - **BREAKING**: Module path is now `github.com/opensearch-project/opensearch-go/v5`. Update import paths from `/v4` to `/v5`; the in-source `opensearchapi.X` package qualifier is unchanged

UPGRADING_V5.md

@@ -96,6 +96,34 @@ if err := client.DiscoverNodes(ctx); err != nil {
 
 The signature already changed earlier in this version to take `context.Context` (see CHANGELOG); this is a behavioral change on top of the signature change.
 
+## Metrics error on disabled removed
+
+The per-request transport counters (`Requests`, `Failures`, and responses-by-status) are now always collected via lock-free atomics, independent of `EnableMetrics`. As a result, `opensearch.Client.Metrics()` (and `opensearchtransport.Transport.Metrics()`) no longer returns the `"transport metrics not enabled"` error when `EnableMetrics` is false -- it always returns the per-request counters. `EnableMetrics` now gates only the detailed-metrics snapshot (per-connection enumeration, per-policy breakdowns, and router cache state); those fields stay zero/nil when it is unset.
+
+Callers that branched on the error to detect the disabled state should drop that check. The returned error is now non-nil only when a detailed-metrics snapshot callback fails.
+
+```go
+// v4: Metrics() errored when EnableMetrics was false, so callers used the
+// error to detect the disabled state.
+m, err := client.Metrics()
+if err != nil {
+    // treated as "metrics disabled" -- no counters available
+    return
+}
+use(m.Requests, m.Failures)
+
+// v5: per-request counters are always populated. A non-nil error now means a
+// detailed-snapshot callback failed, not that metrics are disabled.
+m, err := client.Metrics()
+if err != nil {
+    log.Printf("detailed metrics snapshot failed: %s", err)
+    // m.Requests / m.Failures / m.Responses are still valid here
+}
+use(m.Requests, m.Failures)
+```
+
+Detailed fields such as `Policies` and `Router` remain populated only when `EnableMetrics` is set; reading them without it yields nil, unchanged from v4.
+
 ## `opensearchtransport.Client` renamed to `opensearchtransport.Transport`
 
 The concrete `opensearchtransport.Client` type was renamed to `opensearchtransport.Transport`. The type owns HTTP round-trip concerns -- connection pooling, retries, node selection, and discovery -- so `Transport` reflects its role and avoids colliding conceptually with the API clients above it (`opensearch.Client` and `opensearchapi.Client`).

guides/transport-metrics.md

@@ -2,9 +2,11 @@
 
 The opensearch-go transport exposes a pull-based metrics API that returns a point-in-time snapshot of request counters, connection pool state, per-connection health, policy-level breakdowns, and router cache state. All fields are JSON-tagged for easy serialization.
 
+Metrics come in two tiers. The **per-request counters** (`requests`, `failures`, and responses-by-status) are always collected via lock-free atomics and returned by `Metrics()` regardless of configuration. The **detailed-metrics snapshot** (connection-pool state, per-connection health, per-policy breakdowns, and router cache state) is opt-in behind `EnableMetrics`.
+
 ## Quick Start
 
-Metrics collection is opt-in. Set `EnableMetrics: true` on `opensearch.Config`; without it, `client.Metrics()` returns `"transport metrics not enabled"`. When constructing through `opensearchapi.NewClient`, set the flag on the embedded `opensearch.Config` and reach the method via `apiClient.Client.Metrics()`.
+The per-request counters require no configuration. To also populate the detailed-metrics snapshot, set `EnableMetrics: true` on `opensearch.Config`. When constructing through `opensearchapi.NewClient`, set the flag on the embedded `opensearch.Config` and reach the method via `apiClient.Client.Metrics()`.
 
 ```go
 client, err := opensearch.NewClient(opensearch.Config{
@@ -24,7 +26,7 @@ data, _ := json.MarshalIndent(m, "", "  ")
 fmt.Println(string(data))
 ```
 
-The `Metrics()` method lives on `opensearch.Client`. It returns an `opensearchtransport.Metrics` struct and an error -- non-nil when metrics are disabled or a snapshot callback fails.
+The `Metrics()` method lives on `opensearch.Client`. It returns an `opensearchtransport.Metrics` struct and an error -- non-nil when a detailed-metrics snapshot callback fails. A `New()`-constructed transport always returns the per-request counters, so `Metrics()` does not error merely because `EnableMetrics` is unset.
 
 ---
 

opensearchtransport/doc.go

@@ -198,7 +198,10 @@ Use the EnableDebugLogger option to enable the debugging logger for connection m
 Alternatively, set the OPENSEARCH_GO_DEBUG environment variable to "true" to enable debug
 logging globally without code changes. When enabled, debug output is written to stderr.
 
-Use the EnableMetrics option to enable metric collection and export.
+Use the EnableMetrics option to enable the detailed-metrics snapshot
+(per-connection, per-policy, and router state). The per-request counters
+(requests, failures, responses by status) are always collected and are
+returned by Metrics regardless of this option.
 
 # Controlling the Router via Environment Variable
 

opensearchtransport/metrics.go

@@ -29,11 +29,9 @@ package opensearchtransport
 import (
 	"errors"
 	"fmt"
-	"maps"
 	"slices"
 	"strconv"
 	"strings"
-	"sync"
 	"sync/atomic"
 	"time"
 )
@@ -43,10 +41,23 @@ type Measurable interface {
 	Metrics() (Metrics, error)
 }
 
+// Response status codes are bounded to [statusMin, statusMax). Anything
+// outside that range is folded into a single overflow bucket so a malformed
+// upstream status is still counted rather than panicking on a bad index.
+const (
+	statusMin      = 100
+	statusMax      = 600
+	statusBuckets  = statusMax - statusMin // 500 in-range buckets
+	statusOverflow = -1                    // map key for the overflow bucket
+)
+
 // Metrics represents the transport metrics.
 type Metrics struct {
-	Requests  int         `json:"requests"`
-	Failures  int         `json:"failures"`
+	Requests int `json:"requests"`
+	Failures int `json:"failures"`
+	// Responses counts responses by HTTP status code. Any status outside the
+	// valid [100, 600) range is folded into a single overflow bucket keyed by
+	// -1 (statusOverflow) rather than its literal code.
 	Responses map[int]int `json:"responses"`
 
 	// Connection pool state.
@@ -79,10 +90,12 @@ type Metrics struct {
 
 	Connections []fmt.Stringer `json:"connections"`
 
-	// Per-policy breakdown (only populated when router with policies is active)
+	// Per-policy breakdown. Part of the detailed-metrics path: populated only
+	// when EnableMetrics is set and a router with policies is active; nil otherwise.
 	Policies []PolicySnapshot `json:"policies,omitempty"`
 
-	// Router cache state (only populated when scored routing is active)
+	// Router cache state. Part of the detailed-metrics path: populated only
+	// when EnableMetrics is set and scored routing is active; nil otherwise.
 	Router *RouterSnapshot `json:"router,omitempty"`
 }
 
@@ -214,6 +227,12 @@ type metrics struct {
 	standbyPromotions atomic.Int64 // Standby -> Active
 	standbyDemotions  atomic.Int64 // Active -> Standby
 
+	// detailed gates the expensive detailed-metrics path: registration and
+	// invocation of the per-connection / per-policy / per-snapshot callbacks and
+	// the connection-state enumeration in Metrics. The cheap per-request counters
+	// above are populated regardless of detailed. Set from Config.EnableMetrics.
+	detailed bool
+
 	// Metric callbacks registered by policies at init time.
 	// Immutable after client construction; no synchronization needed.
 	connMetricCallbacks []ConnectionMetricCallback // batch per-connection
@@ -225,52 +244,61 @@ type metrics struct {
 	addressResolverRewrites atomic.Int64 // Resolver returned a different URL
 	addressResolverErrors   atomic.Int64 // Resolver returned an error
 
-	mu struct {
-		sync.RWMutex
-		responses map[int]int
-	}
+	// responses counts HTTP responses by status code, lock-free. Index i holds
+	// the count for status code statusMin+i; responsesOverflow holds any code
+	// outside [statusMin, statusMax). Snapshotted in responsesSnapshot.
+	responses         [statusBuckets]atomic.Int64
+	responsesOverflow atomic.Int64
+}
+
+// detailedEnabled reports whether the detailed-metrics path is active. It is nil-safe:
+// a nil *metrics (no metrics struct wired) reports false, so callers can guard
+// detailed-only work with config.metrics.detailedEnabled() without a separate nil check.
+func (m *metrics) detailedEnabled() bool {
+	return m != nil && m.detailed
 }
 
-// incrementResponse increments the counter for the given status code.
+// incrementResponse records one response with the given status code. It is
+// lock-free: a single atomic add to the bucket for statusCode, or to the
+// overflow bucket when statusCode is outside [statusMin, statusMax).
 func (m *metrics) incrementResponse(statusCode int) {
-	m.mu.Lock()
-	m.mu.responses[statusCode]++
-	m.mu.Unlock()
+	if statusCode < statusMin || statusCode >= statusMax {
+		m.responsesOverflow.Add(1)
+		return
+	}
+	m.responses[statusCode-statusMin].Add(1)
+}
+
+// responsesSnapshot returns a map of status code to count. Codes with a zero
+// count are omitted; the overflow bucket, when non-zero, is keyed by
+// statusOverflow.
+func (m *metrics) responsesSnapshot() map[int]int {
+	out := make(map[int]int)
+	for i := range m.responses {
+		if n := m.responses[i].Load(); n > 0 {
+			out[statusMin+i] = int(n)
+		}
+	}
+	if n := m.responsesOverflow.Load(); n > 0 {
+		out[statusOverflow] = int(n)
+	}
+	return out
 }
 
-// Metrics returns the transport metrics.
+// Metrics returns the transport metrics. The detailed fields -- per-connection
+// enumeration, per-policy snapshots, and the router snapshot -- are populated
+// only when Config.EnableMetrics is set.
 func (c *Transport) Metrics() (Metrics, error) {
 	if c.metrics == nil {
+		// Defensive: a custom transport could embed *Transport without the
+		// standard constructor. Treat as no metrics available.
 		return Metrics{}, errors.New("transport metrics not enabled")
 	}
 
-	// Build responses map with pre-allocated capacity (READ operation)
-	c.metrics.mu.RLock()
-	responses := make(map[int]int, len(c.metrics.mu.responses))
-	maps.Copy(responses, c.metrics.mu.responses)
-	c.metrics.mu.RUnlock()
-
-	// Get connections from current connection pool
-	var ready, dead []*Connection
-	var singleConns []*Connection
-	c.mu.RLock()
-	if c.mu.connectionPool != nil {
-		switch pool := c.mu.connectionPool.(type) {
-		case *multiServerPool:
-			ready, dead = pool.connectionsByState()
-		case *singleServerPool:
-			singleConns = pool.connections()
-		}
-	}
-	c.mu.RUnlock()
-
 	m := Metrics{
 		Requests:  int(c.metrics.requests.Load()),
 		Failures:  int(c.metrics.failures.Load()),
-		Responses: responses,
-
-		LiveConnections: len(ready) + len(singleConns),
-		DeadConnections: len(dead),
+		Responses: c.metrics.responsesSnapshot(),
 
 		ConnectionsPromoted: int(c.metrics.connectionsPromoted.Load()),
 		ConnectionsDemoted:  int(c.metrics.connectionsDemoted.Load()),
@@ -280,7 +308,6 @@ func (c *Transport) Metrics() (Metrics, error) {
 		ClusterHealthChecks: int(c.metrics.clusterHealthChecks.Load()),
 		HealthChecksSuccess: int(c.metrics.healthChecksSuccess.Load()),
 		HealthChecksFailed:  int(c.metrics.healthChecksFailed.Load()),
-		OverloadedServers:   0, // Set below when iterating connections
 
 		StandbyPromotions: int(c.metrics.standbyPromotions.Load()),
 		StandbyDemotions:  int(c.metrics.standbyDemotions.Load()),
@@ -290,6 +317,31 @@ func (c *Transport) Metrics() (Metrics, error) {
 		AddressResolverErrors:   int(c.metrics.addressResolverErrors.Load()),
 	}
 
+	// Detailed-metrics path: connection enumeration + callbacks. The detailed-only
+	// fields (LiveConnections, DeadConnections, OverloadedServers,
+	// StandbyConnections, Connections, Policies, Router) stay zero/nil when
+	// the detailed path is off.
+	if !c.metrics.detailed {
+		return m, nil
+	}
+
+	// Get connections from current connection pool
+	var ready, dead []*Connection
+	var singleConns []*Connection
+	c.mu.RLock()
+	if c.mu.connectionPool != nil {
+		switch pool := c.mu.connectionPool.(type) {
+		case *multiServerPool:
+			ready, dead = pool.connectionsByState()
+		case *singleServerPool:
+			singleConns = pool.connections()
+		}
+	}
+	c.mu.RUnlock()
+
+	m.LiveConnections = len(ready) + len(singleConns)
+	m.DeadConnections = len(dead)
+
 	// Build per-connection metrics. Each connection's connState atomic
 	// determines isDead/isStandby/isOverloaded -- no positional tricks needed.
 	//