really not a fan of leaking the storage implementation here

I can just always fill those details here. It's generic enough that it should be possible to implement in arbitrary backend.

this sets up a duplicate prefix construction path that doesn't honor the configured etcd prefix (--etcd-prefix), and assumes nesting under the API group name, which isn't a valid assumption

Prefix is easily fixable, I'm not sure about the api group name - why it's not valid (except core/v1 ones)?

none of the kube-apiserver types place themselves under group subpaths. see test/integration/etcd/etcd_storage_path_test.go for all the various non-standard key paths used.

I don't think we should set up another place like this where we try to pull together all the etcd prefix + resource prefix config values... if we want to run metrics on storage, why not do it inside generic store and storage, where this information already lives?

I was looking into that yesterday, but got pulled into different things before I was able to get to the bottom of this. Short answer is that I have no idea how the storage code is structured.

The way I understand it is that Storage (the layer that talks to etcd and the place where monitoring needs to live) is created before generic.Store(s) and get injected into Stores by something (Decorator)?.

So I can either modify the injection logic, to also pass some data down to Storage (which does have an unpleasant smell to it), or add a simple "Count" function to Storage. I'd rather do the latter, but I don't really want to make it full blown API-like request.

The issue with the latter is that I have to either provide EXTREMELY inefficient implementations for etcdv2 and Cacher, or just always return -1/unimplemented error. Neither is particularly nice...

or add a simple "Count" function to Storage. I'd rather do the latter, but I don't really want to make it full blown API-like request

That is the most accurate representation of what this is attempting to do. If we want to pull info out of etcd in a new way, the Storage interface is the correct place to represent that.

I have to either provide EXTREMELY inefficient implementations for etcdv2 and Cacher

Etcd2 returning an unimplemented error doesn't particularly bother me. Cacher could delegate to underlying store (as it does for most things), or add a KeyCount() function to the cache.Store interface

This is what I'll try to send out today, assuming I'll stop getting interrupted...

I don't know how expensive a call this is, but counting every resource type every 5 seconds is more frequent than I expected. who would be a good resource to ask about this?

I can put anything you prefer here. As for resources to know I guess someone from etcd team - maybe @xiang90 still works on that?

I increased it to one minute interval. It should be good enough, but we'll see how scale tests will behave. @shyamjvs @kubernetes/sig-scalability-misc

I don't think the comment is accurate... looks like it would work without the watch cache, right?

Right.

don't run forever, make sure we have a stop channel and non-nil Destroy() func that will stop this

for example:

stopMetricsCh := make(chan struct{})
destroy := e.DestroyFunc
e.DestroyFunc = func(){
  close(stopMetricsCh)
  if destroy != nil {
    destroy()
  }
}

e.startObservingCount(..., stopMetricsCh)

don't join opts.StorageConfig.Prefix+prefix here... the storage is responsible for dealing with prepending opts.StorageConfig.Prefix

also, don't assume prefix is the key... call e.KeyRootFunc() to get the key

I started to return a destroy function from 'start...', which is functionally equivalent.

simplify to go wait.Forever(...?

Done.

I'd expect the key parameter to match that passed to GetToList(), etc, and get joined with the pathPrefix already held by store

func (s *store) Count(key string) (int64, error) {
  key = path.Join(s.pathPrefix, key)
  getResp, err := s.client.KV.Get(context.Background(), key, ...
  ...
}

Done.

What happens for co-habitation? Will we get two monitors?

I don't know storage code good enough to answer that. It depends on how co-habitation is implemented and whether two Stores for co-habitating resources share the same Storage object, or control will enter the if Storage == nilbranch twice.

Having two monitors is not ideal, but it's also not a disaster IMO.

Agree. We can live with that.

nit: returns.

Done.

is this an error? If we want to distinguish between error and "not implement" we should have at least a special pre-defined error type.

Agreed, but I guess we'd need to add it to apimachinery/pkg/api/errors to be consistently used. I can add it in separate PR, as I expect a lot of discussions around that.

Alternative: convention that -1 means "not implemented"

-1 currently means "no data", as per @liggitt request. Also we're not writing in C to do such things;)

It's not "Current". It's number of objects from some period of time in the last minute (or whetever user will say). May be worth being explicit...

I will change it, but frankly no metric is ever "current" in the strict sense:)

This log level would include client calls. You probably want 5, just before the client calls

Done.

instead of time.Minute using s.StorageConfig.CountMetricPollPeriod allows overriding defaults like the other options above

And you can still set a default in NewEtcdOptions above

It's not perfect, but done.

we should not issue large range to etcd. just to count the keys, we should use keys only option with limit and maybe also with pagination.

is WithCountOnly not more efficient than WithKeysOnly?

oh. i did not see count only is already there. but still if we know the count is going to be huge, probably need pagination as well. etcd does not maintain a index for count(*), but scan everything internally.

doing a micro-benchmark could be helpful. if count 1M keys take <1s, it might be ok.

If we would have 1M keys, we would have other problems than this metric...

counting 1M keys just to make the test result more reliable and safer. you can divide that by 10 to get 100k for an upper bound, etc.. periodically blocking etcd for more than 50ms is not good in general (for node kinds there can be 5k, for pods there can be 50k). i have not really looked into this pr. so just some random suggestions and thoughts on this.

@xiang90

I can run benchmarks after the freeze, as it's not enough time now. It shouldn't be an issue, as it's a 1-word change to disable this functionality.

As for pagination I'm not sure I follow. I believe that etcd-side 'WithLimit' restricts response size (i.e. number of items returned), which shouldn't apply to "count only" requests, as they always return single value. Or is the 'countOnly' implemented on the client side, thus "limit" will effectively cap the returned value to, well, limit?

Or maybe you meant doing some kind of manual pagination by dividing the range into multiple subranges, querying them separately and combining the result?