-
Notifications
You must be signed in to change notification settings - Fork 73
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
Collectors can now be async #304
Conversation
/** | ||
* Fetch metrics from all configured collectors | ||
*/ | ||
public async refresh () { |
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.
Does refresh
need a lock (or wrapper returning an ongoing promise) so it cannot get run more than once at a time?
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 think we're okay in this case. The caller should manage that, whether that be a bridge or the application requesting the /metrics endpoint. I don't think a race will cause any serious problems as you're just asking it to refresh metrics , which should be RO functions.
Back and forth in my head: |
This was my thinking. A bridge should ensure that collector functions do not block for serious amounts of time, and if they do then the /metrics endpoint should show that clearly by taking longer rather than us trying to make the endpoint return faster but with potentially the wrong values. |
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.
Looks good to me. 👍
Fixes #139