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

Add prometheus endpoint for metrics #740

Merged
merged 2 commits into from
Apr 30, 2020

Conversation

sysadmind
Copy link
Contributor

The existing metrics package used in dkron already supports prometheus, so this just adds in the necessary pieces to hook it up to the API. There is a config value to enable serving this endpoint to allow users the choice with a default of false to keep today's functionality. I know that it was discussed in #494 that the metrics system was likely going to be updated, but this seems like an easy step in the meantime.

The documentation for metrics was updated to align with current configuration keys.

The existing metrics package used in dkron already supports prometheus, so this just adds in the necessary pieces to hook it up to the API. There is a config value to enable serving this endpoint to allow users the choice with a default of false to keep today's functionality. I know that it was discussed in distribworks#494 that the metrics system was likely going to be updated, but this seems like an easy step in the meantime.

The documentation for metrics was updated to align with current configuration keys.
Copy link
Member

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

Thanks @sysadmind! I don't get why don't use prometheus.DefaultRegistry?

@sysadmind
Copy link
Contributor Author

I changed to using prometheus.DefaultRegistry. I initially thought that the metrics package was already setting up the metrics that come along with the DefaultRegistry, but when I went to compare more closely after your comment, I realized that the DefaultRegistry actually has a lot more metrics and they are more standardized across the prometheus community than those configured by the metrics package so I changed the PR.

Copy link
Member

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

❤️

@vcastellm vcastellm merged commit e2c31ac into distribworks:master Apr 30, 2020
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.

2 participants