Pre-register Micrometer meters to eliminate per-request builder chains

[xinlian12]

ClientTelemetryMetrics was rebuilding and re-registering every meter on every request (35 builder().register() calls). While Micrometer's register() is idempotent, the builder chain construction + tag assembly + registry lookup is pure waste after the first call.

Changes:

• Add MeterCache inner class with ConcurrentHashMap<Tags, Meter> per type
• Cache keyed by (CosmosMetricName, Tags) - meters built once, reused on cache hit via single ConcurrentHashMap.get()
• Replace all 34 static builder().register() patterns with getMeterCache()
• Cache RntbdMetricsV2 meters as instance fields (tags fixed per endpoint)
• Remove getPercentiles().clone() in CosmosMeterOptions (unnecessary copy)
• Clear all caches on registry add/remove for correctness
• Leave recordRequestTimeline() uncached (dynamic event names, rarely enabled)
• Leave Gauge/FunctionCounter in RntbdMetricsV2 as-is (registered once per endpoint lifecycle, not per-request)

At 37K ops/s this eliminates ~550K builder chains/sec, ~370K array clones/sec, and ~550K Micrometer internal ConcurrentHashMap lookups/sec.

[github-actions]

Warning

⚠️ Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• releaseassets.githubusercontent.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "releaseassets.githubusercontent.com"

See Network Configuration for more information.

Generated by sdkReviewAgent for issue #39

[github-actions]

🟡 Recommendation · Correctness: getPercentiles() now exposes the internal array directly

The constructor already clones the caller-supplied array (line 30: this.percentiles = percentiles != null ? percentiles.clone() : new double[0]), establishing clear defensive-copy intent. Removing the .clone() from the getter means any caller who mutates the returned array corrupts the CosmosMeterOptions internal state.

In the current usage the immediate risk is low — MeterCache.getOrCreateTimer/getOrCreateSummary pass the array into Micrometer's builder which copies it internally, and the cache hit path never calls getPercentiles() again. However, if the meter cache is cleared (registry add/remove) and meters are rebuilt, or if getPercentiles() is called from test or config-inspection code, a mutated array will produce incorrectly configured meters with no warning.

Fix: restore the defensive copy, or document the contract explicitly.

public double[] getPercentiles() {
    return this.percentiles.clone();  // restore defensive copy
}

---

_{⚠️ AI-generated review — may be incorrect. Agree? → resolve the conversation. Disagree? → reply with your reasoning.}

[github-actions]

Warning

⚠️ Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• releaseassets.githubusercontent.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "releaseassets.githubusercontent.com"

See Network Configuration for more information.

Note

🔒 Integrity filtering filtered 1 item

Integrity filtering activated and filtered the following item during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.

• pr:#39 (pull_request_read: Resource 'pr:#39' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.)

Generated by sdkReviewAgent for issue #39

[github-actions]

🟡 Recommendation · Correctness: RntbdMetricsV2 instance-field meters become stale after compositeRegistry replacement

When remove() empties all child registries, line 391 replaces compositeRegistry with a fresh empty one (compositeRegistry = createFreshRegistry()). The Timer/DistributionSummary instances assigned here (this.requestLatencyTimer, etc.) were registered with the old compositeRegistry, and they stay frozen in these instance fields for the lifetime of the RntbdMetricsV2 object.

Subsequent markComplete() calls record into these stale meters. Since the old compositeRegistry now has no child registries (they were cleared + closed), the recordings are silently dropped.

Before this PR, markComplete() re-issued getMeterCache(...).getOrCreate*(this.tags, ...) on every call. After clearAllMeterCaches(), the first post-clear call would rebuild the meter against the new compositeRegistry, self-healing the stale reference. The instance-field optimization removes that property.

Practical impact is limited — compositeRegistry is only replaced when all registries are removed, which usually coincides with client shutdown. But if a user removes all registries then adds a new one (a supported lifecycle pattern), measurements from any still-live endpoints will be silently lost until those endpoints are evicted and new RntbdMetricsV2 instances are created.

Options:

• (a) Keep instance fields but also cache the RntbdMetricsV2 in meterCaches so clearAllMeterCaches() can invalidate them and force reconstruction on next use.
• (b) Keep instance fields and document that they are scoped to the compositeRegistry that existed at construction time (acceptable if endpoint lifecycle always tracks registry lifecycle).
• (c) Route RntbdMetricsV2 meters through getMeterCache with endpoint-scoped tags, dropping the instance fields entirely (preserves the pre-PR self-healing at the cost of a single hash-map lookup per markComplete — still far cheaper than the full builder chain).

---

_{⚠️ AI-generated review — may be incorrect. Agree? → resolve the conversation. Disagree? → reply with your reasoning.}

[github-actions]

🟡 Recommendation · Test Coverage: No tests for MeterCache semantics or registry lifecycle

MeterCache is now on the hot path for every recorded metric. The core invariants have zero test coverage:

1. Cache hit — two calls with the same (CosmosMetricName, Tags) pair return the same Meter instance (not just an equal one).
2. Cache invalidation — after clearAllMeterCaches(), the next getOrCreate* call builds a fresh meter registered with the current compositeRegistry.
3. Registry replace correctness — after remove() empties all registries and replaces compositeRegistry, subsequent getOrCreate* calls populate the cache against the new registry (not the old, empty one).
4. Histogram variant isolation — getOrCreateSummary and getOrCreateSummaryNoHistogram for the same (CosmosMetricName, Tags) return meters with the correct histogram configuration (regression guard for the fix applied in the previous review round).

A self-contained unit test with a SimpleMeterRegistry (already on the compile classpath) can cover all of these without any integration dependencies. Without it, a future refactor of MeterCache has no safety net.

---

_{⚠️ AI-generated review — may be incorrect. Agree? → resolve the conversation. Disagree? → reply with your reasoning.}