Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove stats span #2331

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+5 −10
Draft

Remove stats span #2331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions cmd/containerd-shim-runhcs-v1/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,16 +434,6 @@ func (s *service) Wait(ctx context.Context, req *task.WaitRequest) (resp *task.W
}

func (s *service) Stats(ctx context.Context, req *task.StatsRequest) (_ *task.StatsResponse, err error) {
ctx, span := oc.StartSpan(ctx, "Stats")
defer span.End()
defer func() { oc.SetSpanStatus(span, err) }()

span.AddAttributes(trace.StringAttribute("tid", req.ID))

if s.isSandbox {
span.AddAttributes(trace.StringAttribute("pod-id", s.tid))
}

r, e := s.statsInternal(ctx, req)
return r, errdefs.ToGRPC(e)
}
Expand Down
5 changes: 5 additions & 0 deletions pkg/octtrpc/interceptor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,11 @@ func ServerInterceptor(opts ...Option) ttrpc.UnaryServerInterceptor {
}
return func(ctx context.Context, unmarshal ttrpc.Unmarshaler, info *ttrpc.UnaryServerInfo, method ttrpc.Method) (_ interface{}, err error) {
name := convertMethodName(info.FullMethod)
// Do not create spans for stats calls as they could be called
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is hacky but not sure there is any other way if we want to necessarily drop only the stat spans coming from this interceptor. We could avoid this and possibly atleast drop the repetitive span coming from the service.Stats() call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this Interceptor is also creating duplicate spans since most of the APIs in service.go already create their own spans? Maybe we should just keep the spans in service.go and remove this interceptor? That way controlling what spans we want will be easy, plus those spans tend to include additional information (like container ID etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I did think of this and found this conversation in the original PR that added it #678 (comment) so left it as is. Can this be revisited @kevpar ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add a test case for stats to the interceptor tests added in #2330

// too frequently and flood logs.
if name == "containerd.task.v2.Task.Stats" {
return method(ctx, unmarshal)
}

var span *trace.Span
opts := []trace.StartOption{trace.WithSampler(o.sampler), oc.WithServerSpanKind}
Expand Down
Loading