-
Notifications
You must be signed in to change notification settings - Fork 451
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
feat: endpoint reachability plugin #720
base: main
Are you sure you want to change the base?
feat: endpoint reachability plugin #720
Conversation
Thanks for the PR @5war00p! Does this only check the entire content of the request to assume that it contains a URL? We also have another plugin - https://github.com/Portkey-AI/gateway/blob/main/plugins/default/validUrls.ts Is that conflicting? |
This just does almost same, but instead of GET request I'm using HEAD request also we as we need to validate endpoint is reaching or not just header is fine no need validate dns separately. |
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, just that this assumes that the response only contains a URL. in comparison,
https://github.com/Portkey-AI/gateway/blob/main/plugins/default/validUrls.ts feels more complete, I think it's a good idea to make a HEAD request to check the validity of a URL, you can perhaps add it as a fallback in the code of the existing plugin. That would be a 2 line change and more robust
To your point, |
let data = null; | ||
|
||
try { | ||
let text = getText(context, eventType); |
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.
My suggestion here is use regex to get all the URLs available from text and validate them using URL
or some other way.
With this we can have config weather to run plugin for all the endpoints or first endpoint found within the text.
What do you think? @5war00p
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.
Makes sense if text might contain >1 url, however this issue is to build a plugin for endpoint reachability right? and we should use this util func in another which extracts urls and check each url using this plugin. Correct me if I went off.
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.
Your thought makes sense, but this PR is a plugin PR which I think doesn't make sense to use it as utility function. I think we should focus on keep this as individual plugin. keeping that I prefer we should go for regex match.
As an enhancement I think we shouldn't just blindly hit the URL, we should ask user which type of method they're thinking of ex: GET
and POST
and let plugin do OPTIONS
request and make sure the endpoint(s) support the user requested methods.
let me know if this makes sense to you.
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 if thats the case we should make it as a new separate plugin.
Endpoint Reachability Plugin
Added new plugin that checks and validates whether an endpoint (url) is reachable or unreachable.
Motivation: (optional)
Its a preventive check to not do unnecessary actions without a proper input
Related Issues: (optional)