-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(bigtable): first_response_latencies and connectivity_error_count metrics #10616
base: main
Are you sure you want to change the base?
Conversation
@@ -397,6 +400,7 @@ func (t *Table) readRows(ctx context.Context, arg RowSet, f func(Row) bool, mt * | |||
*headerMD, _ = stream.Header() | |||
for { | |||
res, err := stream.Recv() | |||
mt.currOp.setFirstRespTime(time.Now()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when multiple responses are received?
what happens when there are no rows returned?
is the behavior consistent with java?
Does this work across retries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when multiple responses are received?
Updated the code to set only on the first response.
serverLatAttrs, _ := mt.toOtelMetricAttrs(metricNameServerLatencies) | ||
if mt.currOp.currAttempt.serverLatencyErr == nil { | ||
mt.instrumentServerLatencies.Record(mt.ctx, mt.currOp.currAttempt.serverLatency, metric.WithAttributes(serverLatAttrs...)) | ||
} | ||
if mt.currOp.currAttempt.serverLatencyErr == nil && mt.currOp.currAttempt.locationErr == nil { | ||
mt.instrumentConnErrCount.Add(mt.ctx, 0, metric.WithAttributes(connErrCountAttrs...)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need to add a zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR adds below metrics: