Adds stats to topo

[rafael]

Description

We are in the process of doing a topology migration. While we are doing this, it will be helpful to have more telemetry into the operations that Vitess is doing to the underlying lock service.

The following PR adds stats_conn. This is a decorator for the topology connection that emits stats when any operation is performed. It maintains stats per operation and cell.

Tests

I added relevant unit tests and tested in our dev environment.

[demmer]

Overall this looks good

[demmer]

I don't see any reason why we need topoStatsConnCounts as well as topoStatsConnTimings since the histograms already include the count (along with the timing measures).

Furthermore I think we should name these: TopologyOperations (the timings) and TopologyErrors (counter)

[rafael]

Oh I did not know this. Cool.

[demmer]

IMO we should do this decoration all the time and not require a runtime flag.

There's no real downside to having the metrics tracked.

[brirams]

seems reasonable to me. just nits that you may take or leave as you see fit

[falun]

Change seems pretty chill; one question about stat granularity

[falun]

I'm not sure how much visibility you want into the class of failures but it potentially useful to tag based on the error code. So something like adding:

// vt/topo/errors.go
// caveat: ideally we'd convert this back to *string for display or have a parallel ErrorCodeName
func ErrorCode(e error) *ErrorCode {
	if e == nil { return nil }
	switch err := e.(type) {
	case *Error:
		return err.code
	case Error:
		return err.code
	default:
		return nil
	}
}

and then in your error handling (I'm taking some liberty with go for brevity ❤️ ... and also eliding some of the necessary error handling / details)

if err != nil {
  topoStatsConnErrors.Add(statsKey, int64(1))
  topoStatsConnScopedErrors.Add([...statsKey, topo.ErrorCode(err)], int64(1))
}

[rafael]

I think for now I would like to keep the generic error count. I'm mostly interested in tracking general errors as we make the changes from skipping consul agent and talking to consul via the HA proxy vs normal operations.

Should we iterate in the future if we see need for more granularity ?

[falun]

Minor, not strong, objections; presumably something downstream of these calls is logging error codes so we can check aggregation for details if necessary?

My thought here is that if our error counts are non-zero (and we don't know because we don't have telemetry right now, correct?) then moving from non-zero to non-zero as we shift between consul and consul-via-proxy doesn't actually tell us if we've functionally changed the behavior since the type of errors are also important.

Two caveats

1. If we know/suspect there are no errors in steady-state then this is less important
2. If my read of stats.CountersWithMultiLabels is incorrect and it's more effort than just adding a single scoped counter then the effort:(payoff | code noise) ratio changes and it's less compelling for me

That said, if you want, I'm pretty okay pushing this into the the future should we need it.

[rafael]

I agree with your reasoning here.

My current understanding is number 1. I'm expecting to have zero errors during normal operations. So thinking it should be ok.

[rafael]

Thanks for the feedback! I think all comments have been addressed.