Skip to content

feat(bigtable): populate attempt_latencies2 via peer info#14245

Open
sushanb wants to merge 1 commit intogoogleapis:mainfrom
sushanb:attempt2_reason
Open

feat(bigtable): populate attempt_latencies2 via peer info#14245
sushanb wants to merge 1 commit intogoogleapis:mainfrom
sushanb:attempt2_reason

Conversation

@sushanb
Copy link
Copy Markdown
Contributor

@sushanb sushanb commented Mar 24, 2026

No description provided.

@sushanb sushanb requested a review from bhshkh March 24, 2026 04:32
@sushanb sushanb requested review from a team as code owners March 24, 2026 04:32
@product-auto-label product-auto-label bot added the api: bigtable Issues related to the Bigtable API. label Mar 24, 2026
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 introduces a new metric attempt_latencies2 to capture attempt latencies with additional transport-related dimensions from peer info. The changes look mostly good, but there's an issue in how the transport region and zone are being populated, which would lead to incorrect metric data. I've left a comment with a suggestion to fix it.

Comment on lines +725 to +726
mt.currOp.currAttempt.setTransportRegion(peerInfo.GetApplicationFrontendZone())
mt.currOp.currAttempt.setTransportZone(peerInfo.GetApplicationFrontendZone())
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

Both transportRegion and transportZone are being set to the same value from peerInfo.GetApplicationFrontendZone(). This is incorrect. The region should be extracted from the zone string. For example, if the zone is us-central1-b, the region should be us-central1.

You can extract the region by taking the substring before the last hyphen. Please also remember to add import "strings" to the file.

mt.currOp.currAttempt.setTransportZone(peerInfo.GetApplicationFrontendZone())
		zone := peerInfo.GetApplicationFrontendZone()
		region := zone
		if i := strings.LastIndex(zone, "-"); i != -1 {
			region = zone[:i]
		}
		mt.currOp.currAttempt.setTransportRegion(region)

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

Labels

api: bigtable Issues related to the Bigtable API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant