Regarding pkg.cri.sbserver vs pkg.cri.server, I'm unsure how much we'd like to expose to users.

I wanted to follow a common naming convention for the spans, so used : "relative path to the package"+ "name describing the span logic or the method", so added the name. Any reason we do not want to expose server vs sbserver difference? I was assuming this could help catch divergence issues between the both.

Because we eventually migrate from pkg.cri.server to pkg.cri.sbserver and don't want to surprise the consumer of the traces.

That being said, I do agree having different names will help users spot the difference in between.

For RecordError calls, how about renaming this function to something like pullImage and calling it like below? Calling RecordError() on all branches seems error-prone.

resp, err := pullImage(...)
if err != nil {
  span.RecordError(err)

Good point. I can take a look into creating a thin wrapper

I'm also concerned about the pattern of span.RecordError(err) in each branch; that's not going to be easy to enforce as we continue to maintain the code.

I'd prefer the wrapper, but another reasonable approach would be to change the signature to use a named return:

func (c *criService) PullImage(ctx context.Context, r *runtime.PullImageRequest) (_ *runtime.PullImageResponse, err error)

and then capture the error in a defer:

defer func(){
	if err != nil {
		span.RecordError(err)
	}
}()

On further reflection: for sbserver and server, can we create the spans inside a gRPC interceptor and stuff them into ctx if we later want to add attributes?

Hi @samuelkarp, I already updated this PR to add a wrapper and removed most of the RecordError calls. Please take a look. I am fine changing the current impl to use a defer func with named error, if we strongly feel about it.

for sbserver and server, can we create the spans inside a gRPC interceptor and stuff them into ctx if we later want to add attributes

IIUC we already have a span from the gRPC interceptor which is currently the top level span. This PR adds additional child spans for tracing after the gRPC calls is received. Sure, we can keep using the parent gRPC span as well to add the attributes, but I think ability to trace which packages are being invoked is an added advantage.

I'm hesitant to expose Go's unexported methods to spans. Wouldn't people treat them as a part of the published interface?

I am not sure I agree to that. Isn't the idea of tracing is being able to look into the unexposed methods, say a method that does network calls ,to get more visibility into the network latency?

For this case though, I don't think there is any real value to instrumenting this method so I will go ahead and remove the instrumentation.

I think there can be value for long operations like network, but not for small functions like this.

Updated PR to remove this operation

Would it be different from imageRef?

I think they are the same, but I might be wrong

Image object has both ID (digest of image config) and References (repo tag, repo digest)

ID: sha256:1123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef
References: []string{
			"docker.io/library/busybox:latest",
			"docker.io/library/busybox@sha256:e6693c20186f837fc393390135d8a598a96a833917917789d63766cab6c59582",
		},

However, In containerd metadata store, I see that we assign Image.ID to metadata ImageRef https://github.com/containerd/containerd/blob/main/pkg/cri/server/container_create.go#L206. So within containerd, imageRef is treated same as image.ID

Thanks. How about using imageRef here then? I'd like to keep attributes consistent.

okay. I am changing all the attributes to lowercase and dot notation, so this will be "image.ref"

We have snapshotter as well.

What is the difference? If no valid snapshotter is provided, the snapshotterName defaults to "overlayfs" so decided to use this value

Oh sorry, I meant, we have snapshotterName and snapshotter as attributes. If they are semantically same, we should pick one (I prefer snapshotter).

updated. I decided to go with "snapshotter.name". also we have "image.name" and "image.ref" so it felt more semantically similar

I'm also concerned about the pattern of span.RecordError(err) in each branch; that's not going to be easy to enforce as we continue to maintain the code.

I'd prefer the wrapper, but another reasonable approach would be to change the signature to use a named return:

func (c *criService) PullImage(ctx context.Context, r *runtime.PullImageRequest) (_ *runtime.PullImageResponse, err error)

and then capture the error in a defer:

defer func(){
	if err != nil {
		span.RecordError(err)
	}
}()

On further reflection: for sbserver and server, can we create the spans inside a gRPC interceptor and stuff them into ctx if we later want to add attributes?

Maybe just use "ref" as the name?

I made some changes to the attribute names, based on recommednations in the https://opentelemetry.io/docs/reference/specification/common/attribute-naming/. As per that, I went with image.ref.

I think there can be value for long operations like network, but not for small functions like this.

We probably also want a span within the unpack operation. The pull.UnpackWait span will capture the delay imposed on the overall pull operation, but won't capture the actual time it takes to unpack a layer.

Updated PR to add spans around unpack operations

This is how the new traces will look like

Also added span attributes for the descriptor mediatype, size and digest, to identify which particular oci_descriptor/layer information

We should pick either image.name or image.ref, assuming both of them would have something like docker.io/library/debian:latest.

Wherever I used image.name refers to name of the images docker.io/library/debian:latest

image.ref refers to the image id (which is the digest of the image config) sha256:51086ed63d8cba3a6a3d94ecd103e9638b4cb8533bb896caf2cda04fb79b862f

Is the digest same as the image's digest? How about calling it image.digest

We use ref as a way to refer the human-readable image name. We shouldn't use that for something else.

I see, sure I can change image.name to image.ref

As for ctr

[ec2-user@ip-172-31-62-162 client]$ sudo ctr i ls
REF                             TYPE                                                      DIGEST                                                                  SIZE     PLATFORMS                                                                                               LABELS
docker.io/library/debian:latest application/vnd.docker.distribution.manifest.list.v2+json sha256:e538a2f0566efc44db21503277c7312a142f4d0dedc5d2886932b92626104bff 52.5 MiB linux/386,linux/amd64,linux/arm/v5,linux/arm/v7,linux/arm64/v8,linux/mips64le,linux/ppc64le,linux/s390x -

Digest here refers to the image Index file,

[ec2-user@ip-172-31-62-162 client]$ cat  /var/lib/containerd/io.containerd.content.v1.content/blobs/sha256/e538a2f0566efc44db21503277c7312a142f4d0dedc5d2886932b92626104bff| json_reformat | tail -n 10
            "platform": {
                "architecture": "s390x",
                "os": "linux"
            },
            "size": 529
        }
    ],
    "mediaType": "application/vnd.docker.distribution.manifest.list.v2+json",
    "schemaVersion": 2
}

this is different from image.id which refers to the image config digest.

The way I see this, ID is used to refer to unique images (platform + name), REF is a generic name for the image that can belong to any platform, DIGEST is used to denote the index file for the image.

Okay, this is what I am using now:
image.ref - name of the image such as docker.io/library/debian:latest
image.id - digest of the image config which is also the unique image identifier

https://github.com/microsoft/hcsshim/blob/main/cmd/containerd-shim-runhcs-v1/service.go#L100

Here is an example of how we did this in hcsshim. Basically we set the error state based on a defer rather than needing to wrap all methods. That way any call that returns the error from func will apply.

Thanks for the comments @jterry75. I will implement some of the changes based on your recommendations:

1. Create the top level span at the InstrumentedService layer. Use the same span in the CRI layer and create new spans. I will add child spans when we reach containerd client side methods[same as today].
2. Use the defer approach to catch errors during exit at the InstrumentedService layer.

Also, need a clarification regardingspan.RecordError(err). RecordError() does not set the status of the Span to error. it is simply recording the error in the span similar to how we add attributes.

From otel doc:

It is highly recommended that you also set a span’s status to Error when using RecordError, unless you do not wish to consider the span tracking a failed operation as an error span. The RecordError function does not automatically set a span status when called.

Currently, I am considering the span to be still valid even though we might encounter a failed operation. Is there any real benefit of marking the span as an error span when we encounter an error in the code?

Oh what an odd change in otel. Wow I guess I need to freshen up. IMO yes. The span status will be wrong if we dont set it to error when there was an api failure. So yes a human can read the logs and sorta make sense but anything scraping the output for alarms/errors/etc will not because it will not see the status as a failure. We dont want to have to add special cases for every api like "output was X on PullImage when status was SUCCESS == an error". That is just a poor experience.

I do get the case for recording an error without the span failing I guess. If for example we retried an operation internally but failed on the first attempt etc. But if the overall operation fails, I feel like we must set the span status to error or else its misleading right?

Love love love to set attributes on existing spans!!!!!!

Again if we defer this it would hit any branch.

Can we do this again in the instrumented service rather than duplicating functions here?

This is a good example of why defer matters. We actually arent going to error from the api when it IsNotFound so we shouldnt process it then.

I was actually looking at it a little differently. If the image reference does not exist, client would only see an empty image status response and no error. Looking at the trace can provide information about the underlying error which was encountered. This will be true even if I use defer, sicne only returned errors will be caught.

This is new for otel so yes my expectation here was off. It sorta feels odd to record this as an error though given that its "expected" from the api perspective. Its not like the retry example I gave above that could be valid for recording an error case.

No error state?

Nice to see we can use instrumentedService.

Should we make this strings.Join a part of tracing package API?

For instance, StartSpan could accept arbitrary number of strings, like this:
tracing.StartSpan(ctx, criSbServerSpanPrefix, "PullImage")

We can but does it bring any value? it will be simpler if the instrumentation side makes the decision on how to create the span name( add prefix, suffix or use any custom logic)

add prefix, suffix or use any custom logic

Does it make sense to be consistent everywhere?
Like we'd specify tracing.StartSpan(ctx, "a", "b", "c") and it'll apply same separator across the project.

Okay, I see your point. I am planning to change the current span methods once I add a wrapper object, so I will add this change in the follow up PR.

defer span.End() or defer span.Stop() or defer span.Close() feels more natural (we'd probably have to wrap span object).

yeah, that is a good point. I can make the change and wrap trace.Span object. As for context propagation, I am thinking I will keep the existing logic. I can modify StartSpan and CurrentSpan to return me the wrapper Span, but keep span context propagation as is.

I'd prefer a wrapper object, but I don't want to block this PR. That's fine to keep it as is and/or follow up later.

Sounds good to me. I will send a follow up PR adding the wrapper object and restructure some of the current span methods.