-
Notifications
You must be signed in to change notification settings - Fork 9
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
Generate runtime metrics #213
Conversation
78a7eea
to
a078a01
Compare
4b32c58
to
826dac6
Compare
8a6b7ec
to
ec76899
Compare
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
ec76899
to
ef4cac5
Compare
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.
Left some comments. I wonder thought how this will evolve when we transition to daemon mode, as I think Prometheus might make more sense in that case. IIRC the prometheus go library can collect this metrics for us.
pkg/runtime/profiler/metrics.go
Outdated
|
||
type metricsProbe struct { | ||
config MetricsConfig | ||
collector context.CancelFunc |
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.
collector
seems a bit strange as a name for a cancel function, what about cancelCollector
?
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.
Makes sense
pkg/runtime/profiler/metrics.go
Outdated
go func() { | ||
ticks := time.NewTicker(m.rate) | ||
for { | ||
select { | ||
case <-ticks.C: | ||
m.sample() | ||
case <-ctx.Done(): | ||
ticks.Stop() | ||
m.generate() | ||
} | ||
} | ||
}() |
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'm not seeing how this goroutine exits when the context is cancelled. Wouldn't the goroutine be looping forever when ctx.Done()
returns a closed channel?
A maybe simpler approach could be to:
go func() { | |
ticks := time.NewTicker(m.rate) | |
for { | |
select { | |
case <-ticks.C: | |
m.sample() | |
case <-ctx.Done(): | |
ticks.Stop() | |
m.generate() | |
} | |
} | |
}() | |
go func() { | |
ticks := time.NewTicker(m.rate) | |
defer ticks.Stop() | |
for { | |
select { | |
case <-ticks.C: | |
m.sample() | |
case <-ctx.Done(): | |
m.generate() | |
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.
Good catch. Thanks
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
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.
Looking good!
Description
Add options to the agent for generating go runtime metrics.
Fixes: #199
Checklist: