Plugins: Chore: Renamed instrumentation middleware to metrics middleware

[zaibkhan]

Mirrors ai-code-review-evaluation#3 for like-for-like benchmarking.

• Base: main
• Head: plugins/rename-instrumentation-middleware-to-metrics-middleware

Original PR excerpt:

Test 3

[codoki-pr-intelligence]

Codoki PR Review

Summary: Rename instrumentation to metrics, decouple logging context
What’s good: Clean split between metrics collection and contextual logging; logger interface gained FromContext with a concrete wrapper, making logging more composable.
Review Status: ❌ Requires changes
Overall Priority: High

Issues (Critical & High only)

Severity	Issue	Why it matters
High	Correctness — Shared state in NewMetricsMiddleware can cause cross-clie...	…/clientmiddleware/metrics_middleware.go This mutates a shared middleware instance captured by the closure; if the middleware is applied multiple times, "next" will be overwritten and calls may route to the wrong client or race under concurrency. Construct a fresh wrapper per application while reusing the registered collectors.

Showing top 1 issues. Critical: 0, High: 1. See inline suggestions for more.

Key Feedback (click to expand)

• Needs improvement: Metrics middleware reuses a single instance across clients by mutating next, which can cause unsafe cross-client state; duration histogram uses millisecond units but second-like buckets, skewing telemetry.
• Testing: Add unit tests for ContextualLoggerMiddleware to assert context contains endpoint/pluginId/dsUID/uname (e.g., by retrieving a logger via log.FromContext and checking structured fields), and a test that the middleware stack order includes ContextualLoggerMiddleware before LoggerMiddleware. Consider a concurrency test to apply NewMetricsMiddleware more than once and verify no shared state mutation across returned clients.
• Documentation: Document the required middleware order: ContextualLoggerMiddleware must precede LoggerMiddleware to ensure logs include endpoint/plugin metadata. Also note the rename from NewInstrumentationMiddleware to NewMetricsMiddleware as a breaking change.
• Compatibility: NewInstrumentationMiddleware and newInstrumentationMiddleware were removed; if external code consumes these, this is a breaking API change. Consider a temporary deprecated alias or a clear migration note in release docs.
• Open questions: Are there scenarios where NewLoggerMiddleware is used without NewContextualLoggerMiddleware earlier in the chain? If so, logs will miss pluginId/endpoint unless another middleware populates the context.

Confidence: 3/5 — Needs work before merge (1 high · status: Requires changes)

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Metrics as MetricsMiddleware
    participant CtxLog as ContextualLoggerMiddleware
    participant LogMw as LoggerMiddleware
    Caller->>Metrics: QueryData(req)
    Metrics->>Metrics: instrumentPluginRequest()
    Metrics->>CtxLog: QueryData(ctx, req)
    CtxLog->>CtxLog: instrumentContext()
    CtxLog->>LogMw: QueryData(ctx, req)
    LogMw->>LogMw: logRequest()
    LogMw-->>CtxLog: resp, err
    CtxLog-->>Metrics: resp, err
    Metrics-->>Caller: resp, err

Loading

React with 👍 or 👎 if you found this review useful.

[codoki-pr-intelligence]

⚠️ High: This mutates a shared middleware instance captured by the closure; if the middleware is applied multiple times, "next" will be overwritten and calls may route to the wrong client or race under concurrency. Construct a fresh wrapper per application while reusing the registered collectors.

Suggested change

	imw.next = next
	return &MetricsMiddleware{pluginMetrics: imw.pluginMetrics, pluginRegistry: pluginRegistry, next: next}