-
Notifications
You must be signed in to change notification settings - Fork 258
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
base: main
Are you sure you want to change the base?
Remove stats span #2331
Conversation
Signed-off-by: Kirtana Ashok <[email protected]>
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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
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. |
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