Skip to content
This repository has been archived by the owner on Jun 13, 2023. It is now read-only.

Add a prometheus exporter #57

Merged
merged 5 commits into from
Jun 21, 2018
Merged

Add a prometheus exporter #57

merged 5 commits into from
Jun 21, 2018

Conversation

JulienBalestra
Copy link
Collaborator

@JulienBalestra JulienBalestra commented Jun 20, 2018

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

}
metricFile.Sync()
glog.Infof("Generated metrics file in %s", metricFile.Name())

Copy link
Contributor

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)
Copy link
Contributor

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

Copy link
Collaborator Author

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 ?

Copy link
Contributor

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?

Copy link
Collaborator Author

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 ?

mfpierre
mfpierre previously approved these changes Jun 21, 2018
Copy link
Contributor

@mfpierre mfpierre left a 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 ✅ )

@JulienBalestra JulienBalestra merged commit 0fd001d into master Jun 21, 2018
@JulienBalestra JulienBalestra deleted the JulienBalestra/state branch June 21, 2018 14:22
@JulienBalestra JulienBalestra mentioned this pull request Jun 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants