-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
use got to push influx metrics #6257
Conversation
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.
One question for my own edification, but LGTM
auth, | ||
username: this._config.username, | ||
password: this._config.password, | ||
throwHttpErrors: false, |
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.
this just prevents got
from throwing on some non-successful response status code right?
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.
Yep. By default got throws an exception if it gets a 404, 500 or whatever. This tells it to return a response. I mainly did this because the existing error handling code assumes this behaviour from request, but I could have rewritten the error handling code here
shields/core/server/influx-metrics.js
Lines 33 to 45 in fd7eddc
} catch (error) { | |
log.error( | |
new Error(`Cannot push metrics. Cause: ${error.name}: ${error.message}`) | |
) | |
} | |
if (response && response.statusCode >= 300) { | |
log.error( | |
new Error( | |
`Cannot push metrics. ${response.request.href} responded with status code ${response.statusCode}` | |
) | |
) | |
} | |
} |
to handle both cases in the catch block instead. I don't think it makes much difference either way though.
refs #4655
Nice bit of low hanging fruit here 🍏
Because this is stand-alone utility code and not part of the services I haven't routed it through
fetchFactory
/sendRequest
. I've just gone straight from request to got.