Add a thin wrapper around otel Span object#7655
Conversation
|
Hi @swagatbora90. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cc @mxpv |
| // StartSpan starts child span in a context. | ||
| func StartSpan(ctx context.Context, opName string, opts ...trace.SpanStartOption) (context.Context, trace.Span) { | ||
| // StartSan starts child span in a context. | ||
| func StartSpan(ctx context.Context, opName string, opts ...trace.SpanStartOption) (context.Context, *Span) { |
There was a problem hiding this comment.
I thought we could have tracing.Span(ctx, prefix, name) but then it cannot take SpanStartOption...
There was a problem hiding this comment.
You are right Kaz. We can have a separate methods that pass either StartSpanOption or names. I am not sure if this is something we want.
mxpv
left a comment
mxpv
left a comment
There was a problem hiding this comment.
This is great! I have a few nit suggestions.
| ) | ||
| return images.HandlerFunc(func(ctx context.Context, desc ocispec.Descriptor) ([]ocispec.Descriptor, error) { | ||
| ctx, span := tracing.StartSpan(ctx, "pkg.unpack.unpacker.UnpackHandler") | ||
| ctx, span := tracing.StartSpan(ctx, tracing.SpanName(unpackSpanPrefix, "UnpackHandler")) |
There was a problem hiding this comment.
Do we need both StartSpan and SpanName ?
I'd suggest
ctx, span := tracing.StartSpan(ctx, unpackSpanPrefix, "UnpackHandler")There was a problem hiding this comment.
tracing.StartSpan already accepts SpanStartOption...
func StartSpan(ctx context.Context, opName string, opts ...trace.SpanStartOption) (context.Context, *Span)
so I had to use tracing.SpanName to pass the list of strings to create the span name.
The other options were to use interface{} and infer the type later, or create a new method that accepts StartSpan(ctx, name ...string) but then it cannot take SpanStartOption...
There was a problem hiding this comment.
Oh, ok, that make sense.
In this case I'd suggest tracing.Name instead of tracing.SpanName to keep it more laconic.
|
|
||
| for i, desc := range layers { | ||
| _, layerSpan := tracing.StartSpan(ctx, "pkg.unpack.unpacker.unpackLayer") | ||
| _, layerSpan := tracing.StartSpan(ctx, tracing.SpanName(unpackSpanPrefix, "unpackLayer")) |
There was a problem hiding this comment.
Too many "spans", possibly:
layerSpan.SetAttributes(
tracing.Attribute("layer.media.type", desc.MediaType),
tracing.Attribute("layer.media.size", desc.Size),
tracing.Attribute("layer.media.digest", desc.Digest.String()),
)There was a problem hiding this comment.
Yes, this will track unpacking of each layer and create one span for each. This was added in response to #7453 (comment)
There was a problem hiding this comment.
What I mean here is whether we could rename tracing.SpanAttribute to tracing.Attribute
There was a problem hiding this comment.
Ahh I see. I have changed SpanAttribute to Attribute.
Signed-off-by: Swagat Bora <[email protected]>
swagatbora90
force-pushed
the
tracing-refactor
branch
from
249eb9b to
7def13d
Compare
5 participants
Follow up to the one of the comments in #7453 (comment)
This PR wraps the otel Span object and implements related methods which is used during tracing instrumentation.
Also, adds a new SpanName method as discussed in https://github.com/containerd/containerd to add a common seaprator in span names /pull/7453#discussion_r1012469550
Signed-off-by: Swagat Bora [email protected]