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.

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
Draft

Remove stats span #2331

wants to merge 1 commit into from

Conversation

kiashok
Copy link
Contributor

@kiashok kiashok commented Dec 3, 2024

Stats calls are made quite often by customers and the spans created could flood the logs. This change is removing the spans for stats calls.

The following spans are being removed:

time="2024-11-22T06:19:59.305267800Z" level=info msg=Span duration="708.4µs" endTime="2024-11-22T06:19:59.305976200Z" name=containerd.task.v2.Task.Stats parentSpanID=0000000000000000 spanID=e9bfa06a09bb9548 spanKind=server startTime="2024-11-22T06:19:59.305267800Z" traceID=ddf4f2231ce67194bd833c15a4c73fcf
time="2024-11-22T06:19:59.304139100Z" level=info msg=Span duration=2.1059ms endTime="2024-11-22T06:19:59.306245000Z" name=Stats parentSpanID=30ddb65909c4d7f1 pod-id=83162ca3a00729c49df470140aab6a966c0e2950d4e7267eb488f203ac06cf19 spanID=914dea16815abd59 startTime="2024-11-22T06:19:59.304139100Z" tid=4b2c96383294ae1a2ae8d764fb4518c8b5dbbe1b745da379623aa30f154374ca traceID=a207fe1ca8efe6033aaa7ba74ae4fb5c

Signed-off-by: Kirtana Ashok <[email protected]>
@kiashok
Copy link
Contributor Author

kiashok commented Dec 3, 2024

@ambarve @helsaawy @kevpar @msscotb thoughts?

@@ -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

@kevpar
Copy link
Member

kevpar commented Dec 13, 2024

I think it's probably best if span filtering is implemented in the logrus exporter. This would allow us to still get the spans if we use another exporter, such as one where the span volume is not as big of an issue.

I'd also suggest implementing this as a configurable option for which spans to filter. That way it can be set dynamically rather than needing to hardcode it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants