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

metrics: change periodic update to synchronous collect. #185

Merged
merged 6 commits into from
Feb 13, 2020

Conversation

morimoto-cybozu
Copy link
Contributor

Signed-off-by: morimoto-cybozu kenji_morimoto@cybozu.co.jp

Signed-off-by: morimoto-cybozu <kenji_morimoto@cybozu.co.jp>
Signed-off-by: morimoto-cybozu <kenji_morimoto@cybozu.co.jp>
Signed-off-by: morimoto-cybozu <kenji_morimoto@cybozu.co.jp>
Signed-off-by: morimoto-cybozu <kenji_morimoto@cybozu.co.jp>
@morimoto-cybozu morimoto-cybozu self-assigned this Feb 10, 2020
@morimoto-cybozu morimoto-cybozu changed the title web: remove unnecessary log. metrics: change periodic update to synchronous collect. Feb 10, 2020
@morimoto-cybozu morimoto-cybozu marked this pull request as ready for review February 10, 2020 14:28
Signed-off-by: morimoto-cybozu <kenji_morimoto@cybozu.co.jp>
Comment on lines 91 to 112
env := well.NewEnvironment(ctx)
for key, metric := range c.metrics {
key, metric := key, metric
env.Go(func(ctx context.Context) error {
err := metric.updater(ctx, c.model)
if err != nil {
log.Warn("unable to update metrics", map[string]interface{}{
"name": key,
log.FnError: err,
})
// return nil not to disturb collection of other metrics
return nil
}

for _, col := range metric.collectors {
col.Collect(ch)
}
return nil
})
}
env.Stop()
env.Wait()
Copy link
Member

Choose a reason for hiding this comment

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

well.Environment might be an overkill.
sync.WaitGroup is just enough because there are no error handling code.

Signed-off-by: morimoto-cybozu <kenji_morimoto@cybozu.co.jp>
Copy link
Member

@ymmt2005 ymmt2005 left a comment

Choose a reason for hiding this comment

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

LGTM

@ymmt2005 ymmt2005 merged commit 565046c into master Feb 13, 2020
@ymmt2005 ymmt2005 deleted the synchronous-metrics-scraping branch February 13, 2020 07:37
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.

None yet

2 participants