-
Notifications
You must be signed in to change notification settings - Fork 264
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
[WIP] Fixes #298 Add time analysis of each engine for the results retrieved #341
Conversation
These changes are include the time taken by the network request from client to server and back. We should only display the time taken to generate the results, that is returning the time along with results. |
The changes should be in backend code and not in front end IMHO. |
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.
Please shift the time analysis code to backend.
As we want the time analysis of response generation and not the time taken by the server to process the request and send back the response.
In the current scenario the network lag will cause the time to increase whereas the actual time of generation will be different.
@AnshulMalik @raju249 @gabru-md The thing is we cannot include time analysis in backend as API |
I think we don't have to include or change anything in the URL. Others, any thoughts ? |
@bhaveshAn That's good point. |
@raju249, our response is currently just array, not a dictionary so we can not directly add another key to response. |
Ohh. |
Scrapers currently return one parameter: the urls list. It would be possible to modify the scrapers to return two parameters instead: the urls list and the elapsed time. The first parameter would be returned to the calling program and the second parameter would only be used for display, logging, or printing. |
That's possible @cclauss, but when I think about it's use case, I don't think this would be useful anywhere? |
As @AnshulMalik stated. I guess there is no definite benefit/use of actually including the time elapsed into query-server results. I mean i cannot figure out the possible benefits that it will have since it is not a |
@bhaveshAn reminder |
I actually like this PR a lot... In modern software you should "instrument everything" and round trip time measurement is worth doing as long as at least one user is going to study the resulting data from time to time to understand if we have any bottlenecks that are worth addressing. |
@cclauss considering round trip time is not recommended, since it can vary a lot depending upon user connection, It won't help anyhow, but marking time for fetching and parsing on backend can be a matrix which we can rely on. |
Our code's performance is roughly: the time to run server.search() - the time to run each scraper's search(). Once we have tuned the former, we can focus on which search engines are too slow. My bet would be that some are faster than others but without metrics, we do not know for sure. Some search engines might be so slow that we drop them. |
I think unless we pass the time in response from server, adding time via |
What is the status of this? Should this be closed and reopened later, when there is a follow-up? |
@mariobehling I think we should go with adding this feature, since time analysis comes under performance category. And thats cool idea.
|
@vaibhavsingh97 What are your thoughts? |
@mariobehling @bhaveshAn IMO, instead of checking time on client side, we should add one time parameter to the backend API and return the response along with the time. But my question is where time will be useful? |
@bhaveshAn Update please |
@vaibhavsingh97 Should I make these corresponding changes. |
@bhaveshAn yes, just pass one |
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.
Please resolve conflicts.
This PR needs updation in codebase. Please give some time. |
I agree time should be retrieved from the server, not the client. Work on this issue is stalled. Closing this due to inactivity. Please reopen new PR if you want to resolve this. |
Fixes #298
Checklist
master
branch.Changes proposed in this pull request:
@vaibhavsingh97 @gabru-md Please review the PR.
Providing heroku deployment at https://hidden-harbor-34393.herokuapp.com/