-
Notifications
You must be signed in to change notification settings - Fork 36
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
Fix for span leaks #1108
base: main
Are you sure you want to change the base?
Fix for span leaks #1108
Conversation
Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
var loggers []*childSpanLogger | ||
var ticker = time.NewTicker(time.Minute) | ||
var once sync.Once | ||
var lock sync.Mutex |
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.
Please let's get rid of static variables to use struct fields.
var loggers []*childSpanLogger | |
var ticker = time.NewTicker(time.Minute) | |
var once sync.Once | |
var lock sync.Mutex |
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.
ok, done
type childSpanLogger struct { | ||
span opentracing.Span | ||
childSpan opentracing.Span | ||
entries map[interface{}]interface{} | ||
lock sync.RWMutex | ||
operation string | ||
} |
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.
Please let's try to re-use exist span logger to avoid code duplication.
type childSpanLogger struct { | |
span opentracing.Span | |
childSpan opentracing.Span | |
entries map[interface{}]interface{} | |
lock sync.RWMutex | |
operation string | |
} | |
type todoSpanLogger struct { | |
*originalSpanLogger | |
} |
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.
ok, done
go func() { | ||
for { | ||
select { | ||
case <-ticker.C: | ||
for _, l := range loggers { | ||
if l.span != nil { | ||
l.lock.Lock() | ||
l.childSpan.Finish() | ||
l.childSpan = l.span.Tracer().StartSpan(l.operation, opentracing.ChildOf(l.span.Context())) | ||
l.lock.Unlock() | ||
} | ||
} | ||
case <-ctx.Done(): | ||
return | ||
} | ||
} | ||
}() |
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.
Please let's try to use cond
or fanout pattern for subscriptions on events and use shared state.
go func() { | |
for { | |
select { | |
case <-ticker.C: | |
for _, l := range loggers { | |
if l.span != nil { | |
l.lock.Lock() | |
l.childSpan.Finish() | |
l.childSpan = l.span.Tracer().StartSpan(l.operation, opentracing.ChildOf(l.span.Context())) | |
l.lock.Unlock() | |
} | |
} | |
case <-ctx.Done(): | |
return | |
} | |
} | |
}() |
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.
ok, done
b0e8abe
to
94446be
Compare
Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
94446be
to
9eef717
Compare
@Mixaster995 Is this still a good PR in need of review? |
@edwarnicke no, it was a test solution for reported problem, but the problem itself is not clear and really hard to reproduce or catch. Thus, we decided to postpone investigation of this problem until better times. |
@Mixaster995 Got it |
Signed-off-by: Mikhail Avramenko avramenkomihail15@gmail.com
Description
Added another span logger for 'infinite' Find case scenario(registry)
Issue link
#1085
How Has This Been Tested?
Types of changes