Skip to content

feat(spanner): add DCP observability#14613

Open
rahul2393 wants to merge 1 commit into
dcp-split/2-directpath-fallbackfrom
dcp-split/3-observability
Open

feat(spanner): add DCP observability#14613
rahul2393 wants to merge 1 commit into
dcp-split/2-directpath-fallbackfrom
dcp-split/3-observability

Conversation

@rahul2393
Copy link
Copy Markdown
Contributor

Split of #14604

Internal reference: go/go-dcp-design

@rahul2393 rahul2393 requested review from a team as code owners May 19, 2026 12:45
@product-auto-label product-auto-label Bot added the api: spanner Issues related to the Spanner API. label May 19, 2026
@rahul2393 rahul2393 requested a review from olavloite May 19, 2026 12:46
Copy link
Copy Markdown
Contributor

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

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 implements OpenTelemetry metrics for the Spanner Dynamic Channel Pool (DCP), enabling the tracking of channel counts, RPC load, scaling events, and selection frequency. The changes include metric registration, recording logic within the pool operations, and comprehensive integration tests. Review feedback identified a bug where the channel_slot attribute was not recorded when the base attribute map was empty and suggested an optimization to avoid unnecessary slice copies when retrieving attributes to improve performance.

Comment on lines +1131 to +1144
func (p *dynamicChannelPool) setEntryOTAttributes(e *dcpEntry) []attribute.KeyValue {
if e == nil {
return nil
}
attr := p.dcpOTAttributes()
if attr == nil {
return nil
}
attr = append(attr,
attributeKeyDCPChannelSlot.Int64(e.metricSlot),
)
e.otAttributes.Store(&attr)
return attr
}
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.

high

There is a bug where the channel_slot attribute is not recorded if attributeMap is empty. This happens because p.dcpOTAttributes() returns nil when the map is empty, causing setEntryOTAttributes to return early. The check should be on whether OpenTelemetry is enabled (otConfig != nil), and it should always append the channel_slot attribute for entries. Additionally, the implementation should ensure it doesn't modify the original attributeMap by creating a new slice with sufficient capacity.

func (p *dynamicChannelPool) setEntryOTAttributes(e *dcpEntry) []attribute.KeyValue {
	if e == nil {
		return nil
	}
	otConfig := p.otConfig.Load()
	if otConfig == nil {
		return nil
	}
	attr := make([]attribute.KeyValue, 0, len(otConfig.attributeMap)+1)
	attr = append(attr, otConfig.attributeMap...)
	attr = append(attr, 
		attributeKeyDCPChannelSlot.Int64(e.metricSlot),
	)
	e.otAttributes.Store(&attr)
	return attr
}

Comment on lines +1111 to +1119
func (p *dynamicChannelPool) dcpOTAttributes() []attribute.KeyValue {
otConfig := p.otConfig.Load()
if otConfig == nil || len(otConfig.attributeMap) == 0 {
return nil
}
attr := make([]attribute.KeyValue, len(otConfig.attributeMap))
copy(attr, otConfig.attributeMap)
return attr
}
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.

medium

The current implementation of dcpOTAttributes returns nil if attributeMap is empty and performs an unnecessary shallow copy of the attributes. If OpenTelemetry is enabled, it should return the base attributes (even if empty) so that entry-specific attributes (like channel_slot) can be correctly appended in setEntryOTAttributes. Also, since the attributes are not modified by the internal callers, the copy can be avoided to improve efficiency.

func (p *dynamicChannelPool) dcpOTAttributes() []attribute.KeyValue {
	otConfig := p.otConfig.Load()
	if otConfig == nil {
		return nil
	}
	return otConfig.attributeMap
}
References
  1. In-place modification within a getter method is permissible if the data is immutable post-initialization (e.g., loaded from a configuration file), as it avoids unnecessary allocations for copies.

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