Skip to content

perf(spanner): cache OTel attribute objects in metrics tracer - #9309

Open
olavloite wants to merge 1 commit into
mainfrom
spanner-cache-metrics-attributes
Open

olavloite wants to merge 1 commit into
mainfrom
spanner-cache-metrics-attributes

Conversation

@olavloite

Copy link
Copy Markdown
Contributor

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.

@olavloite
olavloite requested a review from a team as a code owner September 13, 2026 07:41
@product-auto-label product-auto-label Bot added the api: spanner Issues related to the Spanner API. label Sep 13, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread handwritten/spanner/src/metrics/metrics-tracer.ts Outdated
@olavloite
olavloite force-pushed the spanner-cache-metrics-attributes branch from 299d468 to 9994776 Compare September 13, 2026 10:28
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread handwritten/spanner/src/metrics/metrics-tracer.ts Outdated
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.
@olavloite
olavloite force-pushed the spanner-cache-metrics-attributes branch from 9994776 to c557ff9 Compare September 13, 2026 10:47
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: spanner Issues related to the Spanner API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant