Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces an optimization to cache OpenTelemetry (OTel) attribute objects within the MetricsTracer and MetricsTracerFactory, preventing redundant object allocations and cloning during metric recording. Comprehensive unit tests have been added to verify the caching behavior and its lifecycle. The feedback recommends replacing a non-null assertion operator (!) with optional chaining (?.) in _createOperationOtelAttributes to avoid potential runtime TypeErrors when currentOperation is null.
299d468 to
9994776
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces caching of OpenTelemetry (OTel) attribute objects in both MetricsTracer and MetricsTracerFactory to prevent repeated object allocations and cloning during metric recording. It also adds comprehensive unit tests to validate the caching behavior. The review feedback points out a potential runtime crash due to non-null assertions on this.currentOperation and currentAttempt in recordAttemptCompletion, recommending a defensive early return to ensure safety.
Caches OpenTelemetry metric attribute objects to eliminate repeated object allocations and reverse-enum lookups on hot RPC execution paths:
- Replaces repeated object cloning (`{...this._clientAttributes}`) across attempt, operation, GFE, and AFE metric recordings with a cached lookup keyed by status code.
- Shares the attribute cache across MetricsTracer instances in MetricsTracerFactory per database, instance, and method, ensuring zero attribute object allocations in steady state.
- Freezes cached attribute objects to guarantee immutability across shared references.
- Fixes defensive checks in attempt attribute generation when operations or attempts are uninitialized.
- Ensures consistent fallback to UNKNOWN status across attempt and operation metrics for unrecognized status codes.
9994776 to
c557ff9
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an OpenTelemetry (OTel) attributes caching mechanism in the Spanner metrics tracer to avoid repeated object allocations and cloning during metric recording. Specifically, it adds an attributes cache map to MetricsTracerFactory and MetricsTracer, allowing tracers for the same method and resource to share and reuse frozen attribute objects. Additionally, several non-null assertions have been replaced with safer optional chaining or null checks, and comprehensive unit tests have been added to verify the caching behavior and safety improvements. As there are no review comments, no further feedback is provided.
Caches OpenTelemetry metric attribute objects to eliminate repeated object allocations and reverse-enum lookups on hot RPC execution paths:
{...this._clientAttributes}) across attempt, operation, GFE, and AFE metric recordings with a cached lookup keyed by status code.