-
Notifications
You must be signed in to change notification settings - Fork 18
Conversation
a49389c
to
c815f4f
Compare
scripts/update/docs.go
Outdated
} | ||
metricFile.Sync() | ||
glog.Infof("Generated metrics file in %s", metricFile.Name()) | ||
|
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.
extra line
Help: "Total number of kubelet probe failures", | ||
}), | ||
} | ||
err := prometheus.Register(s.promVersion) |
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.
you could remove the error
from NewState
and use MustRegister
instead of Register
which would make the program panic if failing, this would clean up all the error handling code as well
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.
@mfpierre I wasn't sure about that, do you think it worth it ?
Why panic should be better than an error ?
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.
Don't you exit anyway if it fails?
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.
Yes but a panic in the middle of my code path sounds weird to me. Don't you think ?
37f3a17
to
62a81e8
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.
the code LGTM 📊
(ok to merge once the CI is green ✅ )
62a81e8
to
75c935e
Compare
What does this PR do?
Register a Prometheus exporter with custom metrics to improve the observability of pupernetes.
This is a part of #54
Additional Notes
This PR should be merged after the huge refactoring in #56