Using image name will make the cardinality too high. Prometheus' doc recommends to keep the cardinality lower than 10, so I guess registry name should be a better option here.

[1] https://prometheus.io/docs/practices/instrumentation/#do-not-overuse-labels

	imagePulls = ns.NewLabeledCounter("image_pulling", "count of image pulling by registry", "status", "registry")

I changed to this.

I don't think it's reasonable to assume that containerd will only pull from 10 distinct domains during its lifetime (which appears to be the actual distinction, not registries?). Some container registry providers use many distinct domains (for example, Amazon ECR has a different domain name for each customer account).

Should we use a latency metric instead, with error, registry as labels? By doing that users will be able to

1. see the image pull latencies,
2. see both error count and success count, and therefore calculate the error ratio.

To add a count for both error pulling and success pulling, I agree.

	imagePulls = ns.NewLabeledCounter("image_pulling", "count of image pulling by registry", "error", "registry")

For image pull latencies, do you mean the pulling duration? Could you explain?

• for the duration, I think imagePullThroughput would be better as the image size would 1MiB or 1GB. The latency does not make sense in some scenarios.

If we add the error as a label, some error message contains the image name and some detailed information. If so, the registry label is not that important. If not, we have to make the error more groupable.

I was thinking of making this metric something like this:

imagePullTimer = ns.NewLabeledTimer("image_pull", "time to pull an image", "error", "registry")

, where error is just a boolean indicating whether the pull succeeds or not.

By doing this:

1. if user wants to get the overall pulling duration, regardless of each image size, they can get this metric with error=='false'.
2. if user wants to get the number of failed image pulls, they can get counts of this metric with error=='true'

But yeah, I think what you are doing here (i.e. separating the counter metric from duration/throughput metric) is also fine. I don't have a strong preference here.

I added like

	imagePulls = ns.NewLabeledCounter("image_pulls", "count of image pulling by registry", "status", "registry")

Two value is valid: status=failed and status=succeed

I guess a common practice of handling error in defer is to use named return value, instead of using a var like here, because in this case you can't be sure err will always be the return value.

For example, if you look at

the returned error is based on a local err created in the for-loop. So your defer here will not catch that local error.

(It's similar to https://go.dev/play/p/LNkan9NkAUt)

If so, I have to wrap like below.

func (c *criService) PullImage(ctx context.Context, r *runtime.PullImageRequest) (*runtime.PullImageResponse, error) {
	resp, domain, err := c.PullImageImp(ctx, r)
	if err != nil {
		imagePulls.WithValues("failed", domain).Inc()
	} else {
		imagePulls.WithValues("succeed", domain).Inc()
	}
	return resp, err
}

PullImageImp is the original function. Is this OK?

no you dont need to. Just use a named return value for the returned error and use the named return value in your defer, and golang will make sure it is always the returned error. Like this:

func (c *criService) PullImage(ctx context.Context, r *runtime.PullImageRequest) (_ *runtime.PullImageResponse, retErr error) {
	defer func() {
		if retErr != nil {
			...
		} else {
			...
		}
	}()

As I need the domain as a label of the metrics, and the image registry domain is not returned.

This still does not work.

So I kept the change, would you take a look again?

Sorry for the late reply. I am just trying to minimize the number of wrapper methods. Can we do something like

func (c *criService) PullImage(ctx context.Context, r *runtime.PullImageRequest) (_ *runtime.PullImageResponse, retErr error) {
        var domain string
	defer func() {
		if retErr != nil {
			...
		} else {
			...
		}
	}()

and domain will be empty if it errs when trying to parse it, and will be non-empty if the parsing succeeds.

nod one metric per domain ref is probably enough..

If someone needs per image log data... maybe that should be retrieved from the log?

We might need to add the size and duration to the CRI PullImage Response..

I think per domain by default is too granular/too high cardinality.